Alexander,
ok, now I understand what is it all about. But I must admit that it is
something contrary to evidence.
I approve the patch with two remarks:
1. In sql_parse.cc there is "my_free(thd->security_ctx->user);"
But user may be NULL and AFAIR free() doesn't always like NULL.
2. I think there is another, cleaner way to solve this double free
problem: set thd->security_ctx->user to NULL in sql_parse.cc. But
I think it is a good idea to ask Joro what is the right way of
doing things and to do second review.
Regards,
Sergey
On Fri, Feb 04, 2011 at 10:59:07AM +0300, Alexander Barkov wrote:
> Hi Sergey,
>
> Sergey Vojtovich wrote:
> >Hi Alexander,
> >
> >still, there is one question...
> >
> >On Tue, Feb 01, 2011 at 03:36:16PM -0000, Alexander Barkov wrote:
> >>#At file:///home/bar/mysql-bzr/mysql-5.5.b58036/ based on
> revid:ole.john.aske@stripped
> >>
> >> 3294 Alexander Barkov 2011-02-01
> >BUG#.... line is missing.
> >
> >> Problem: ucs2 was correctly disallowed in "SET NAMES" only,
> >> while mysql_real_connect() and mysql_change_user() still allowed
> >> to use ucs2, which made server crash.
> >...skip...
> >
> >>=== modified file 'sql/sql_acl.cc'
> >>--- a/sql/sql_acl.cc 2011-01-14 15:48:11 +0000
> >>+++ b/sql/sql_acl.cc 2011-02-01 15:30:06 +0000
> >>@@ -7799,7 +7799,8 @@ public:
> >> Thd_charset_adapter(THD *thd_arg) : thd (thd_arg) {} bool
> >>init_client_charset(uint cs_number)
> >> {
> >>- thd_init_client_charset(thd, cs_number);
> >>+ if (thd_init_client_charset(thd, cs_number))
> >>+ return true;
> >> thd->update_charset();
> >> return thd->is_error();
> >> }
> >>@@ -8236,6 +8237,18 @@ static bool parse_com_change_user_packet
> >> uint dummy_errors;
> >> DBUG_ENTER ("parse_com_change_user_packet");
> >>+
> >>+ /*
> >>+ The caller expects that this function allocates user_name using
> >>+ my_strndup() and calls my_free(user_name) later in some cases.
> >>+
> >>+ Let's set user_name to NULL here, to avoid my_free() on uninitialized
> >>+ memory for those cases when we return from here *before* user_name is
> >>+ actually allocated. This happens on errors: packet error, bad charset.
> >>+ */
> >>+ mpvio->auth_info.user_name= NULL;
> >>+ mpvio->auth_info.user_name_length= 0;
> >>+
> >> if (passwd >= end)
> >> {
> >> my_message(ER_UNKNOWN_COM_ERROR, ER(ER_UNKNOWN_COM_ERROR), MYF(0));
> >>
> >When does the caller set auth_info.user_name to not NULL and when does the
> >caller free it?
>
>
> Normally, parse_com_change_user_packet() does the following:
>
> /* we should not free mpvio->user here: it's saved by
> dispatch_command() */
> if (!(mpvio->auth_info.user_name= my_strndup(user_buff, user_len,
> MYF(MY_WME)
> return 1;
>
>
> So it either allocates user_name or sets it to NULL
> on my_strndup failure.
>
>
> Now suppose we exited earlier, before my_strndup() happened.
>
>
>
>
> bool dispatch_command(...)
> {
> ...
>
> case COM_CHANGE_USER:
>
> ....
> Security_context save_security_ctx= *thd->security_ctx;
> ....
> rc= acl_authenticate(thd, 0, packet_length);
> ...
> if (rc)
> {
> ...
> /*
> If acl_authenticate() returned before my_strndup(), then
> thd->security_ctx->user points to the original user value
> which was when we entered this function.
> But! save_security_ctx->user also points to the same user string.
> */
>
> my_free(thd->security_ctx->user); // We assume it was alloced here!
> *thd->security_ctx= save_security_ctx;
>
> /*
> Now thd->security_ctx points to already freed memory
> */
> ...
> }
>
>
>
>
> Then, sooner or later, THD executes:
>
> thd->security_ctx::destroy(),
>
> which does my_free(user) on the previously freed address.
>
>
--
Sergey Vojtovich <svoj@stripped>
MySQL AB, Software Engineer
Izhevsk, Russia, www.mysql.com
