Bug#29417234:CONTRIBUTION BY FACEBOOK: FIX WRITING FORMAT DESCRIPTION EVENT

Problem:

In the function: Format_description_log_event::write(..)

memcpy((char *)buff + ST_COMMON_HEADER_LEN_OFFSET + 1, &post_header_len[0],
        Binary_log_event::LOG_EVENT_TYPES);

This works fine, if the server generating the FDE is the same as the server
writing it out (hence post_header_len.size == Binary_log_event::LOG_EVENT_TYPES).

However, when we rotate relay logs, we use this function to write out the FDE
by using the event obtained from the master directly. If the master is on an
older version of MySQL with not as many event types, then this memcpy will
read beyond its buffer causing an ASAN failure.

Analysis:

Different versions of mysqld have different number of binlog event types. The
Format_description_event log has an post_header_len whose size is based on the number
of events supported by the mysqld version. So a 5.6 mysqld generates a FDE
with 35 types while 5.7 mysqld generates a FDE with 38 types. When 5.7 mysqld
attempts to read a 5.6 generated FDE and then rewrite out the 5.6 FDE, it
uses its own value of LOG_EVENT_TYPES (38) to determine the length of the
array, which is actually only 35 types.

The copy accesses undefined memory to generate the FDE.

Fix:
if the post_header_len is greater than Binary_log_event::LOG_EVENT_TYPES, use
the sizeof post_header_len vector while writing post_header_len vector,
otherwise use Binary_log_event::LOG_EVENT_TYPES.

P.S: This patch also corrects another issue which happens while decoding the
post_header_len array in FDE.

While decoding post_header_len we use the variable number_of_event_types to
determine how many bytes to read from the buffer, now at this point the
number_of_event_types holds the value which is one more than the actual value.
This problem happens because we don't consider the "BINLOG_CHECKSUM_ALG_DESC_LEN"
before decoding post_header_len. In 8.0 this issue has already been
corrected(WL#11567) so this part of change is just for 5.7.

RB: 21849

Author: Neha Kumari

libbinlogevents/src/control_events.cpp

@@ -335,26 +335,29 @@ Format_description_event(const char* buf, unsigned int event_len,
   number_of_event_types=
    event_len - (LOG_EVENT_MINIMAL_HEADER_LEN + ST_COMMON_HEADER_LEN_OFFSET + 1);
 
-  const uint8_t *ubuf = reinterpret_cast<const uint8_t*>(buf);
+  ver_calc = get_product_version();
+  if (ver_calc >= checksum_version_product) {
+    /* the last bytes are the checksum alg desc and value (or value's room) */
+    number_of_event_types -= BINLOG_CHECKSUM_ALG_DESC_LEN;
+  }
+
+  const uint8_t *ubuf = reinterpret_cast<const uint8_t*>(buf) +
+                        ST_COMMON_HEADER_LEN_OFFSET + 1;
   post_header_len.insert(post_header_len.begin(),
-                         ubuf + ST_COMMON_HEADER_LEN_OFFSET + 1,
-                         (ubuf + ST_COMMON_HEADER_LEN_OFFSET + 1 +
-                          number_of_event_types));
+                         ubuf, (ubuf + number_of_event_types));
 
+  ubuf += number_of_event_types;
   calc_server_version_split();
-  if ((ver_calc= get_product_version()) >= checksum_version_product)
+  if (ver_calc >= checksum_version_product)
   {
-    /* the last bytes are the checksum alg desc and value (or value's room) */
-    number_of_event_types -= BINLOG_CHECKSUM_ALG_DESC_LEN;
     /*
       FD from the checksum-home version server (ver_calc ==
       checksum_version_product) must have
       number_of_event_types == LOG_EVENT_TYPES.
     */
     BAPI_ASSERT(ver_calc != checksum_version_product ||
                 number_of_event_types == LOG_EVENT_TYPES);
-    footer()->checksum_alg= (enum_binlog_checksum_alg)
-                                  post_header_len[number_of_event_types];
+    footer()->checksum_alg= static_cast<enum_binlog_checksum_alg> (*ubuf);
   }
   else
   {

mysql-test/std_data/slave-relay-bin-win.index

@@ -0,0 +1 @@
+.\master-bin.000001

mysql-test/std_data/slave-relay-bin.000001

@@ -0,0 +1 @@
+./master-bin.000001

mysql-test/std_data/slave-relay-bin.index

@@ -0,0 +1,15 @@
+include/master-slave.inc
+Warnings:
+Note	####	Sending passwords in plain text without SSL/TLS is extremely insecure.
+Note	####	Storing MySQL user name or password information in the master info repository is not secure and is therefore not recommended. Please consider using the USER and PASSWORD connection options for START SLAVE; see the 'START SLAVE Syntax' in the MySQL Manual for more information.
+[connection master]
+RESET MASTER;
+[connection slave]
+CALL mtr.add_suppression("The slave coordinator and worker threads are stopped, possibly leaving data in inconsistent state");
+CALL mtr.add_suppression("Got fatal error 1236 from master when reading data from binary log:");
+START SLAVE;
+include/wait_for_slave_param.inc [Slave_IO_State]
+FLUSH LOGS;
+STOP SLAVE;
+RESET SLAVE;
+include/rpl_end.inc

mysql-test/suite/rpl/r/rpl_fde_write.result

@@ -0,0 +1,57 @@
+# ==== Purpose ====
+#
+# Tests the statement based replication of tables in a cross version setup
+# where the master is a 8.0 server and slave is a 5.7 server, and verify
+# that there is no crash at the time of FLUSH LOGS on slave side.
+#
+# ==== Implementation ====
+#
+# Start a master server on 5.7
+# Copy the binary log file and index of a 8.0 master server to the datadir of 5.7 master server
+# Start the slave on a 5.7 server
+# Execute Flush logs command on the slave
+# Without the fix the Flush Logs command will trigger an ASAN failure
+#
+# ==== References ====
+#
+# Bug#29417234:CONTRIBUTION BY FACEBOOK: FIX WRITING FORMAT DESCRIPTION EVENT
+#
+
+--source include/have_binlog_format_statement.inc
+--source include/not_gtid_enabled.inc
+--let $rpl_skip_start_slave= 1
+--source include/master-slave.inc
+
+--let $MYSQLD_MASTER_DATADIR= `select @@datadir`
+
+# clear master datadir
+RESET MASTER;
+--remove_file $MYSQLD_MASTER_DATADIR/master-bin.000001
+--remove_file $MYSQLD_MASTER_DATADIR/master-bin.index
+
+# on Win* platforms path separator is backslash
+if (`SELECT CONVERT(@@VERSION_COMPILE_OS USING latin1) IN ('Win32', 'Win64', 'Windows')`)
+{
+    --copy_file std_data/slave-relay-bin-win.index $MYSQLD_MASTER_DATADIR/master-bin.index
+}
+if (`SELECT CONVERT(@@VERSION_COMPILE_OS USING latin1) NOT IN ('Win32', 'Win64', 'Windows')`)
+{
+    --copy_file std_data/slave-relay-bin.index $MYSQLD_MASTER_DATADIR/master-bin.index
+}
+
+--copy_file std_data/slave-relay-bin.000001 $MYSQLD_MASTER_DATADIR/master-bin.000001
+
+--source include/rpl_connection_slave.inc
+CALL mtr.add_suppression("The slave coordinator and worker threads are stopped, possibly leaving data in inconsistent state");
+CALL mtr.add_suppression("Got fatal error 1236 from master when reading data from binary log:");
+START SLAVE;
+--let $slave_param= Slave_IO_State
+--let $slave_param_value = Waiting for master to send event
+--source include/wait_for_slave_param.inc
+
+FLUSH LOGS;
+STOP SLAVE;
+RESET SLAVE;
+
+--let $rpl_only_running_threads= 1
+--source include/rpl_end.inc

mysql-test/suite/rpl/t/rpl_fde_write.test

@@ -5522,8 +5522,33 @@ bool Format_description_log_event::write(IO_CACHE* file)
     created= get_time();
   int4store(buff + ST_CREATED_OFFSET, static_cast<uint32>(created));
   buff[ST_COMMON_HEADER_LEN_OFFSET]= LOG_EVENT_HEADER_LEN;
-  memcpy((char*) buff+ST_COMMON_HEADER_LEN_OFFSET + 1,  &post_header_len.front(),
-         Binary_log_event::LOG_EVENT_TYPES);
+
+  size_t number_of_events;
+  if (post_header_len.size() == Binary_log_event::LOG_EVENT_TYPES)
+    // Replicating between master and slave with same version.
+    // number_of_events will be same as Binary_log_event::LOG_EVENT_TYPES
+    number_of_events = Binary_log_event::LOG_EVENT_TYPES;
+  else if (post_header_len.size() > Binary_log_event::LOG_EVENT_TYPES)
+    /*
+      Replicating between new master and old slave.
+      In that case there won't be any memory issues, as there won't be
+      any out of memory read.
+    */
+    number_of_events = Binary_log_event::LOG_EVENT_TYPES;
+  else
+    /*
+      Replicating between old master and new slave.
+      In that case it might lead to different number_of_events on master and
+      slave. When the relay log is rotated, the FDE from master is used to
+      create the FDE event on slave, which is being written here. In that case
+      we might end up reading more bytes as
+      post_header_len.size() < Binary_log_event::LOG_EVENT_TYPES;
+      casuing memory issues.
+    */
+    number_of_events = post_header_len.size();
+
+  memcpy((char*) buff + ST_COMMON_HEADER_LEN_OFFSET + 1,  &post_header_len.front(),
+          number_of_events);
   /*
     if checksum is requested
     record the checksum-algorithm descriptor next to