List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:January 18 2011 9:42am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3514)
Bug#58026
View as plain text  
On 1/18/11 5:11 AM, Dmitry Shulga wrote:
> Hi Davi!
>
> On 17.01.2011, at 15:18, Davi Arnaut wrote:
>
>> Hi Dmitry,
>>
>> On 11/24/10 7:07 AM, Dmitry Shulga wrote:
>>> #At file:///Users/shulga/projects/mysql/5.1-bugteam-bug58026/
>>> based on
>>> revid:davi.arnaut@stripped
>>>
>>> 3514 Dmitry Shulga	2010-11-24 Fixed bug#58026 - massive recursion
>>> and crash in regular expression handling.
>>
>> Looks good so far, a few comments below.
>>
>>> The problem was that parsing of nested regular expression
>>> involved recursive calls. Such recursion was not take care of
>>> available memory,
>>
>> "Such recursion was not take care of available memory," sounds
>> strange.
>>
>> I suggest rewriting to:
>>
>> "Such recursion didn't take into account the amount of available
>> stack space, which ended up leading to stack overflow crashes."
> Done
>>
>>> which may lead to stack overflow crashes. @ client/mysqltest.cc
>>> Passed NULL pointer to function as last actual argument in call
>>> to my_regcomp. In this case check for enough memory in stack
>>> doesn't occur. Such checking doesn't need in mysql clients.
>>
>> "Such checking is not necessary in mysql clients"
> Done
>>>
>> Please introduce a typedef for the stack check function.
>> Suggestion:
>>
>> typedef int (*my_regex_stack_check_t)();
>>
>> To be added to my_regex.h
> Done.
>
>>>
>>> +extern "C" +int +check_enough_stack_size()
>>
>> Hum, since the hook is global, there isn't much reason to pass the
>> stack check function each time we need to compile a expression.
>>
>> Just add the hook to regcomp.c with internal linkage:
>>
>> static my_regex_stack_check_t stack_check;
>>
>> and we let a hook be registered at my_regex_init time:
>>
>> my_regex_init(CHARSET_INFO *cs, my_regex_stack_check_t
>> stack_check);
>>
>> In this case, my_regex_init sets the hook to the given one.
> I don't agree with your suggestion. First, because solution with
> explicit argument is more meaningful. With your approach usage of
> check_stack_overrun function is implicit and hidden inside body of
> my_regex_init/my_regcomp. In my approach check_stack_overrun passed
> explicitly and developer reading this source code clearly understand
> what I do there.

If we followed this line of thought, the same argument could be used for 
any initialization that is done through my_regex_init.

Besides, what I suggest is a already established pattern in MySQL code. 
For example, look at how the error hooks are routed back into the 
server, should we add a argument to every my_error just because it might 
be hidden inside the body of my_error? Sorry, but your argument is not 
sound. Functions do implicit things, that's what they are useful for.

> Second, because current_thd is not defined for some client programs,
> mysqltest for example.

The hook won't be set in these programs.

>>
>>> +{ +#ifndef EMBEDDED_LIBRARY    // Avoid compiler warning +
>>> uchar stack_top; +#endif +  return
>>> check_stack_overrun(current_thd, STACK_MIN_SIZE, +&stack_top);
>>
>> This won't compile with embedded? stack_top won't have been
>> declared.
> As for the embedded version of mysql the function
> check_stack_overrun() set to nothing without this #ifdef we got next
> error during compilation:
>
> cc1plus: warnings being treated as errors item_cmpfunc.cc: In
> function ‘int check_enough_stack_size()’: item_cmpfunc.cc:4799:
> warning: unused variable ‘stack_top’

Ifdef the whole thing for embedded then..

Regards,

Davi
Thread
bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3514) Bug#58026Dmitry Shulga24 Nov
  • Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3514)Bug#58026Davi Arnaut17 Jan
    • Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3514) Bug#58026Dmitry Shulga18 Jan
      • Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3514)Bug#58026Davi Arnaut18 Jan
        • Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3514) Bug#58026Dmitry Shulga19 Jan
          • Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3514)Bug#58026Davi Arnaut19 Jan