From de130323b4401ba9dfcb08ebd7f8d688cb317a80 Mon Sep 17 00:00:00 2001 From: Sergei Golubchik Date: Wed, 28 Sep 2022 14:27:55 +0200 Subject: [PATCH] MDEV-29368 Assertion `trx->mysql_thd == thd' failed in innobase_kill_query from process_timers/timer_handler and use-after-poison in innobase_kill_query This is a 10.5 version of 9b750dcbd89e, fix for MDEV-23536 Race condition between KILL and transaction commit InnoDB needs to remove trx from thd before destroying it (trx), otherwise a concurrent KILL might get a pointer from thd to a destroyed trx. ha_close_connection() should allow engines to clear ha_data in hton->on close_connection(). To prevent the engine from being unloaded while hton->close_connection() is running, we remove the lock from ha_data and unlock the plugin manually. --- mysql-test/main/kill_debug.result | 16 ++++++++++++++++ mysql-test/main/kill_debug.test | 20 ++++++++++++++++++++ sql/handler.cc | 9 ++++----- sql/sql_class.cc | 2 ++ sql/sql_parse.cc | 2 +- storage/innobase/handler/ha_innodb.cc | 2 ++ 6 files changed, 45 insertions(+), 6 deletions(-) diff --git a/mysql-test/main/kill_debug.result b/mysql-test/main/kill_debug.result index d0fadb5c7b5..061e7602383 100644 --- a/mysql-test/main/kill_debug.result +++ b/mysql-test/main/kill_debug.result @@ -221,3 +221,19 @@ DROP FUNCTION MY_KILL; set global sql_mode=default; disconnect con1; disconnect con2; +# +# MDEV-29368 Assertion `trx->mysql_thd == thd' failed in innobase_kill_query from process_timers/timer_handler and use-after-poison in innobase_kill_query +# +connect foo,localhost,root; +create table t1 (a int) engine=innodb; +insert t1 values (1); +set debug_sync='THD_cleanup_after_set_killed SIGNAL go0 WAIT_FOR go1'; +set debug_sync='innobase_connection_closed SIGNAL go2 WAIT_FOR go3'; +disconnect foo; +connection default; +set debug_sync='now WAIT_FOR go0'; +set debug_sync='found_killee SIGNAL go1 WAIT_FOR go2'; +kill $id; +set debug_sync='now SIGNAL go3'; +drop table t1; +set debug_sync='reset'; diff --git a/mysql-test/main/kill_debug.test b/mysql-test/main/kill_debug.test index ba6a0ced528..32a764004e3 100644 --- a/mysql-test/main/kill_debug.test +++ b/mysql-test/main/kill_debug.test @@ -9,6 +9,7 @@ -- source include/not_embedded.inc -- source include/have_debug_sync.inc +-- source include/have_innodb.inc set local sql_mode=""; set global sql_mode=""; @@ -296,3 +297,22 @@ DROP FUNCTION MY_KILL; set global sql_mode=default; disconnect con1; disconnect con2; + +--echo # +--echo # MDEV-29368 Assertion `trx->mysql_thd == thd' failed in innobase_kill_query from process_timers/timer_handler and use-after-poison in innobase_kill_query +--echo # +connect foo,localhost,root; +let $id=`select connection_id()`; +create table t1 (a int) engine=innodb; +insert t1 values (1); +set debug_sync='THD_cleanup_after_set_killed SIGNAL go0 WAIT_FOR go1'; +set debug_sync='innobase_connection_closed SIGNAL go2 WAIT_FOR go3'; +disconnect foo; + +connection default; +set debug_sync='now WAIT_FOR go0'; +set debug_sync='found_killee SIGNAL go1 WAIT_FOR go2'; +evalp kill $id; +set debug_sync='now SIGNAL go3'; +drop table t1; +set debug_sync='reset'; diff --git a/sql/handler.cc b/sql/handler.cc index 0d5609f753a..85600d02b63 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -894,15 +894,14 @@ void ha_close_connection(THD* thd) { for (auto i= 0; i < MAX_HA; i++) { - if (thd->ha_data[i].lock) + if (plugin_ref plugin= thd->ha_data[i].lock) { - handlerton *hton= plugin_hton(thd->ha_data[i].lock); + thd->ha_data[i].lock= NULL; + handlerton *hton= plugin_hton(plugin); if (hton->close_connection) hton->close_connection(hton, thd); - /* make sure SE didn't reset ha_data in close_connection() */ - DBUG_ASSERT(thd->ha_data[i].lock); - /* make sure ha_data is reset and ha_data_lock is released */ thd_set_ha_data(thd, hton, 0); + plugin_unlock(NULL, plugin); } DBUG_ASSERT(!thd->ha_data[i].ha_ptr); } diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 87526acbdba..87f5262f9ce 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -1533,6 +1533,8 @@ void THD::cleanup(void) wsrep_client_thread= false; #endif /* WITH_WSREP */ + DEBUG_SYNC(this, "THD_cleanup_after_set_killed"); + mysql_ha_cleanup(this); locked_tables_list.unlock_locked_tables(this); diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index ec22b539668..643d671878f 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -9271,7 +9271,7 @@ kill_one_thread(THD *thd, longlong id, killed_state kill_signal, killed_type typ tmp= find_thread_by_id(id, type == KILL_TYPE_QUERY); if (!tmp) DBUG_RETURN(error); - + DEBUG_SYNC(thd, "found_killee"); if (tmp->get_command() != COM_DAEMON) { /* diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index 25f2f4d635f..4a710a5c384 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -4791,6 +4791,7 @@ static int innobase_close_connection(handlerton *hton, THD *thd) DBUG_ASSERT(hton == innodb_hton_ptr); if (auto trx= thd_to_trx(thd)) { + thd_set_ha_data(thd, innodb_hton_ptr, NULL); if (trx->state == TRX_STATE_PREPARED && trx->has_logged_persistent()) { trx_disconnect_prepared(trx); @@ -4798,6 +4799,7 @@ static int innobase_close_connection(handlerton *hton, THD *thd) } innobase_rollback_trx(trx); trx->free(); + DEBUG_SYNC(thd, "innobase_connection_closed"); } return 0; }