# This is a BitKeeper generated diff -Nru style patch. # # ChangeSet # 2007/11/08 16:03:19+01:00 mats@capulet.net # 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. It also adds checks to see # that reading the server variables does not go outside the bounds # of the available space, giving a limited amount of integrity check. # # mysql-test/r/mysqlbinlog.result # 2007/11/08 16:03:17+01:00 mats@capulet.net +1 -0 # Result change. # # mysql-test/std_data/corrupt-relay-bin.000624 # 2007/11/08 16:03:17+01:00 mats@capulet.net +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/08 16:03:17+01:00 mats@capulet.net +0 -0 # # mysql-test/t/mysqlbinlog.test # 2007/11/08 16:03:17+01:00 mats@capulet.net +4 -0 # Adding test that it fails gracefully for a corrupt relay log. # # sql/log_event.cc # 2007/11/08 16:03:17+01:00 mats@capulet.net +81 -11 # Adding check that status var length does not cause wrap-around # when performing subtraction. Extending get_str_len_and_pointer() to # check that the string can actually be read without reading outside # bounds. Adding checks when reading server variables from the Query- # log_event so that the variable can really be read. Abort reading # and mark the event as invalid otherwise. # diff -Nru a/mysql-test/r/mysqlbinlog.result b/mysql-test/r/mysqlbinlog.result --- a/mysql-test/r/mysqlbinlog.result 2007-11-08 16:05:24 +01:00 +++ b/mysql-test/r/mysqlbinlog.result 2007-11-08 16:05:24 +01:00 @@ -325,4 +325,5 @@ drop table t1; 1 drop table t1; +shell> mysqlbinlog std_data/corrupt-relay-bin.000624 > var/tmp/bug31793.sql End of 5.0 tests 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-08 16:05:24 +01:00 +++ b/mysql-test/t/mysqlbinlog.test 2007-11-08 16:05:24 +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-08 16:05:24 +01:00 +++ b/sql/log_event.cc 2007-11-08 16:05:24 +01:00 @@ -1400,17 +1400,43 @@ /* 2 utility functions for the next method */ -/* - Get the pointer for a string (src) that contains the length in - the first byte. Set the output string (dst) to the string value - and place the length of the string in the byte after the string. +/** + Read a string with length from memory. + + Get the pointer for a string (src) that contains the length in + the first byte. Set the output string (dst) to the string value + and place the length of the string in the byte after the string. + + @param src Pointer to variable holding a pointer to the memory to + read the string from. + @param dst Pointer to variable holding a pointer where the string + actual string starts. Starting from this position, the + string can be copied using @c memcpy(). + @param len Pointer to variable where the length will be stored. + @param end One-past-the-end of the memory where the string is + stored. + + @return Zero if the entire string can be copied successfully, @c + UINT_MAX if the length could not be read from memory + (that is, if *src >= end), the number of bytes that + are missing if we hit the end of the memory. */ -static void get_str_len_and_pointer(const Log_event::Byte **src, - const char **dst, - uint *len) +static int +get_str_len_and_pointer(const Log_event::Byte **src, + const char **dst, + uint *len, + const Log_event::Byte *end) { - if ((*len= **src)) - *dst= (char *)*src + 1; // Will be copied later + if (*src >= end) + return -1; // Will be UINT_MAX in two-complement arithmetics + uint length= **src; + if (length > 0) + { + if (*src + length >= end) + return *src + length - end; // Number of bytes missing + *dst= (char *)*src + 1; // Will be copied later + } + *len= length; (*src)+= *len + 1; } @@ -1424,6 +1450,23 @@ *(*dst)++= 0; } + +/** + Macro to check that there is enough space to read from memory. + + @param PTR Pointer to memory + @param END End of memory + @param CNT Number of bytes that should be read. + */ +#define CHECK_SPACE(PTR,END,CNT) \ + do { \ + DBUG_ASSERT((PTR) + (CNT) <= (END)); \ + if ((PTR) + (CNT) > (END)) { \ + query= 0; \ + DBUG_VOID_RETURN; \ + } \ + } while (0) + /* Query_log_event::Query_log_event() This is used by the SQL slave thread to prepare the event before execution. @@ -1475,6 +1518,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)); @@ -1494,6 +1548,7 @@ { switch (*pos++) { case Q_FLAGS2_CODE: + CHECK_SPACE(pos, end, 4); flags2_inited= 1; flags2= uint4korr(pos); DBUG_PRINT("info",("In Query_log_event, read flags2: %lu", (ulong) flags2)); @@ -1504,6 +1559,7 @@ #ifndef DBUG_OFF char buff[22]; #endif + CHECK_SPACE(pos, end, 8); sql_mode_inited= 1; sql_mode= (ulong) uint8korr(pos); // QQ: Fix when sql_mode is ulonglong DBUG_PRINT("info",("In Query_log_event, read sql_mode: %s", @@ -1512,15 +1568,21 @@ break; } case Q_CATALOG_NZ_CODE: - get_str_len_and_pointer(&pos, &catalog, &catalog_len); + if (get_str_len_and_pointer(&pos, &catalog, &catalog_len, end)) + { + query= 0; + DBUG_VOID_RETURN; + } break; case Q_AUTO_INCREMENT: + CHECK_SPACE(pos, end, 4); auto_increment_increment= uint2korr(pos); auto_increment_offset= uint2korr(pos+2); pos+= 4; break; case Q_CHARSET_CODE: { + CHECK_SPACE(pos, end, 6); charset_inited= 1; memcpy(charset, pos, 6); pos+= 6; @@ -1528,20 +1590,28 @@ } case Q_TIME_ZONE_CODE: { - get_str_len_and_pointer(&pos, &time_zone_str, &time_zone_len); + if (get_str_len_and_pointer(&pos, &time_zone_str, &time_zone_len, end)) + { + query= 0; + DBUG_VOID_RETURN; + } break; } case Q_CATALOG_CODE: /* for 5.0.x where 0<=x<=3 masters */ + CHECK_SPACE(pos, end, 1); if ((catalog_len= *pos)) catalog= (char*) pos+1; // Will be copied later + CHECK_SPACE(pos, end, catalog_len + 2); pos+= catalog_len+2; // leap over end 0 catalog_nz= 0; // catalog has end 0 in event break; case Q_LC_TIME_NAMES_CODE: + CHECK_SPACE(pos, end, 2); lc_time_names_number= uint2korr(pos); pos+= 2; break; case Q_CHARSET_DATABASE_CODE: + CHECK_SPACE(pos, end, 2); charset_database_number= uint2korr(pos); pos+= 2; break;