Patch approved.
Please, add to the long run goals a discussion on how to handle
such problems because most likely there are other cases similar
to this.
Cheers.
Luis Soares wrote:
> #At file:///home/lsoares/Workspace/bzr/work/bugfixing/48357/mysql-5.1/ based on
> revid:build@stripped
>
> 3195 Luis Soares 2009-11-09
> BUG#48357: SHOW BINLOG EVENTS: Wrong offset or I/O error
>
> In function log_event.cc:Query_log_event::write, there was a cast that
> was triggering undefined behavior. The offending cast is the
> following:
>
> write_str_with_code_and_len((char **)(&start),
> catalog, catalog_len, Q_CATALOG_NZ_CODE);
>
> This results in calling write_str_with_code_and_len with first
> argument pointing to a (char **) while "start" is itself a pointer to
> uchar (uchar *). Inside write_str_with_..., the content of start is
> then be updated:
>
> (*dst)+= len;
>
> The instruction above would cause the (*dst) pointer (ie, the "start"
> argument, from the caller point of view, and which actually points to
> uchar instead of pointing to char) to be updated so that it would
> increment catalog_len. However, this seems to break strict-aliasing
> rules ultimately causing the increment and assignment to behave
> unexpectedly.
>
> We fix this by removing the cast and by making the types match.
>
> modified:
> sql/log_event.cc
> === modified file 'sql/log_event.cc'
> --- a/sql/log_event.cc 2009-10-23 03:13:42 +0000
> +++ b/sql/log_event.cc 2009-11-09 17:36:13 +0000
> @@ -2138,8 +2138,8 @@ void Query_log_event::pack_info(Protocol
> /**
> Utility function for the next method (Query_log_event::write()) .
> */
> -static void write_str_with_code_and_len(char **dst, const char *src,
> - int len, uint code)
> +static void write_str_with_code_and_len(uchar **dst, const char *src,
> + uint len, uint code)
> {
> /*
> only 1 byte to store the length of catalog, so it should not
> @@ -2234,7 +2234,7 @@ bool Query_log_event::write(IO_CACHE* fi
> }
> if (catalog_len) // i.e. this var is inited (false for 4.0 events)
> {
> - write_str_with_code_and_len((char **)(&start),
> + write_str_with_code_and_len(&start,
> catalog, catalog_len, Q_CATALOG_NZ_CODE);
> /*
> In 5.0.x where x<4 masters we used to store the end zero here. This was
> @@ -2272,7 +2272,7 @@ bool Query_log_event::write(IO_CACHE* fi
> {
> /* In the TZ sys table, column Name is of length 64 so this should be ok */
> DBUG_ASSERT(time_zone_len <= MAX_TIME_ZONE_NAME_LENGTH);
> - write_str_with_code_and_len((char **)(&start),
> + write_str_with_code_and_len(&start,
> time_zone_str, time_zone_len, Q_TIME_ZONE_CODE);
> }
> if (lc_time_names_number)
>
>
>
> ------------------------------------------------------------------------
>
>
