MDEV-40298 Use-After-Free when SQL Thread stops in the middle of SHOW SLAVE STATUS

[ParadoxV5]

• Fixes MDEV-40298

MDEV-36287 acknowledged that SHOW SLAVE STATUS needed to acquire a mutex lock before accessing the SQL Thread’s THD, as otherwise it may access invalid memory if a concurrent STOP SLAVE deletes the THD. But it missed that the SQL Thread’s mutex is mi->rli.run_lock, not mi->run_lock, so the bug was still not fixed.

This commit fills the oversight in by acquiring the correct corresponding lock for each of Slave_IO_State & Slave_SQL_State.

[ParadoxV5]

The code documentation for Relay_log_info::data_lock reads:

standard lock acquisition order to avoid deadlocks:
run_lock, data_lock, relay_log.LOCK_log, relay_log.LOCK_index

But mi->rli.run_lock is not held when acquiring mi->rli.data_lock in the old code down there, and MDEV-36287 even released mi->run_lock holding at this point.

Is the code comment obsolete, or could there have been a deadlock this whole time?

[knielsen]

This comment is about the requirements if we need to hold the run_lock and the data_lock at the same time. Then it is necessary to take them always in the same order to avoid the deadlock where one thread is holding lock A and waiting for B, while another thread is holding B and waiting for A.

But when one lock is released before taking the other lock, there is no ordering requirement.

You can see some places in the code where we use mysql_mutex_record_order() to assert on incorrect locking order for some mutexes. Looks like not for run_lock and data_lock (you're welcome to add it if you want, but not directly related to this patch of course).

[knielsen]

Thanks, Jimmy!

It seems really dumb that in my MDEV-36287 patch I modified the code to hold the lock, but used the wrong lock! But it's clear in handle_slave_sql() that it's the mi->rli.run_lock that's the correct lock to protect rli.sql_driver_thd. And the mi itself is protected from going away until mi->release() is called, so I agree with your patch.

And thanks for doing a test case for the bug, test cases for race conditions are often a bother to implement.

Approved after fixing a couple suggestions for improving the test case.

• Kristian.

[knielsen]

This comment is about the requirements if we need to hold the run_lock and the data_lock at the same time. Then it is necessary to take them always in the same order to avoid the deadlock where one thread is holding lock A and waiting for B, while another thread is holding B and waiting for A.

But when one lock is released before taking the other lock, there is no ordering requirement.

You can see some places in the code where we use mysql_mutex_record_order() to assert on incorrect locking order for some mutexes. Looks like not for run_lock and data_lock (you're welcome to add it if you want, but not directly related to this patch of course).

[knielsen]

ParadoxV5 ***@***.***> writes:

> You are not using `--source include/master-slave.inc` here, nor `rpl_end.inc` to clean up. Since this is entirely contained on the slave side, actual replication not required.

Ah, right, I see. No IO thread, no actual connection to a master, no extra server needed to be started. Nice! - Kristian.