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

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

Perhaps adding this line before my_free() is a good idea:

   DBUG_ASSER(thd->security_ctx->user != save_security_ctx->user);


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