From: Dmitry Shulga
Date: January 18 2011 7:11am
Subject: Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3514) Bug#58026
List-Archive: http://lists.mysql.com/commits/129023
Message-Id: <BA27139F-7BEF-45B7-A28A-B672A9E88384@oracle.com>
MIME-Version: 1.0 (Apple Message framework v1082)
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: quoted-printable

Hi Davi!

On 17.01.2011, at 15:18, Davi Arnaut wrote:

> Hi Dmitry,
>=20
> 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
>>=20
>>  3514 Dmitry Shulga	2010-11-24
>>       Fixed bug#58026 - massive recursion and crash in regular =
expression
>>       handling.
>=20
> Looks good so far, a few comments below.
>=20
>>       The problem was that parsing of nested regular expression =
involved
>>       recursive calls. Such recursion was not take care of available =
memory,
>=20
> "Such recursion was not take care of available memory," sounds =
strange.
>=20
> I suggest rewriting to:
>=20
> "Such recursion didn't take into account the amount of available stack =
space, which ended up leading to stack overflow crashes."
Done
>=20
>>       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.
>=20
> "Such checking is not necessary in mysql clients"
Done
>>=20
> Please introduce a typedef for the stack check function. Suggestion:
>=20
>    typedef int (*my_regex_stack_check_t)();
>=20
> To be added to my_regex.h
Done.

>>=20
>> +extern "C"
>> +int
>> +check_enough_stack_size()
>=20
> 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.
>=20
> Just add the hook to regcomp.c with internal linkage:
>=20
>    static my_regex_stack_check_t stack_check;
>=20
> and we let a hook be registered at my_regex_init time:
>=20
>    my_regex_init(CHARSET_INFO *cs, my_regex_stack_check_t =
stack_check);
>=20
> 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=20
hidden inside body of my_regex_init/my_regcomp. In my approach =
check_stack_overrun passed explicitly and developer reading this source =
code=20
clearly understand what I do there.

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

>=20
>> +{
>> +#ifndef EMBEDDED_LIBRARY    // Avoid compiler warning
>> +  uchar stack_top;
>> +#endif
>> +  return check_stack_overrun(current_thd, STACK_MIN_SIZE,
>> +&stack_top);
>=20
> 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 =91int check_enough_stack_size()=92:
item_cmpfunc.cc:4799: warning: unused variable =91stack_top=92
=20
Regards, Dmitry.

>=20
> Regards,
>=20
> Davi