From: Alexander Barkov
Date: February 4 2011 10:57am
Subject: Re: bzr commit into mysql-5.5 branch (alexander.barkov:3294)
List-Archive: http://lists.mysql.com/commits/130390
Message-Id: <4D4BDBAD.9020506@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi Joro, Sergey,

Can you please review my patch for Bug#58036?

The full patch for 5.5 is here:

http://lists.mysql.com/commits/130143

(I'll sent a patch for 5.1 separately, as the
affected code was significantly different in 5.1)


Thanks!


Sergey, thanks for review!

Please see my comments inline:


Sergey Vojtovich wrote:
> 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.

free() likes NULL in C89/C99.


> 
> 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.

Possibly. My reasoning was, as far as acl_authentificate() is called in
at least two places, it's fair to say that both places
expect thd->security_ctx->user to be alloced (or to be NULL),
and they both expect than thd->security_ctx->user never stays untouched.
So this is a minimal fix to cover both calls.

If Joro suggests a better fix for this problem,
then I'll be happy to incorporate into my patch:


+
+  /*
+    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;
+

Thanks!


> 
> 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.
>>
>>
>