Sven Sandberg wrote:
> Hi Mats,
>
> The patch improves the situation by not crashing on the particular
> binlog attached to the bug report. But there are still many ways that
> similar errors could occur:
Unfortunately, yes. There's a big bunch of missing checks in that file,
so many that I deem it unfeasible to fix all of them, since it requires
a significant amount of work to find and fix. We need a more thorough
change of the code (on the level of abolishing use of memcpy() in favor
of our own safe version) to make sure that all is patched.
I will fix the ones you mention below and spend some effort to find
other more blatant ones, and then commit another patch. We need to get a
patch for the most obvious ones out, so that we can do some significant
refactoring instead of constantly patching existing code.
Just my few cents,
Mats Kindahl
>
> - Each status case in the `switch (*pos++)' in this Query_log_event
> constructor can potentially read beyond `end'. I'd suggest adding a
> macro like this:
>
> #define ENSURE_SPACE(N) \
> do { \
> if (pos + N > end) { \
> query = 0; \
> DBUG_VOID_RETURN; \
> } \
> } while (0);
>
> And use it like this:
>
> @@ log_event.cc:1496
> case Q_FLAGS2_CODE:
> flags2_inited= 1;
> + ENSURE_SPACE(4);
> flags2= uint4korr(pos);
> DBUG_PRINT("info",("In Query_log_event, read flags2: %lu",
> (ulong) flags2));
> pos+= 4;
> break;
>
> etc.
>
> - The same should be tested for catalog and timezone too.
>
> - Other event types have similar problems. E.g.,
> Load_log_event::copy_log_event should check field_block_len,
> table_name_len, and fname_len.
>
> /Sven
>
>
> Mats Kindahl wrote:
>> Patch submitted manually
>>
>>
>> ------------------------------------------------------------------------
>>
>> # This is a BitKeeper generated diff -Nru style patch.
>> #
>> # ChangeSet
>> # 2007/11/07 14:59:24+01:00 mats@stripped # BUG#31793 (log
>> event corruption causes crash):
>> # # When running mysqlbinlog on a 64-bit machine with a corrupt
>> relay log,
>> # it causes mysqlbinlog to crash. In this case, the crash is caused
>> # because a request for 18446744073709534806U bytes is issued, which
>> # apparantly can be served on a 64-bit machine (speculatively, I
>> assume)
>> # but this causes the memcpy() issued later to copy the data to
>> segfault.
>> # # The request for the number of bytes is caused by a computation
>> # of data_len - server_vars_len where server_vars_len is corrupt in
>> such
>> # a sense that it is > data_len. This causes a wrap-around, with the
>> # the data_len given above.
>> # # This patch adds a check that if server_vars_len is greater than
>> # data_len before the substraction, and aborts reading the event in
>> # that case marking the event as invalid. # #
>> mysql-test/std_data/corrupt-relay-bin.000624
>> # 2007/11/07 14:58:30+01:00 mats@stripped +2033 -0
>> # BitKeeper file
>>
> /home/mats/devel/b31793-mysql-5.0-rpl/mysql-test/std_data/corrupt-relay-bin.000624
>>
>> # # mysql-test/std_data/corrupt-relay-bin.000624
>> # 2007/11/07 14:58:30+01:00 mats@stripped +0 -0
>> # # mysql-test/t/mysqlbinlog.test
>> # 2007/11/07 14:59:22+01:00 mats@stripped +4 -0
>> # Adding test that it fails gracefully for a corrupt relay log.
>> # # sql/log_event.cc
>> # 2007/11/07 14:59:22+01:00 mats@stripped +11 -0
>> # Adding check that status var length does not cause wrap-around
>> # when performing subtraction.
>> # Binary files a/mysql-test/std_data/corrupt-relay-bin.000624 and
>> b/mysql-test/std_data/corrupt-relay-bin.000624 differ
>> diff -Nru a/mysql-test/t/mysqlbinlog.test
>> b/mysql-test/t/mysqlbinlog.test
>> --- a/mysql-test/t/mysqlbinlog.test 2007-11-07 15:00:13 +01:00
>> +++ b/mysql-test/t/mysqlbinlog.test 2007-11-07 15:00:13 +01:00
>> @@ -237,4 +237,8 @@
>> --echo $c
>> drop table t1;
>>
>> +echo shell> mysqlbinlog std_data/corrupt-relay-bin.000624 >
>> var/tmp/bug31793.sql;
>> +error 1;
>> +exec $MYSQL_BINLOG $MYSQL_TEST_DIR/std_data/corrupt-relay-bin.000624
>> > $MYSQLTEST_VARDIR/tmp/bug31793.sql;
>> +
>> --echo End of 5.0 tests
>> diff -Nru a/sql/log_event.cc b/sql/log_event.cc
>> --- a/sql/log_event.cc 2007-11-07 15:00:13 +01:00
>> +++ b/sql/log_event.cc 2007-11-07 15:00:13 +01:00
>> @@ -1475,6 +1475,17 @@
>> if (tmp)
>> {
>> status_vars_len= uint2korr(buf + Q_STATUS_VARS_LEN_OFFSET);
>> + /*
>> + Check if status variable length is corrupt and will lead to very
>> + wrong data. We could be even more strict and require data_len to
>> + be even bigger, but this will suffice to catch most corruption
>> + errors that can lead to a crash.
>> + */
>> + if (status_vars_len >= data_len + 1)
>> + {
>> + query= 0;
>> + DBUG_VOID_RETURN;
>> + }
>> data_len-= status_vars_len;
>> DBUG_PRINT("info", ("Query_log_event has status_vars_len: %u",
>> (uint) status_vars_len));
>>
>>
>>
>> ------------------------------------------------------------------------
>>
>>
>
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
