Details
-
Type:
Bug
-
Status: Closed
-
Resolution: Fixed
-
Affects Version/s: None
-
Fix Version/s: None
-
Component/s: None
-
Labels:
Description
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.
Gliffy Diagrams
Attachments
Activity
- All
- Comments
- Work Log
- History
- Activity
- Transitions
Re: Race condition in kill_threads_for_user
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);
}
Re: Race condition in kill_threads_for_user
Fixed in 5.3.4 and 5.5
Launchpad bug id: 910817
Re: Race condition in kill_threads_for_user
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.