Re: bzr commit into mysql-5.5 branch (alexander.barkov:3294)

[Alexander Barkov]

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.