List:Commits« Previous MessageNext Message »
From:Dmitry Shulga Date:January 18 2011 7:11am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3514) Bug#58026
View as plain text  
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.

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

> 
>> +{
>> +#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’
 
Regards, Dmitry.

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