MyRocks crashes on pure virtual function when querying rocksdb_trx list

Summary:
Rdb_transactions are added and removed from the global
list by the Rdb_transaction constructor/destructors.

However, information_schema.rocksdb_trx queries scan
this global list and calls the virtual function is_writebatch_trx()
to determine how to cast the object.

It is possible for the subclasses destructors to have completed, but
before Rdb_transaction's destructor can remove the object from the
global list that an information_schema.rocksdb_trx query is initiated.
The query walks the list of transactions and comes across a partially
deconstructed Rdb_transaction_impl object. When it calls
is_writebatch_trx() on this object, it triggers a pure function access
crash because the function has already been removed/cleared.

Fix this by adding the objects to the global list after
the object is fully constructed and remove it from the global list
when the object enters the destructor.

Reviewed By: yizhang82

Differential Revision: D29164519

fbshipit-source-id: aaf39f330c8

mysql-test/suite/rocksdb/r/trx_info_cleanup.result

@@ -0,0 +1,15 @@
+create table t1 (a int) engine=rocksdb;
+set @@global.debug = '+d,rocksdb_trx_list_crash';
+insert into t1 values (1);
+insert into t1 values (2);
+set debug_sync = 'now WAIT_FOR destructor_started';
+select count(*) from information_schema.rocksdb_trx;
+count(*)
+0
+set debug_sync = 'now SIGNAL trx_list_query';
+set @@global.debug = '-d,rocksdb_trx_list_crash';
+SELECT a from t1;
+a
+1
+2
+DROP TABLE t1;

mysql-test/suite/rocksdb/t/trx_info_cleanup.test

@@ -0,0 +1,32 @@
+--source include/have_rocksdb.inc
+--source include/have_debug_sync.inc
+
+--source include/count_sessions.inc
+
+create table t1 (a int) engine=rocksdb;
+
+set @@global.debug = '+d,rocksdb_trx_list_crash';
+
+connect (con1,localhost,root,,);
+insert into t1 values (1);
+insert into t1 values (2);
+# Disconnect will trigger transaction cleanup
+disconnect con1;
+
+connection default;
+
+# Wait for the connection to be waiting for cleanup
+set debug_sync = 'now WAIT_FOR destructor_started';
+
+# This will be empty, but the query will still walk the list
+select count(*) from information_schema.rocksdb_trx;
+
+# Allow the connection to finish cleanup
+set debug_sync = 'now SIGNAL trx_list_query';
+
+set @@global.debug = '-d,rocksdb_trx_list_crash';
+
+SELECT a from t1;
+DROP TABLE t1;
+
+--source include/wait_until_count_sessions.inc

storage/rocksdb/ha_rocksdb.cc

@@ -3627,18 +3627,44 @@ class Rdb_transaction {
 
   void set_tx_read_only(bool val) { m_tx_read_only = val; }
 
-  explicit Rdb_transaction(THD *const thd)
-      : m_thd(thd), m_tbl_io_perf(nullptr) {
+  /*
+    Add or remove from the global list of active transactions
+    needed by information_schema queries.
+  */
+  void add_to_global_trx_list() {
     RDB_MUTEX_LOCK_CHECK(s_tx_list_mutex);
     s_tx_list.insert(this);
     RDB_MUTEX_UNLOCK_CHECK(s_tx_list_mutex);
   }
 
-  virtual ~Rdb_transaction() {
+  void remove_from_global_trx_list(void) {
+    DBUG_EXECUTE_IF("rocksdb_trx_list_crash", {
+      THD *thd = new THD();
+      thd->thread_stack = reinterpret_cast<char *>(&(thd));
+      thd->store_globals();
+
+      const char act[] =
+          "now signal destructor_started wait_for trx_list_query";
+      DBUG_ASSERT(!debug_sync_set_action(thd, STRING_WITH_LEN(act)));
+
+      thd->restore_globals();
+      delete thd;
+    });
     RDB_MUTEX_LOCK_CHECK(s_tx_list_mutex);
     s_tx_list.erase(this);
     RDB_MUTEX_UNLOCK_CHECK(s_tx_list_mutex);
   }
+
+  explicit Rdb_transaction(THD *const thd)
+      : m_thd(thd), m_tbl_io_perf(nullptr) {}
+
+  virtual ~Rdb_transaction() {
+#ifndef DEBUG_OFF
+    RDB_MUTEX_LOCK_CHECK(s_tx_list_mutex);
+    DBUG_ASSERT(s_tx_list.find(this) == s_tx_list.end());
+    RDB_MUTEX_UNLOCK_CHECK(s_tx_list_mutex);
+#endif
+  }
 };
 
 #ifndef DBUG_OFF
@@ -4076,6 +4102,10 @@ class Rdb_transaction_impl : public Rdb_transaction {
   }
 
   virtual ~Rdb_transaction_impl() override {
+    // Remove from the global list before all other processing is started.
+    // Otherwise, information_schema.rocksdb_trx can crash on this object.
+    Rdb_transaction::remove_from_global_trx_list();
+
     rollback();
 
     // Theoretically the notifier could outlive the Rdb_transaction_impl
@@ -4299,6 +4329,10 @@ class Rdb_writebatch_impl : public Rdb_transaction {
   }
 
   virtual ~Rdb_writebatch_impl() override {
+    // Remove from the global list before all other processing is started.
+    // Otherwise, information_schema.rocksdb_trx can crash on this object.
+    Rdb_transaction::remove_from_global_trx_list();
+
     rollback();
     delete m_batch;
   }
@@ -4428,6 +4462,10 @@ static Rdb_transaction *get_or_create_tx(THD *const thd) {
     tx->set_params(THDVAR(thd, lock_wait_timeout), THDVAR(thd, max_row_locks));
     tx->start_tx();
     set_tx_on_thd(thd, tx);
+
+    // Add the transaction to the global list of transactions
+    // once it is fully constructed.
+    tx->add_to_global_trx_list();
   } else {
     tx->set_params(THDVAR(thd, lock_wait_timeout), THDVAR(thd, max_row_locks));
     if (!tx->is_tx_started()) {