MDEV-29894: Calling a function from a different database in a slave side trigger crashes

bnestere · bnestere · commit c2d44ecb904d · 2023-06-21T12:51:01.000-06:00
When opening and locking tables, if triggers will be invoked in a separate database, thd->set_db() is invoked, thus freeeing the memory and headers which thd->db had previously pointed to. In row based replication, the event execution logic initializes thd->db to point to the database which the event targets, which is owned by the corresponding table share (introduced in d9898c9 for MDEV-7409). The problem then, is that during the table opening and locking process for a row event, memory which belongs to the table share would be freed, which is not valid. This patch replaces the thd->reset_db() calls to thd->set_db(), which copies-by-value, rather than by reference. Then when the memory is freed, our copy of memory is freed, rather than memory which belongs to a table share. Notes: 1. The call to change thd->db now happens on a higher-level, in Rows_log_event::do_apply_event() rather than ::do_exec_row(), in the call stack. This is because do_exec_row() is called within a loop, and each invocation would redundantly set and unset the db to the same value. 2. thd->set_db() is only used if triggers are to be invoked, as there is no vulnerability in the non-trigger case, and copying memory would be an unnecessary inefficiency. Reviewed By: ============ Andrei Elkin <andrei.elkin@mariadb.com>
diff --git a/mysql-test/suite/rpl/r/rpl_row_trigger_multi_db.result b/mysql-test/suite/rpl/r/rpl_row_trigger_multi_db.result
@@ -0,0 +1,60 @@
+include/master-slave.inc
+[connection master]
+connection slave;
+set global slave_run_triggers_for_rbr=1;
+connection master;
+CREATE TABLE t1 (a int);
+connection slave;
+connection slave;
+CREATE DATABASE db2;
+CREATE FUNCTION db2.get_value(a INT) RETURNS int(2)  RETURN 0;
+#
+# Test Insert_rows_log_event
+connection slave;
+CREATE TRIGGER tr_ins BEFORE INSERT ON t1 FOR EACH ROW BEGIN
+DECLARE a INT;
+SET a = db2.get_value(1);
+END//
+connection master;
+INSERT INTO t1 VALUES (1);
+connection slave;
+connection slave;
+DROP TRIGGER tr_ins;
+#
+# Test Update_rows_log_event
+connection master;
+INSERT INTO t1 VALUES (5);
+connection slave;
+connection slave;
+CREATE TRIGGER tr_upd BEFORE UPDATE ON t1 FOR EACH ROW BEGIN
+DECLARE a INT;
+SET a = db2.get_value(1);
+END//
+connection master;
+UPDATE t1 SET a=a+1 WHERE a=5;
+connection slave;
+connection slave;
+DROP TRIGGER tr_upd;
+#
+# Test Delete_rows_log_event
+connection master;
+INSERT INTO t1 VALUES (7);
+connection slave;
+connection slave;
+CREATE TRIGGER tr_del BEFORE DELETE ON t1 FOR EACH ROW BEGIN
+DECLARE a INT;
+SET a = db2.get_value(1);
+END//
+connection master;
+DELETE FROM t1 WHERE a=7;
+connection slave;
+connection slave;
+DROP TRIGGER tr_del;
+#
+# Cleanup
+connection slave;
+SET GLOBAL slave_run_triggers_for_rbr=NO;
+DROP DATABASE db2;
+connection master;
+DROP TABLE t1;
+include/rpl_end.inc
diff --git a/mysql-test/suite/rpl/t/rpl_row_trigger_multi_db.test b/mysql-test/suite/rpl/t/rpl_row_trigger_multi_db.test
@@ -0,0 +1,98 @@
+#
+#   This test ensures that a table share's database name is not freed when
+# using row based replication with triggers that open different databases
+#
+#
+# References:
+#   MDEV-29894: Calling a function from a different database in a slave side
+#               trigger crashes
+#
+--source include/master-slave.inc
+--source include/have_binlog_format_row.inc
+
+--connection slave
+--let $old_slave_run_triggers= `SELECT @@global.slave_run_triggers_for_rbr`
+set global slave_run_triggers_for_rbr=1;
+
+--connection master
+CREATE TABLE t1 (a int);
+--sync_slave_with_master
+
+--connection slave
+CREATE DATABASE db2;
+CREATE FUNCTION db2.get_value(a INT) RETURNS int(2)  RETURN 0;
+
+--echo #
+--echo # Test Insert_rows_log_event
+
+--connection slave
+DELIMITER //;
+CREATE TRIGGER tr_ins BEFORE INSERT ON t1 FOR EACH ROW BEGIN
+DECLARE a INT;
+SET a = db2.get_value(1);
+END//
+DELIMITER ;//
+
+--connection master
+INSERT INTO t1 VALUES (1);
+--sync_slave_with_master
+
+--connection slave
+DROP TRIGGER tr_ins;
+
+
+--echo #
+--echo # Test Update_rows_log_event
+--connection master
+--let $row_val=5
+--eval INSERT INTO t1 VALUES ($row_val)
+--sync_slave_with_master
+
+--connection slave
+DELIMITER //;
+CREATE TRIGGER tr_upd BEFORE UPDATE ON t1 FOR EACH ROW BEGIN
+DECLARE a INT;
+SET a = db2.get_value(1);
+END//
+DELIMITER ;//
+
+--connection master
+--eval UPDATE t1 SET a=a+1 WHERE a=$row_val
+--sync_slave_with_master
+
+--connection slave
+DROP TRIGGER tr_upd;
+
+
+--echo #
+--echo # Test Delete_rows_log_event
+--connection master
+--let $row_val=7
+--eval INSERT INTO t1 VALUES ($row_val)
+--sync_slave_with_master
+
+--connection slave
+DELIMITER //;
+CREATE TRIGGER tr_del BEFORE DELETE ON t1 FOR EACH ROW BEGIN
+DECLARE a INT;
+SET a = db2.get_value(1);
+END//
+DELIMITER ;//
+
+--connection master
+--eval DELETE FROM t1 WHERE a=$row_val
+--sync_slave_with_master
+
+--connection slave
+DROP TRIGGER tr_del;
+
+
+--echo #
+--echo # Cleanup
+--connection slave
+--eval SET GLOBAL slave_run_triggers_for_rbr=$old_slave_run_triggers
+DROP DATABASE db2;
+--connection master
+DROP TABLE t1;
+
+--source include/rpl_end.inc
diff --git a/sql/log_event.h b/sql/log_event.h
@@ -5188,6 +5188,51 @@ class Rows_log_event : public Log_event
   uint      m_key_nr;   /* Key number */
   bool master_had_triggers;     /* set after tables opening */
 
+  /*
+    RAII helper class to automatically handle the override/restore of thd->db
+    when applying row events, so it will be visible in SHOW PROCESSLIST.
+
+    If triggers will be invoked, their logic frees the current thread's db,
+    so we use set_db() to use a copy of the table share's database.
+
+    If not using triggers, the db is never freed, and we can reference the
+    same memory owned by the table share.
+  */
+  class Db_restore_ctx
+  {
+  private:
+    THD *thd;
+    LEX_CSTRING restore_db;
+    bool db_copied;
+
+    Db_restore_ctx(Rows_log_event *rev)
+        : thd(rev->thd), restore_db(rev->thd->db)
+    {
+      TABLE *table= rev->m_table;
+
+      if (table->triggers && rev->do_invoke_trigger())
+      {
+        thd->reset_db(&null_clex_str);
+        thd->set_db(&table->s->db);
+        db_copied= true;
+      }
+      else
+      {
+        thd->reset_db(&table->s->db);
+        db_copied= false;
+      }
+    }
+
+    ~Db_restore_ctx()
+    {
+      if (db_copied)
+        thd->set_db(&null_clex_str);
+      thd->reset_db(&restore_db);
+    }
+
+    friend class Rows_log_event;
+  };
+
   int find_key(); // Find a best key to use in find_row()
   int find_row(rpl_group_info *);
   int write_row(rpl_group_info *, const bool);
diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc
@@ -5676,6 +5676,7 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi)
                        " (master had triggers)" : ""));
   if (table)
   {
+    Rows_log_event::Db_restore_ctx restore_ctx(this);
     master_had_triggers= table->master_had_triggers;
     bool transactional_table= table->file->has_transactions_and_rollback();
     table->file->prepare_for_insert(get_general_type_code() != WRITE_ROWS_EVENT);
@@ -7601,15 +7602,13 @@ Write_rows_log_event::do_exec_row(rpl_group_info *rgi)
 {
   DBUG_ASSERT(m_table != NULL);
   const char *tmp= thd->get_proc_info();
-  LEX_CSTRING tmp_db= thd->db;
   char *message, msg[128];
   const LEX_CSTRING &table_name= m_table->s->table_name;
   const char quote_char=
     get_quote_char_for_identifier(thd, table_name.str, table_name.length);
   my_snprintf(msg, sizeof msg,
               "Write_rows_log_event::write_row() on table %c%.*s%c",
               quote_char, int(table_name.length), table_name.str, quote_char);
-  thd->reset_db(&m_table->s->db);
   message= msg;
   int error;
 
@@ -7631,7 +7630,6 @@ Write_rows_log_event::do_exec_row(rpl_group_info *rgi)
     my_error(ER_UNKNOWN_ERROR, MYF(0));
   }
 
-  thd->reset_db(&tmp_db);
   return error;
 }
 
@@ -8237,15 +8235,13 @@ int Delete_rows_log_event::do_exec_row(rpl_group_info *rgi)
 {
   int error;
   const char *tmp= thd->get_proc_info();
-  LEX_CSTRING tmp_db= thd->db;
   char *message, msg[128];
   const LEX_CSTRING &table_name= m_table->s->table_name;
   const char quote_char=
     get_quote_char_for_identifier(thd, table_name.str, table_name.length);
   my_snprintf(msg, sizeof msg,
               "Delete_rows_log_event::find_row() on table %c%.*s%c",
               quote_char, int(table_name.length), table_name.str, quote_char);
-  thd->reset_db(&m_table->s->db);
   message= msg;
   const bool invoke_triggers= (m_table->triggers && do_invoke_trigger());
   DBUG_ASSERT(m_table != NULL);
@@ -8305,7 +8301,6 @@ int Delete_rows_log_event::do_exec_row(rpl_group_info *rgi)
       error= HA_ERR_GENERIC; // in case if error is not set yet
     m_table->file->ha_index_or_rnd_end();
   }
-  thd->reset_db(&tmp_db);
   thd_proc_info(thd, tmp);
   return error;
 }
@@ -8406,15 +8401,13 @@ Update_rows_log_event::do_exec_row(rpl_group_info *rgi)
   const bool invoke_triggers= (m_table->triggers && do_invoke_trigger());
   const char *tmp= thd->get_proc_info();
   DBUG_ASSERT(m_table != NULL);
-  LEX_CSTRING tmp_db= thd->db;
   char *message, msg[128];
   const LEX_CSTRING &table_name= m_table->s->table_name;
   const char quote_char=
     get_quote_char_for_identifier(thd, table_name.str, table_name.length);
   my_snprintf(msg, sizeof msg,
               "Update_rows_log_event::find_row() on table %c%.*s%c",
               quote_char, int(table_name.length), table_name.str, quote_char);
-  thd->reset_db(&m_table->s->db);
   message= msg;
 
 #ifdef WSREP_PROC_INFO
@@ -8443,7 +8436,6 @@ Update_rows_log_event::do_exec_row(rpl_group_info *rgi)
     if ((m_curr_row= m_curr_row_end))
       unpack_current_row(rgi, &m_cols_ai);
     thd_proc_info(thd, tmp);
-    thd->reset_db(&tmp_db);
     return error;
   }
 
@@ -8532,7 +8524,6 @@ Update_rows_log_event::do_exec_row(rpl_group_info *rgi)
 
 err:
   thd_proc_info(thd, tmp);
-  thd->reset_db(&tmp_db);
   m_table->file->ha_index_or_rnd_end();
   return error;
 }
