Bug #74853 THD killed_state isn't thread safe on non-intel
Submitted: 14 Nov 2014 6:07 Modified: 1 Jul 2015 5:03
Reporter: Stewart Smith Email Updates:
Status: Verified Impact on me:
Category:MySQL Server: Replication Severity:S2 (Serious)
Version:5.7.7 OS:Any
Assigned to: CPU Architecture:ARM
Tags: PowerPC

[14 Nov 2014 6:07] Stewart Smith
THD has the following member:
  killed_state volatile killed;

This is used all over the place to detect if threads are killed or not.

There's one problem with this however, volatile doesn't do barriers. So everything could be awfully different for each load that's done.

I think this is actually a bug in replication. In start_slave_thread() there's not only this accessed but a bunch of other things that are volatile too. It appears that there's likely an opportunity to wedge things if a slave thread is killed at exactly the wrong time.

there's also the possibility to lose the killing of a thread in debug builds (a DBUG_EVALUATE_IF will read&write thd->killed, possibly clobbering a changed value).

There's also several places where killed is likely to be missed, potentially for quite a while... some of the other logic around the place looks suspect with a volitle thd->killed, at the very least leading to rather unexpected behaviour.

But the big thing? Since killed is used for more things, there exists the possibility of code that's like "if(!killed) killed=thing;" which would completely fall apart on POWER (and ARM and maybe others) in the current code (this may currently be the case... there's *sooo* many places to audit to work out if this is an actual bug).

How to repeat:
This is a tricky one... there's several scenarios described above where this could be a problem, largely caught by code analysis as especially in the various error paths, mtr test suite is likely quite inadequate to catch them.

If this does all magically currently work, it's not because of design, it's distinctly by accident.

I've observed some strange shutdown behaviour on POWER8 at least, which could be explained by the use of volatile around thd->killed and threads just reading stale values for a long time.

I'm pretty sure the logic in opt_explain.cc for explaining other threads could be convinced to do the wrong thing when reading a stale killed value, although a very close audit would be required to be sure.

My patch is if anything, wonderfully defensive.

Suggested fix:
my patch!

I *think* the correct thing to do here is to remove const for a bunch of the methods that do end up checking an atomic variable... which is why there's changes in const in my patch. I'm going to ask Paul McKenney about this as he's been knee deep in the C++ spec relating to multicore and can likely give a definitive answer.

This changes plugin ABI slightly:
< int thd_killed(const void* thd);
< void thd_set_kill_status(const void* thd);
> int thd_killed(void* thd);
> void thd_set_kill_status(void* thd);

but the idea of plugin ABI is pure fantasy anyway :)
[14 Nov 2014 6:08] Stewart Smith
THD::killed should not be volatile, should instead use correct accessors

(*) I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it.

Contribution: volatile-thd-killed.patch (text/x-patch), 76.41 KiB.

[10 Apr 2015 8:08] Stewart Smith
Still in 5.7.7
[1 Jul 2015 5:03] MySQL Verification Team
Hello Stewart,

Thank you for the report and contribution.

[22 Apr 2020 4:08] Kang Xia
volatile_thd_killed.patch is not a complete resolution.

abort_loop used in mysql_src_5.7.29/sql/sql_class.h:send_kill_message()
is defined as volatile too:

extern MYSQL_PLUGIN_IMPORT bool volatile abort_loop;