MDEV-28352 Spider: heap-use-after-free in ha_spider::lock_tables(), heap freed by spider_commit()

The heap-use-after-free is caused by the following mechanism:

  * In the execution of FLUSH TABLE WITH READ LOCK, the function
    spider_free_trx_conn() is called and the connections held by
    SPIDER_TRX::trx_conn_hash are freed.

  * Then, an instance of ha_spider maintains the freed connections
    because they are also referenced from ha_spider::conns.
    The ha_spider instance is kept in a lock structure until the
    corresponding table is unlocked.

  * Spider accesses ha_spider::conns on the implicit UNLOCK TABLE
    issued by BEGIN.

In the first place, when the connections have been freed, it means
that there are really no remote table locked by Spider.
Thus, there is no need for Spider to access ha_spider::cons on the
implicit UNLOCK TABLE.

We can fix the bug by removing the above mentioned access to
ha_spider::conns. We also modified spider_free_trx_conn() so that it
frees the connections only when no table is locked to reduce the
chance of another heap-use-after-free on ha_spider::conns.

3 files changed, 78 insertions, 160 deletions

diff --git a/storage/spider/ha_spider.cc b/storage/spider/ha_spider.cc
index 753d29c23eb..9b3fffcf873 100644
--- a/storage/spider/ha_spider.cc
+++ b/storage/spider/ha_spider.cc
@@ -1180,10 +1180,8 @@ int ha_spider::external_lock(
   backup_error_status();
 
   DBUG_ENTER("ha_spider::external_lock");
-  DBUG_PRINT("info",("spider this=%p", this));
-  DBUG_PRINT("info",("spider lock_type=%x", lock_type));
-  DBUG_PRINT("info", ("spider sql_command=%d", thd_sql_command(thd)));
 
+  /* Beginning of wide_handler setup */
   if (wide_handler->stage == SPD_HND_STAGE_EXTERNAL_LOCK)
   {
     /* Only the stage executor deals with table locks. */
@@ -1194,7 +1192,7 @@ int ha_spider::external_lock(
   }
   else
   {
-    /* Update the stage executor when the stage changes */
+    /* Update the stage executor when the stage changes. */
     wide_handler->stage= SPD_HND_STAGE_EXTERNAL_LOCK;
     wide_handler->stage_executor= this;
   }
@@ -1203,35 +1201,30 @@ int ha_spider::external_lock(
   wide_handler->external_lock_type= lock_type;
   wide_handler->sql_command = thd_sql_command(thd);
 
-  /* We treat BEGIN as if UNLOCK TABLE. */
-  if (wide_handler->sql_command == SQLCOM_BEGIN)
-  {
-    wide_handler->sql_command = SQLCOM_UNLOCK_TABLES;
-  }
-  if (lock_type == F_UNLCK &&
-      wide_handler->sql_command != SQLCOM_UNLOCK_TABLES)
-  {
-    DBUG_RETURN(0);
-  }
-
-  trx = spider_get_trx(thd, TRUE, &error_num);
+  trx= spider_get_trx(thd, TRUE, &error_num);
   if (error_num)
   {
     DBUG_RETURN(error_num);
   }
-  wide_handler->trx = trx;
+  wide_handler->trx= trx;
+  /* End of wide_handler setup */
 
-  /* Question: Why the following if block is necessary? Why here? */
   if (store_error_num)
   {
     DBUG_RETURN(store_error_num);
   }
 
-  DBUG_ASSERT(wide_handler->sql_command != SQLCOM_RENAME_TABLE &&
-              wide_handler->sql_command != SQLCOM_DROP_DB);
+  /* We treat BEGIN as if UNLOCK TABLE. */
+  if (wide_handler->sql_command == SQLCOM_BEGIN)
+  {
+    wide_handler->sql_command = SQLCOM_UNLOCK_TABLES;
+  }
+  const uint sql_command= wide_handler->sql_command;
+
+  DBUG_ASSERT(sql_command != SQLCOM_RENAME_TABLE &&
+              sql_command != SQLCOM_DROP_DB);
 
-  if (wide_handler->sql_command == SQLCOM_DROP_TABLE ||
-      wide_handler->sql_command == SQLCOM_ALTER_TABLE)
+  if (sql_command == SQLCOM_DROP_TABLE || sql_command == SQLCOM_ALTER_TABLE)
   {
     if (trx->locked_connections)
     {
@@ -1242,44 +1235,41 @@ int ha_spider::external_lock(
     DBUG_RETURN(0);
   }
 
-  if (lock_type != F_UNLCK)
+  if (lock_type == F_UNLCK)
+  {
+    if (sql_command != SQLCOM_UNLOCK_TABLES)
+    {
+      DBUG_RETURN(0); /* Unlock remote tables only by UNLOCK TABLES. */
+    }
+    if (!trx->locked_connections)
+    {
+      DBUG_RETURN(0); /* No remote table actually locked by Spider */
+    }
+  }
+  else
   {
     if (unlikely((error_num= spider_internal_start_trx(this))))
     {
       DBUG_RETURN(error_num);
     }
-    if (wide_handler->sql_command != SQLCOM_SELECT &&
-        wide_handler->sql_command != SQLCOM_HA_READ)
+    if (sql_command != SQLCOM_SELECT && sql_command != SQLCOM_HA_READ)
     {
       trx->updated_in_this_trx= TRUE;
-      DBUG_PRINT("info", ("spider trx->updated_in_this_trx=TRUE"));
+    }
+    if (!wide_handler->lock_table_type)
+    {
+      DBUG_RETURN(0); /* No need to actually lock remote tables. */
     }
   }
 
-  if (wide_handler->lock_table_type > 0 ||
-    wide_handler->sql_command == SQLCOM_UNLOCK_TABLES)
+  if (!partition_handler || !partition_handler->handlers)
   {
-    if (wide_handler->sql_command == SQLCOM_UNLOCK_TABLES)
-    {
-      /* lock tables does not call reset() */
-      /* unlock tables does not call store_lock() */
-      wide_handler->lock_table_type = 0;
-    }
+    DBUG_RETURN(lock_tables()); /* Non-partitioned table */
+  }
 
-    /* lock/unlock tables */
-    if (partition_handler && partition_handler->handlers)
-    {
-      for (uint roop_count= 0; roop_count < partition_handler->no_parts;
-           ++roop_count)
-      {
-        if (unlikely((error_num =
-          partition_handler->handlers[roop_count]->lock_tables())))
-        {
-          DBUG_RETURN(error_num);
-        }
-      }
-    }
-    else if (unlikely((error_num= lock_tables())))
+  for (uint i= 0; i < partition_handler->no_parts; ++i)
+  {
+    if (unlikely((error_num= partition_handler->handlers[i]->lock_tables())))
     {
       DBUG_RETURN(error_num);
     }
diff --git a/storage/spider/mysql-test/spider/bugfix/disabled.def b/storage/spider/mysql-test/spider/bugfix/disabled.def
index 68de2b6bfde..e19ea07b76b 100644
--- a/storage/spider/mysql-test/spider/bugfix/disabled.def
+++ b/storage/spider/mysql-test/spider/bugfix/disabled.def
@@ -1,2 +1 @@
 wait_timeout : MDEV-26045
-mdev_27239 : failed with ASAN build
diff --git a/storage/spider/spd_trx.cc b/storage/spider/spd_trx.cc
index 80658012506..657d2c3ebe3 100644
--- a/storage/spider/spd_trx.cc
+++ b/storage/spider/spd_trx.cc
@@ -93,128 +93,51 @@ uchar *spider_trx_ha_get_key(
   DBUG_RETURN((uchar*) trx_ha->table_name);
 }
 
-int spider_free_trx_conn(
-  SPIDER_TRX *trx,
-  bool trx_free
-) {
-  int roop_count;
+/*
+  Try to free the connections held by the given transaction.
+*/
+int spider_free_trx_conn(SPIDER_TRX *trx, bool trx_free)
+{
+  int loop_count= 0;
   SPIDER_CONN *conn;
+  HASH *conn_hash= &trx->trx_conn_hash;
+
   DBUG_ENTER("spider_free_trx_conn");
-  roop_count = 0;
-  if (
-    trx_free ||
-    spider_param_conn_recycle_mode(trx->thd) != 2
-  ) {
-    while ((conn = (SPIDER_CONN*) my_hash_element(&trx->trx_conn_hash,
-      roop_count)))
-    {
-      spider_conn_clear_queue_at_commit(conn);
-      if (conn->table_lock)
-      {
-        DBUG_ASSERT(!trx_free);
-        roop_count++;
-      } else
-        spider_free_conn_from_trx(trx, conn, FALSE, trx_free, &roop_count);
-    }
-    trx->trx_conn_adjustment++;
-  } else {
-    while ((conn = (SPIDER_CONN*) my_hash_element(&trx->trx_conn_hash,
-      roop_count)))
-    {
-      spider_conn_clear_queue_at_commit(conn);
-      if (conn->table_lock)
-      {
-        DBUG_ASSERT(!trx_free);
-      } else
-        conn->error_mode = 1;
-      roop_count++;
-    }
-  }
-#if defined(HS_HAS_SQLCOM) && defined(HAVE_HANDLERSOCKET)
-  roop_count = 0;
-  if (
-    trx_free ||
-    spider_param_hs_r_conn_recycle_mode(trx->thd) != 2
-  ) {
-    while ((conn = (SPIDER_CONN*) my_hash_element(&trx->trx_hs_r_conn_hash,
-      roop_count)))
-    {
-      if (conn->table_lock)
-      {
-        DBUG_ASSERT(!trx_free);
-        roop_count++;
-      } else
-        spider_free_conn_from_trx(trx, conn, FALSE, trx_free, &roop_count);
-    }
-    trx->trx_hs_r_conn_adjustment++;
-  } else {
-    while ((conn = (SPIDER_CONN*) my_hash_element(&trx->trx_hs_r_conn_hash,
-      roop_count)))
-    {
-      if (conn->table_lock)
-      {
-        DBUG_ASSERT(!trx_free);
-      } else
-        conn->error_mode = 1;
-      roop_count++;
-    }
+  DBUG_ASSERT(!trx_free || !trx->locked_connections);
+
+  /* Clear the connection queues in any case. */
+  while ((conn= (SPIDER_CONN *) my_hash_element(conn_hash, loop_count)))
+  {
+    spider_conn_clear_queue_at_commit(conn);
+    loop_count++;
   }
-  roop_count = 0;
-  if (
-    trx_free ||
-    spider_param_hs_w_conn_recycle_mode(trx->thd) != 2
-  ) {
-    while ((conn = (SPIDER_CONN*) my_hash_element(&trx->trx_hs_w_conn_hash,
-      roop_count)))
-    {
-      if (conn->table_lock)
-      {
-        DBUG_ASSERT(!trx_free);
-        roop_count++;
-      } else
-        spider_free_conn_from_trx(trx, conn, FALSE, trx_free, &roop_count);
-    }
-    trx->trx_hs_w_conn_adjustment++;
-  } else {
-    while ((conn = (SPIDER_CONN*) my_hash_element(&trx->trx_hs_w_conn_hash,
-      roop_count)))
+
+  if (trx_free || spider_param_conn_recycle_mode(trx->thd) != 2)
+  {
+    /* Free connections only when no connection is locked. */
+    if (!trx->locked_connections)
     {
-      if (conn->table_lock)
+      loop_count= 0;
+      while ((conn= (SPIDER_CONN *) my_hash_element(conn_hash, loop_count)))
       {
-        DBUG_ASSERT(!trx_free);
-      } else
-        conn->error_mode = 1;
-      roop_count++;
+        spider_free_conn_from_trx(trx, conn, FALSE, trx_free, &loop_count);
+      }
     }
+    trx->trx_conn_adjustment++;
+
+    DBUG_RETURN(0);
   }
-#endif
-#if defined(HS_HAS_SQLCOM) && defined(HAVE_HANDLERSOCKET)
-  if (trx_free)
+
+  loop_count= 0;
+  while ((conn= (SPIDER_CONN *) my_hash_element(conn_hash, loop_count)))
   {
-    while ((conn = (SPIDER_CONN*) my_hash_element(
-      &trx->trx_direct_hs_r_conn_hash, 0)))
-    {
-#ifdef HASH_UPDATE_WITH_HASH_VALUE
-      my_hash_delete_with_hash_value(&trx->trx_direct_hs_r_conn_hash,
-        conn->conn_key_hash_value, (uchar*) conn);
-#else
-      my_hash_delete(&trx->trx_direct_hs_r_conn_hash, (uchar*) conn);
-#endif
-      spider_free_conn(conn);
-    }
-    while ((conn = (SPIDER_CONN*) my_hash_element(
-      &trx->trx_direct_hs_w_conn_hash, 0)))
+    if (!conn->table_lock)
     {
-#ifdef HASH_UPDATE_WITH_HASH_VALUE
-      my_hash_delete_with_hash_value(&trx->trx_direct_hs_w_conn_hash,
-        conn->conn_key_hash_value, (uchar*) conn);
-#else
-      my_hash_delete(&trx->trx_direct_hs_w_conn_hash, (uchar*) conn);
-#endif
-      spider_free_conn(conn);
+      conn->error_mode= 1;
     }
+    loop_count++;
   }
-#endif
+
   DBUG_RETURN(0);
 }
 
@@ -3417,6 +3340,12 @@ int spider_commit(
   trx->bulk_access_conn_first = NULL;
 #endif
 
+  /*
+    We do (almost) nothing if the following two conditions are both met:
+
+    * This is just the end of a statement, not an explicit commit.
+    * The autocommit is OFF or we are in an explicit transaction.
+  */
   if (all || (!thd_test_options(thd, OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)))
   {
     if (trx->trx_start)