Hi Joro, Sergey,
On 02/04/2011 06:30 PM, Georgi Kodinov wrote:
> Hello,
>
> comments inline.
>
> On 04.02.2011, at 16:50, Sergey Vojtovich wrote:
>
>> On Fri, Feb 04, 2011 at 01:57:49PM +0300, Alexander Barkov wrote:
>>> 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.
>> Ok. After all I can't recall why it could be a problem. Though
>> looking at 5.1 I can see MY_ALLOW_ZERO_PTR. Probably something
>> safemalloc related.
>
> should be ok imho.
>
>>>> 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.
>> Well, we're actually talking about parse_com_change_user_packet(),
>> which is called only when command is COM_CHANGE_USER.
>>
>> The second place, which is COM_CONNECT must not expect that.
>
> Actually acl_authenticate() doesn't need a user name at all. It gets it either
> from the client handshake packet (in case of login) or from the change user packet
> (in case of COM_CHANGE_USER).
>
> So it seems that I wasn't thinking about possibilities a lot when adding
>
> mpvio->auth_info.user_name= thd->security_ctx->user;
>
> to
>
> server_mpvio_initialize().
>
> Now it's not wrong per se, as on login initially the name is set to NULL and in the
> COM_CHANGE_USER case it's set to the current user's name. But it's superfluous and not
> needed (and not used until set).
> We can just as well set mpvio->auth_info.user_name to NULL (respectively length to
> 0) inside server_mpvio_initialize() and call it a day.
Thanks. This worked fine!
I like this fix better, as it eliminates the source of the problem.
(my hack was a workaround to get rid of consequences)
>
> Btw, I don't really like the name charset_is_good_for_parser() :)
>
> IMHO it should be is_supported_parser_charset.
Changed.
This is the modified version:
http://lists.mysql.com/commits/130993
Thanks!
>
> Best Regards,
> Joro
