List:Commits« Previous MessageNext Message »
From:Sven Sandberg Date:September 23 2010 9:09am
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (luis.soares:3298)
View as plain text  
Hi Luís,

Great analysis of that stack trace, that must have been hard to find!

There are a couple of small problems with the patch, and further 
problems with the base64 decoder which I think we should take the 
opportunity to fix. So I reject the patch.

(1) You need to allocate space for the extra character, in case this is 
the last event decoded.

(2) You need to save and restore whatever was at the last character, in 
case this is not the last event decoded.

(3) The base64 decoder has several bugs. The most important one is that 
it reads and writes outside allocated memory if given malformed input 
(e.g., if one character is lost from the string by user mistake). 
Instead of figuring out all corner cases where it does wrong, I wrote a 
new version. Here:
I suggest we use something like this.


Luis Soares wrote:
> #At file:///home/lsoares/Workspace/bzr/work/bugfixing/56883/mysql-next-mr-bugfixing/
> based on revid:mats.kindahl@stripped
>  3298 Luis Soares	2010-09-22
>       BUG#56883: rpl_row_ignorable_event fails on valgrind run
>       The buffer used in mysql_client_binlog_statement is never
>       initialized. If an event is processed and its payload is handled
>       as a null terminated string, then conditional jumps depending on
>       uninitialized values may ocur. This was the case for
>       Rows_query_log_event.
>       We fix this by always setting a null terminator mark on byte
>       'buf+event_len' when an event is decoded from the BINLOG
>       statement. Given that the buffer is reused for all events on a
>       BINLOG statement, then this is also an extra security measure
>       against dumping garbage from a previous event somehow...
>     modified:
>       sql/
> === modified file 'sql/'
> --- a/sql/	2010-09-21 11:32:50 +0000
> +++ b/sql/	2010-09-22 00:30:33 +0000
> @@ -232,6 +232,14 @@ void mysql_client_binlog_statement(THD* 
>        DBUG_PRINT("info", ("event_len=%lu, bytes_decoded=%d",
>                            event_len, bytes_decoded));
> +      /* 
> +        'buf' is reused on every iteration (new event decoding and apply), 
> +        so better make it a NULL terminated string, so that no conditional 
> +        jumps depending on uninitialized strings happen. (eg, as potentially 
> +        that could be the case in Rows_query_log_event - BUG#56883).
> +       */
> +      *(bufptr+event_len)= '\0';
> +
>        if (check_event_type(bufptr[EVENT_TYPE_OFFSET], rli))
>          goto end;
> ------------------------------------------------------------------------

bzr commit into mysql-next-mr-bugfixing branch (luis.soares:3298) Bug#56883Luis Soares22 Sep
  • Re: bzr commit into mysql-next-mr-bugfixing branch (luis.soares:3298)Bug#56883Sven Sandberg23 Sep
    • Re: bzr commit into mysql-next-mr-bugfixing branch (luis.soares:3298)Bug#56883Luís Soares24 Sep