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:
  Result change.
mysql-test/t/mysqlbinlog.test:
  Adding test that it fails gracefully for a corrupt relay log.
sql/log_event.cc:
  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.
mysql-test/std_data/corrupt-relay-bin.000624:
  BitKeeper file /home/mats/devel/b31793-mysql-5.0-rpl/mysql-test/std_data/corrupt-relay-bin.000624

4 files changed, 90 insertions, 12 deletions

diff --git a/mysql-test/r/mysqlbinlog.result b/mysql-test/r/mysqlbinlog.result
index d16a4c39a11..9f001c293de 100644
--- a/mysql-test/r/mysqlbinlog.result
+++ b/mysql-test/r/mysqlbinlog.result
@@ -325,4 +325,5 @@ flush logs;
 drop table t1;
 1
 drop table t1;
+shell> mysqlbinlog std_data/corrupt-relay-bin.000624 > var/tmp/bug31793.sql
 End of 5.0 tests
diff --git a/mysql-test/std_data/corrupt-relay-bin.000624 b/mysql-test/std_data/corrupt-relay-bin.000624
new file mode 100644
index 00000000000..21b4901211c
--- /dev/null
+++ b/mysql-test/std_data/corrupt-relay-bin.000624
diff --git a/mysql-test/t/mysqlbinlog.test b/mysql-test/t/mysqlbinlog.test
index 451eef17108..c83fe94f2eb 100644
--- a/mysql-test/t/mysqlbinlog.test
+++ b/mysql-test/t/mysqlbinlog.test
@@ -237,4 +237,8 @@ let $c= `select $a=$b`;
 --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 --git a/sql/log_event.cc b/sql/log_event.cc
index 3899e772bf8..0e257bf7f6c 100644
--- a/sql/log_event.cc
+++ b/sql/log_event.cc
@@ -1400,17 +1400,46 @@ Query_log_event::Query_log_event(THD* thd_arg, const char* query_arg,
 
 /* 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.
+
+   This function reads the string-with-length stored at
+   <code>src</code> and extract the length into <code>*len</code> and
+   a pointer to the start of the string into <code>*dst</code>. The
+   string can then be copied using <code>memcpy()</code> with the
+   number of bytes given in <code>*len</code>.
+
+   @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 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 <code>*src >= end</code>), otherwise the
+              number of bytes that are missing to read the full
+              string, which happends <code>*dst + *len >= end</code>.
 */
-static void get_str_len_and_pointer(const Log_event::Byte **src, 
-                                    const char **dst, 
-                                    uint *len)
-{
-  if ((*len= **src))
-    *dst= (char *)*src + 1;                          // Will be copied later
+static int
+get_str_len_and_pointer(const Log_event::Byte **src,
+                        const char **dst,
+                        uint *len,
+                        const Log_event::Byte *end)
+{
+  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 +1453,23 @@ static void copy_str_and_move(const char **src,
   *(*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 +1521,17 @@ Query_log_event::Query_log_event(const char* buf, uint event_len,
   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 >= min(data_len + 1, MAX_SIZE_LOG_EVENT_STATUS))
+    {
+      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 +1551,7 @@ Query_log_event::Query_log_event(const char* buf, uint event_len,
   {
     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 +1562,7 @@ Query_log_event::Query_log_event(const char* buf, uint event_len,
 #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 +1571,21 @@ Query_log_event::Query_log_event(const char* buf, uint event_len,
       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 +1593,28 @@ Query_log_event::Query_log_event(const char* buf, uint event_len,
     }
     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;