From 2a4c57333910345b312f305f00dd1f90228e49fc Mon Sep 17 00:00:00 2001 From: Kristian Nielsen Date: Wed, 8 Nov 2023 13:55:03 +0100 Subject: [PATCH] MDEV-32728: Wrong mutex usage 'LOCK_thd_data' and 'wait_mutex' Checking for kill with thd_kill_level() or check_killed() runs apc requests, which takes the LOCK_thd_kill mutex. But this is dangerous, as checking for kill needs to be called while holding many different mutexes, and can lead to cyclic mutex dependency and deadlock. But running apc is only "best effort", so skip running the apc if the LOCK_thd_kill is not available. The apc will then be run on next check of kill signal. Signed-off-by: Kristian Nielsen --- sql/my_apc.cc | 9 ++++++--- sql/my_apc.h | 4 ++-- sql/sql_class.cc | 2 +- sql/sql_class.h | 2 +- unittest/sql/my_apc-t.cc | 2 +- 5 files changed, 11 insertions(+), 8 deletions(-) diff --git a/sql/my_apc.cc b/sql/my_apc.cc index e0feabab8e2..5a4d2b653ec 100644 --- a/sql/my_apc.cc +++ b/sql/my_apc.cc @@ -197,13 +197,16 @@ bool Apc_target::make_apc_call(THD *caller_thd, Apc_call *call, This should be called periodically by the APC target thread. */ -void Apc_target::process_apc_requests() +void Apc_target::process_apc_requests(bool force) { while (1) { Call_request *request; - - mysql_mutex_lock(LOCK_thd_kill_ptr); + + if (force) + mysql_mutex_lock(LOCK_thd_kill_ptr); + else if (mysql_mutex_trylock(LOCK_thd_kill_ptr)) + break; // Mutex is blocked, try again later if (!(request= get_first_in_queue())) { /* No requests in the queue */ diff --git a/sql/my_apc.h b/sql/my_apc.h index 2c0a9ade314..29fa3172a12 100644 --- a/sql/my_apc.h +++ b/sql/my_apc.h @@ -70,10 +70,10 @@ public: bool process= !--enabled && have_apc_requests(); mysql_mutex_unlock(LOCK_thd_kill_ptr); if (unlikely(process)) - process_apc_requests(); + process_apc_requests(true); } - void process_apc_requests(); + void process_apc_requests(bool force); /* A lightweight function, intended to be used in frequent checks like this: diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 1b78f88bd3c..70e4ae88e3c 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -4724,7 +4724,7 @@ extern "C" enum thd_kill_levels thd_kill_level(const MYSQL_THD thd) if (unlikely(apc_target->have_apc_requests())) { if (thd == current_thd) - apc_target->process_apc_requests(); + apc_target->process_apc_requests(false); } return THD_IS_NOT_KILLED; } diff --git a/sql/sql_class.h b/sql/sql_class.h index 1c7839fc266..611b51ff974 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -3606,7 +3606,7 @@ public: return TRUE; } if (apc_target.have_apc_requests()) - apc_target.process_apc_requests(); + apc_target.process_apc_requests(false); return FALSE; } diff --git a/unittest/sql/my_apc-t.cc b/unittest/sql/my_apc-t.cc index c08e7281c92..9edb209b1f6 100644 --- a/unittest/sql/my_apc-t.cc +++ b/unittest/sql/my_apc-t.cc @@ -100,7 +100,7 @@ void *test_apc_service_thread(void *ptr) //apc_target.enable(); for (int i = 0; i < 10 && !service_should_exit; i++) { - apc_target.process_apc_requests(); + apc_target.process_apc_requests(true); my_sleep(int_rand(30)); } }