Commit 2fcb5d65 authored by Monty's avatar Monty

Fixed possible mutex-wrong-order with KILL USER

The old code collected a list of THD's, locked the THD's from getting
deleted by locking two mutex and then later in a separate loop
sent a kill signal to each THD.

The problem with this approach is that, as THD's can be reused,
the second time the THD is killed, the mutex can be taken in
different order, which signals failures in safe_mutex.

Fixed by sending the kill signal directly and not collect the THD's
in a list to be signaled later.  This is the same approach we are using
in kill_zombie_dump_threads().

Other things:
- Reset safe_mutex_t->locked_mutex when freed (Safety fix)
parent 1ca813bf
...@@ -665,6 +665,7 @@ void safe_mutex_free_deadlock_data(safe_mutex_t *mp) ...@@ -665,6 +665,7 @@ void safe_mutex_free_deadlock_data(safe_mutex_t *mp)
my_hash_free(mp->used_mutex); my_hash_free(mp->used_mutex);
my_hash_free(mp->locked_mutex); my_hash_free(mp->locked_mutex);
my_free(mp->locked_mutex); my_free(mp->locked_mutex);
mp->locked_mutex= 0;
mp->create_flags|= MYF_NO_DEADLOCK_DETECTION; mp->create_flags|= MYF_NO_DEADLOCK_DETECTION;
} }
} }
......
...@@ -9371,11 +9371,13 @@ kill_one_thread(THD *thd, my_thread_id id, killed_state kill_signal, killed_type ...@@ -9371,11 +9371,13 @@ kill_one_thread(THD *thd, my_thread_id id, killed_state kill_signal, killed_type
struct kill_threads_callback_arg struct kill_threads_callback_arg
{ {
kill_threads_callback_arg(THD *thd_arg, LEX_USER *user_arg): kill_threads_callback_arg(THD *thd_arg, LEX_USER *user_arg,
thd(thd_arg), user(user_arg) {} killed_state kill_signal_arg):
thd(thd_arg), user(user_arg), kill_signal(kill_signal_arg), counter(0) {}
THD *thd; THD *thd;
LEX_USER *user; LEX_USER *user;
List<THD> threads_to_kill; killed_state kill_signal;
uint counter;
}; };
...@@ -9398,11 +9400,12 @@ static my_bool kill_threads_callback(THD *thd, kill_threads_callback_arg *arg) ...@@ -9398,11 +9400,12 @@ static my_bool kill_threads_callback(THD *thd, kill_threads_callback_arg *arg)
{ {
return MY_TEST(arg->thd->security_ctx->master_access & PROCESS_ACL); return MY_TEST(arg->thd->security_ctx->master_access & PROCESS_ACL);
} }
if (!arg->threads_to_kill.push_back(thd, arg->thd->mem_root)) arg->counter++;
{
mysql_mutex_lock(&thd->LOCK_thd_kill); // Lock from delete mysql_mutex_lock(&thd->LOCK_thd_kill); // Lock from delete
mysql_mutex_lock(&thd->LOCK_thd_data); mysql_mutex_lock(&thd->LOCK_thd_data);
} thd->awake_no_mutex(arg->kill_signal);
mysql_mutex_unlock(&thd->LOCK_thd_data);
mysql_mutex_unlock(&thd->LOCK_thd_kill);
} }
} }
return 0; return 0;
...@@ -9412,42 +9415,17 @@ static my_bool kill_threads_callback(THD *thd, kill_threads_callback_arg *arg) ...@@ -9412,42 +9415,17 @@ static my_bool kill_threads_callback(THD *thd, kill_threads_callback_arg *arg)
static uint kill_threads_for_user(THD *thd, LEX_USER *user, static uint kill_threads_for_user(THD *thd, LEX_USER *user,
killed_state kill_signal, ha_rows *rows) killed_state kill_signal, ha_rows *rows)
{ {
kill_threads_callback_arg arg(thd, user); kill_threads_callback_arg arg(thd, user, kill_signal);
DBUG_ENTER("kill_threads_for_user"); DBUG_ENTER("kill_threads_for_user");
*rows= 0;
if (unlikely(thd->is_fatal_error)) // If we run out of memory
DBUG_RETURN(ER_OUT_OF_RESOURCES);
DBUG_PRINT("enter", ("user: %s signal: %u", user->user.str, DBUG_PRINT("enter", ("user: %s signal: %u", user->user.str,
(uint) kill_signal)); (uint) kill_signal));
*rows= 0;
if (server_threads.iterate(kill_threads_callback, &arg)) if (server_threads.iterate(kill_threads_callback, &arg))
DBUG_RETURN(ER_KILL_DENIED_ERROR); DBUG_RETURN(ER_KILL_DENIED_ERROR);
if (!arg.threads_to_kill.is_empty()) *rows= arg.counter;
{
List_iterator_fast<THD> it2(arg.threads_to_kill);
THD *next_ptr;
THD *ptr= it2++;
do
{
ptr->awake_no_mutex(kill_signal);
/*
Careful here: The list nodes are allocated on the memroots of the
THDs to be awakened.
But those THDs may be terminated and deleted as soon as we release
LOCK_thd_kill, which will make the list nodes invalid.
Since the operation "it++" dereferences the "next" pointer of the
previous list node, we need to do this while holding LOCK_thd_kill.
*/
next_ptr= it2++;
mysql_mutex_unlock(&ptr->LOCK_thd_kill);
mysql_mutex_unlock(&ptr->LOCK_thd_data);
(*rows)++;
} while ((ptr= next_ptr));
}
DBUG_RETURN(0); DBUG_RETURN(0);
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment