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
