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