Bug#34930219 Missing synchronization of access to THD::m_protocol causing SIGSEGV

A stack allocated protocol instance was popped while i_s.processlist was
about to check whether the client connection was still alive. When the
protocol instance went out of scope, the call to connection_alive() accessed
an invalid pointer, causing SIGSEGV.

The fix is to cache the return value from the current protocol's
connection_alive() method when pushing, popping or getting the protocol.
This might leave results that are slightly out of sync with reality, but
a better synchronization is likely to cause performance degradation.

Change-Id: I3d512fadaa0df145af3f25d4cc03fa20143c5310
(cherry picked from commit 99c1919e0596bb6fe0292e602299e6da880fa352)

Author: ssorumgard

mysql-test/suite/x/r/information_schema_processlist.result

@@ -0,0 +1,34 @@
+# I. Reproduce race condition between popping the protocol and selecting
+#    from I_S processlist (Bug#34930219):
+# 1. Create X Protocol session (X1) using "x_root" account and hang it
+#    before popping the protocol (sync point: wait_before_popping_protocol)
+# 2. Create classic protocol session(C1) and list all sessions,
+#    hang it at sync point 'wait_before_returning_protocol'.
+# 3. Wait until C1 is at the expected sync point, then wakeup X1.
+
+
+#
+# Connect X1, send signal.
+
+#
+# Connect Cl, wait for signal.
+SET DEBUG_SYNC= 'now WAIT_FOR waiting_popping';
+
+#
+# Use C1 and list all sessions, wait before returning the protocol.
+SET DEBUG_SYNC= 'wait_before_checking_alive SIGNAL waiting_checking WAIT_FOR continue_checking';
+SELECT COUNT(*) > 0 from information_schema.processlist;
+
+#
+# From the default connection, wait for threads being halted, then
+# let both clients continue.
+SET DEBUG_SYNC= 'now WAIT_FOR waiting_checking';
+SET DEBUG_SYNC= 'now SIGNAL continue_popping';
+SET DEBUG_SYNC= 'now SIGNAL continue_checking';
+
+#
+# Go back to C1 and reap.
+COUNT(*) > 0
+1
+Warnings:
+Warning	1287	'INFORMATION_SCHEMA.PROCESSLIST' is deprecated and will be removed in a future release. Please use performance_schema.processlist instead

mysql-test/suite/x/t/information_schema_processlist.test

@@ -0,0 +1,67 @@
+# This test is created to reproduce a race condition where the session is popping
+# the protocol while I_S processlist is accessing the protocol pointer.
+
+--source include/have_debug_sync.inc
+--source include/have_debug.inc
+
+--source include/xplugin_preamble.inc
+--source include/xplugin_create_user.inc
+
+--echo # I. Reproduce race condition between popping the protocol and selecting
+--echo #    from I_S processlist (Bug#34930219):
+--echo # 1. Create X Protocol session (X1) using "x_root" account and hang it
+--echo #    before popping the protocol (sync point: wait_before_popping_protocol)
+--echo # 2. Create classic protocol session(C1) and list all sessions,
+--echo #    hang it at sync point 'wait_before_returning_protocol'.
+--echo # 3. Wait until C1 is at the expected sync point, then wakeup X1.
+--echo
+
+## Test setup.
+--let $xtest_file= $MYSQL_TMP_DIR/is_processlist_mysqlx.xpl
+--write_file $xtest_file
+
+SET DEBUG_SYNC= 'wait_before_popping_protocol SIGNAL waiting_popping WAIT_FOR continue_popping';
+SELECT 1;
+EOF
+
+## Start the X client.
+--echo
+--echo #
+--echo # Connect X1, send signal.
+--exec_in_background $MYSQLXTEST -ux_root --password='' --file=$xtest_file 2>&1
+
+## Connect C1
+--echo
+--echo #
+--echo # Connect Cl, wait for signal.
+--connect(con1,localhost,root,,)
+SET DEBUG_SYNC= 'now WAIT_FOR waiting_popping';
+
+## Let C1 wait while returning the protocol.
+--echo
+--echo #
+--echo # Use C1 and list all sessions, wait before returning the protocol.
+SET DEBUG_SYNC= 'wait_before_checking_alive SIGNAL waiting_checking WAIT_FOR continue_checking';
+send SELECT COUNT(*) > 0 from information_schema.processlist;
+
+## Both C1 and X1 are now halted. First let X1 continue, then C1.
+--echo
+--echo #
+--echo # From the default connection, wait for threads being halted, then
+--echo # let both clients continue.
+--connection default
+SET DEBUG_SYNC= 'now WAIT_FOR waiting_checking';
+SET DEBUG_SYNC= 'now SIGNAL continue_popping';
+--sleep 1
+SET DEBUG_SYNC= 'now SIGNAL continue_checking';
+
+--echo
+--echo #
+--echo # Go back to C1 and reap.
+--connection con1
+reap;
+
+## Cleanup
+--connection default
+--disconnect con1
+--source ../include/xplugin_cleanup.inc

plugin/test_service_sql_api/test_sql_sleep_is_connected.cc

@@ -64,6 +64,8 @@ static void ensure_api_ok(const char *function, MYSQL_SESSION result) {
 struct Callback_data {
   bool limit_is_connected{false};
   int is_connected_calls{0};
+  int is_disconnected_calls{2};  // Calls after disconnect.
+  int is_preamble_calls{4};      // Calls before starting query.
   int handle_ok_calls{0};
 };
 
@@ -210,6 +212,14 @@ static bool sql_connection_alive(void *ctx) {
   Callback_data *cbd = static_cast<Callback_data *>(ctx);
 
   if (cbd->limit_is_connected) {
+    // Patch for bug#34930219 changes the way connection_alive() is called.
+    // Hence, we must adjust the expected number of calls.
+    if (cbd->is_preamble_calls-- > 0) return true;
+    if (cbd->is_connected_calls == 0 && cbd->is_disconnected_calls > 0) {
+      cbd->is_disconnected_calls--;
+      return false;
+    }
+
     // Connection is disconnected
     // after concrete number of calls
     cbd->is_connected_calls--;

sql/mdl.h

@@ -126,7 +126,7 @@ class MDL_context_owner {
   /**
     Does the owner still have connection to the client?
   */
-  virtual bool is_connected() = 0;
+  virtual bool is_connected(bool = false) = 0;
 
   /**
     Indicates that owner thread might have some commit order (non-MDL) waits

sql/mdl_context_backup.cc

@@ -82,7 +82,7 @@ class MDL_context_backup_manager::MDL_context_backup
 
   uint get_rand_seed() const override { return 1; }
 
-  bool is_connected() override { return true; }
+  bool is_connected(bool = false) override { return true; }
   // End implementation of MDL_context_owner interface.
 
  private:

sql/protocol_callback.cc

@@ -30,6 +30,7 @@
 #include "m_ctype.h"
 #include "mysql_com.h"
 #include "sql/current_thd.h"
+#include "sql/debug_sync.h"
 #include "sql/field.h"
 #include "sql/item.h"
 #include "sql/item_func.h"
@@ -277,8 +278,10 @@ int Protocol_callback::shutdown(bool server_shutdown) {
     false  disconnected
 */
 bool Protocol_callback::connection_alive() const {
-  if (callbacks.connection_alive)
+  if (callbacks.connection_alive) {
+    DEBUG_SYNC(current_thd, "wait_before_checking_alive");
     return callbacks.connection_alive(callbacks_ctx);
+  }
 
   return true;
 }

sql/sql_class.cc

@@ -814,6 +814,7 @@ THD::THD(bool enable_plugins)
   protocol_text->init(this);
   protocol_binary->init(this);
   protocol_text->set_client_capabilities(0);  // minimalistic client
+  m_cached_is_connection_alive.store(m_protocol->connection_alive());
 
   /*
     Make sure thr_lock_info_init() is called for threads which do not get
@@ -3146,32 +3147,54 @@ bool THD::is_classic_protocol() const {
          get_protocol()->type() == Protocol::PROTOCOL_TEXT;
 }
 
-bool THD::is_connected() {
+bool THD::is_connected(bool use_cached_connection_alive) {
   /*
     All system threads (e.g., the slave IO thread) are connected but
     not using vio. So this function always returns true for all
     system threads.
   */
   if (system_thread) return true;
 
+  /*
+    In some situations, e.g. when generating information_schema.processlist,
+    we can live with a cached value to avoid introducing mutex usage.
+  */
+  if (use_cached_connection_alive) {
+    DEBUG_SYNC(current_thd, "wait_before_checking_alive");
+    return m_cached_is_connection_alive.load();
+  }
+
   if (is_classic_protocol())
     return get_protocol()->connection_alive() &&
            vio_is_connected(get_protocol_classic()->get_vio());
 
   return get_protocol()->connection_alive();
 }
 
+Protocol *THD::get_protocol() {
+  m_cached_is_connection_alive.store(m_protocol->connection_alive());
+  return m_protocol;
+}
+
+Protocol_classic *THD::get_protocol_classic() {
+  assert(is_classic_protocol());
+  m_cached_is_connection_alive.store(m_protocol->connection_alive());
+  return pointer_cast<Protocol_classic *>(m_protocol);
+}
+
 void THD::push_protocol(Protocol *protocol) {
   assert(m_protocol != nullptr);
   assert(protocol != nullptr);
   m_protocol->push_protocol(protocol);
   m_protocol = protocol;
+  m_cached_is_connection_alive.store(m_protocol->connection_alive());
 }
 
 void THD::pop_protocol() {
   assert(m_protocol != nullptr);
   m_protocol = m_protocol->pop_protocol();
   assert(m_protocol != nullptr);
+  m_cached_is_connection_alive.store(m_protocol->connection_alive());
 }
 
 void THD::set_time() {

sql/sql_class.h

@@ -1245,6 +1245,14 @@ class THD : public MDL_context_owner,
  private:
   mysql_mutex_t LOCK_query_plan;
 
+  /**
+    Keep a cached value saying whether the connection is alive. Update when
+    pushing, popping or getting the protocol. Used by
+    information_schema.processlist to avoid locking mutexes that might
+    affect performance.
+  */
+  std::atomic<bool> m_cached_is_connection_alive;
+
  public:
   /// Locks the query plan of this THD
   void lock_query_plan() { mysql_mutex_lock(&LOCK_query_plan); }
@@ -1296,7 +1304,7 @@ class THD : public MDL_context_owner,
 
   const Protocol *get_protocol() const { return m_protocol; }
 
-  Protocol *get_protocol() { return m_protocol; }
+  Protocol *get_protocol();
 
   SSL_handle get_ssl() const {
 #ifndef NDEBUG
@@ -1322,10 +1330,7 @@ class THD : public MDL_context_owner,
     return pointer_cast<const Protocol_classic *>(m_protocol);
   }
 
-  Protocol_classic *get_protocol_classic() {
-    assert(is_classic_protocol());
-    return pointer_cast<Protocol_classic *>(m_protocol);
-  }
+  Protocol_classic *get_protocol_classic();
 
  private:
   Protocol *m_protocol;  // Current protocol
@@ -3179,7 +3184,7 @@ class THD : public MDL_context_owner,
   bool is_classic_protocol() const;
 
   /** Return false if connection to client is broken. */
-  bool is_connected() final;
+  bool is_connected(bool use_cached_connection_alive = false) final;
 
   /**
     Mark the current error as fatal. Warning: this does not

sql/sql_show.cc

@@ -2841,16 +2841,16 @@ class List_process_list : public Do_THD_Impl {
       LEX_CSTRING inspect_sctx_host = inspect_sctx->host();
       LEX_CSTRING inspect_sctx_host_or_ip = inspect_sctx->host_or_ip();
 
-      {
-        MUTEX_LOCK(grd, &inspect_thd->LOCK_thd_protocol);
-
-        if ((!(inspect_thd->get_protocol() &&
-               inspect_thd->get_protocol()->connection_alive()) &&
-             !inspect_thd->system_thread) ||
-            (m_user && (inspect_thd->system_thread || !inspect_sctx_user.str ||
-                        strcmp(inspect_sctx_user.str, m_user)))) {
-          return;
-        }
+      /*
+        Since we only access a cached value of connection_alive, which is
+        also an atomic, we do not need to lock LOCK_thd_protocol here. We
+        may get a value that is slightly outdated, but we will not get a crash
+        due to reading invalid memory at least.
+      */
+      if (!inspect_thd->is_connected(true) ||
+          (m_user && (inspect_thd->system_thread || !inspect_sctx_user.str ||
+                      strcmp(inspect_sctx_user.str, m_user)))) {
+        return;
       }
 
       thd_info = new (m_client_thd->mem_root) thread_info;
@@ -3089,13 +3089,16 @@ class Fill_process_list : public Do_THD_Impl {
               ? NullS
               : client_priv_user;
 
-      {
-        MUTEX_LOCK(grd, &inspect_thd->LOCK_thd_protocol);
-        if ((!inspect_thd->get_protocol()->connection_alive() &&
-             !inspect_thd->system_thread) ||
-            (user && (inspect_thd->system_thread || !inspect_sctx_user.str ||
-                      strcmp(inspect_sctx_user.str, user))))
-          return;
+      /*
+        Since we only access a cached value of connection_alive, which is
+        also an atomic, we do not need to lock LOCK_thd_protocol here. We
+        may get a value that is slightly outdated, but we will not get a crash
+        due to reading invalid memory at least.
+      */
+      if (!inspect_thd->is_connected(true) ||
+          (user && (inspect_thd->system_thread || !inspect_sctx_user.str ||
+                    strcmp(inspect_sctx_user.str, user)))) {
+        return;
       }
 
       DBUG_EXECUTE_IF(

sql/srv_session.cc

@@ -1181,6 +1181,7 @@ int Srv_session::execute_command(enum enum_server_command command,
   }
   int ret = dispatch_command(m_thd, data, command);
 
+  DEBUG_SYNC(m_thd, "wait_before_popping_protocol");
   m_thd->pop_protocol();
   assert(m_thd->get_protocol() == &m_protocol_error);
   return ret;

unittest/gunit/test_mdl_context_owner.h

@@ -36,7 +36,7 @@ class Test_MDL_context_owner : public MDL_context_owner {
                  int) override {}
 
   int is_killed() const final { return 0; }
-  bool is_connected() override { return true; }
+  bool is_connected(bool = false) override { return true; }
   bool might_have_commit_order_waiters() const override { return false; }
 
   THD *get_thd() override {