Race condition in kill_threads_for_user

[Vladislav Vaintroub]

kill_threads_for_user has a race condition which can result in invalid pointer in the threads_to_kill list.

  while ((ptr= it++))
    {
      ptr->awake(kill_signal);
      mysql_mutex_unlock(&ptr->LOCK_thd_data);
      (*rows)++;
    }

The problem with this code is that once ptr->LOCK_thd_data is unlocked, very short thereafter memory pointed to by
'ptr' can be freed, and the ptr->next becomes invalid, and ptr=it++ might crash.

Possible fix would be calculating 'next' pointer before unlocking the LOCK_thd_data.

[Michael Widenius (monty)]

Hi!

don't see an issue with the above code.

'it' above is threads_to_kill that is not related to THD in any way.
In other words, we never use ptr->next anywhere.
So even if ptr disappears, it++ will point to the next element in the list.

[Kristian Nielsen (knielsen)]

It's actually a real problem I think, with slightly different explanation
perhaps.

The list nodes in threads_to_kill are allocated on the mem_roots of the
individual THDs. When we run "it++", behind the scenes what it does is that it
dereferences the "next" pointer of the previous list node, which is allocated
on the mem_root of the previous (now perhaps freed) THD.

Here is a patch that fixes it:

=== modified file 'sql/sql_parse.cc'
--- sql/sql_parse.cc 2012-01-16 19:16:35 +0000
+++ sql/sql_parse.cc 2012-02-10 13:39:31 +0000
@@ -1324,7 +1324,7 @@ bool dispatch_command(enum enum_server_c
   {
     STATUS_VAR *current_global_status_var; // Big; Don't allocate on stack
     ulong uptime;
- uint length;
+ uint length __attribute__((unused));
     ulonglong queries_per_second1000;
     char buff[250];
     uint buff_len= sizeof(buff);
@@ -6674,13 +6674,23 @@ static uint kill_threads_for_user(THD *t
   if (!threads_to_kill.is_empty())
   {
     List_iterator_fast<THD> it(threads_to_kill);
- THD *ptr;
- while ((ptr= it++))
+ THD *next_ptr;
+ THD *ptr= it++;
+ do
     {
       ptr->awake(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_data, 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_data.
+ */
+ next_ptr= it++;
       mysql_mutex_unlock(&ptr->LOCK_thd_data);
       (*rows)++;
- }
+ } while ((ptr= next_ptr));
   }
   DBUG_RETURN(0);
 }

[Kristian Nielsen (knielsen)]

Fixed in 5.3.4 and 5.5