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

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