[MDEV-3504] LP:910817 - Race condition in kill_threads_for_user Created: 2012-01-02  Updated: 2012-10-04  Resolved: 2012-10-04

Status: Closed
Project: MariaDB Server
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Bug
Reporter: Vladislav Vaintroub Assignee: Kristian Nielsen
Resolution: Fixed Votes: 0
Labels: Launchpad

Attachments: XML File LPexportBug910817.xml    

 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.

 Comments   
Comment by Michael Widenius [ 2012-01-02 ]

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.

Comment by Kristian Nielsen [ 2012-02-10 ]

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);

}

Comment by Kristian Nielsen [ 2012-02-10 ]

Re: Race condition in kill_threads_for_user
Fixed in 5.3.4 and 5.5

Comment by Rasmus Johansson (Inactive) [ 2012-02-10 ]

Launchpad bug id: 910817

Generated at Thu Feb 08 06:49:07 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.