From d475643da872a78a8b9a4f806b768803959bbb69 Mon Sep 17 00:00:00 2001 From: unknown Date: Fri, 30 Jul 2004 00:53:25 +0200 Subject: [PATCH 1/2] Avoiding a theoretically possible crash (pthread_mutex_lock(0)) which could (at least in POSIX Threads books) happen on SMP machines, when a thread is going to wait on a condition and it is KILLed at the same time. Cleaning code a bit by adding a test in enter_cond() that we have the mutex (was already the case in all places where it's called except one which is fixed here). sql/log.cc: safe_mutex_assert_owner() is now in THD::enter_cond() sql/slave.cc: lock mutex before waiting on condition. sql/sql_class.cc: THD::awake(): before locking the mutex, let's test it's not zero; in theory indeed, the killer thread may see current_cond non-zero and current_mutex zero (order of assignments is not guaranteed by POSIX). A comment noting that there is still a small chance a KILL does not work and needs being re-issued. sql/sql_class.h: Assert in enter_cond() that we have the mutex. It is already the case in all places where we call enter_cond(), so better ensure it there. --- sql/log.cc | 1 - sql/slave.cc | 1 + sql/sql_class.cc | 12 +++++++++++- sql/sql_class.h | 1 + 4 files changed, 13 insertions(+), 2 deletions(-) diff --git a/sql/log.cc b/sql/log.cc index 559d30f28ba..a0e2196cc59 100644 --- a/sql/log.cc +++ b/sql/log.cc @@ -1544,7 +1544,6 @@ bool MYSQL_LOG::write(THD *thd,const char *query, uint query_length, void MYSQL_LOG:: wait_for_update(THD* thd, bool master_or_slave) { - safe_mutex_assert_owner(&LOCK_log); const char* old_msg = thd->enter_cond(&update_cond, &LOCK_log, master_or_slave ? "Has read all relay log; waiting for \ diff --git a/sql/slave.cc b/sql/slave.cc index 2269fc8d8cf..9e9e3045ad2 100644 --- a/sql/slave.cc +++ b/sql/slave.cc @@ -588,6 +588,7 @@ int start_slave_thread(pthread_handler h_func, pthread_mutex_t *start_lock, while (start_id == *slave_run_id) { DBUG_PRINT("sleep",("Waiting for slave thread to start")); + pthread_mutex_lock(cond_lock); const char* old_msg = thd->enter_cond(start_cond,cond_lock, "Waiting for slave thread to start"); pthread_cond_wait(start_cond,cond_lock); diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 44faa3d6963..eb6e74a58c4 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -299,8 +299,18 @@ void THD::awake(bool prepare_to_die) exits the cond in the time between read and broadcast, but that is ok since all we want to do is to make the victim thread get out of waiting on current_cond. + If we see a non-zero current_cond: it cannot be an old value (because + then exit_cond() should have run and it can't because we have mutex); so + it is the true value but maybe current_mutex is not yet non-zero (we're + in the middle of enter_cond() and there is a "memory order + inversion"). So we test the mutex too to not lock 0. + Note that there is a small chance we fail to kill. If victim has locked + current_mutex, and hasn't entered enter_cond(), then we don't know it's + going to wait on cond. Then victim goes into its cond "forever" (until + we issue a second KILL). True we have set its thd->killed but it may not + see it immediately and so may have time to reach the cond_wait(). */ - if (mysys_var->current_cond) + if (mysys_var->current_cond && mysys_var->current_mutex) { pthread_mutex_lock(mysys_var->current_mutex); pthread_cond_broadcast(mysys_var->current_cond); diff --git a/sql/sql_class.h b/sql/sql_class.h index 484a442af20..e045c70517e 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -539,6 +539,7 @@ public: const char* msg) { const char* old_msg = proc_info; + safe_mutex_assert_owner(mutex); mysys_var->current_mutex = mutex; mysys_var->current_cond = cond; proc_info = msg; From 1adf793ddfd6a4eab32e6d28c7a7152ae5422766 Mon Sep 17 00:00:00 2001 From: unknown Date: Fri, 30 Jul 2004 01:00:52 +0200 Subject: [PATCH 2/2] Reverting a line I had just added to slave.cc (mutex is already locked when we come at this place). sql/slave.cc: stupid me; this line is a mistake --- sql/slave.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/sql/slave.cc b/sql/slave.cc index 9e9e3045ad2..2269fc8d8cf 100644 --- a/sql/slave.cc +++ b/sql/slave.cc @@ -588,7 +588,6 @@ int start_slave_thread(pthread_handler h_func, pthread_mutex_t *start_lock, while (start_id == *slave_run_id) { DBUG_PRINT("sleep",("Waiting for slave thread to start")); - pthread_mutex_lock(cond_lock); const char* old_msg = thd->enter_cond(start_cond,cond_lock, "Waiting for slave thread to start"); pthread_cond_wait(start_cond,cond_lock);