Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ParadoxV5 wants to merge 1 commit into
base: 10.6
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions mysql-test/main/rli_run_lock.result
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
CHANGE MASTER TO master_host='127.0.0.1', master_user='root';
START SLAVE SQL_THREAD;
include/wait_for_slave_sql_to_start.inc
SET @@SESSION.debug_sync=
'hold_sss_with_run_lock SIGNAL sss_got_run_lock WAIT_FOR sss_continue';
SHOW SLAVE STATUS;
connect stopper, 127.0.0.1, root, , , $SERVER_MYPORT_1;
SET @@SESSION.debug_sync= 'now WAIT_FOR sss_got_run_lock';
STOP SLAVE SQL_THREAD;
connect continuer, 127.0.0.1, root, , , $SERVER_MYPORT_1;
SET @@SESSION.debug_sync= 'now SIGNAL sss_continue';
connection stopper;
disconnect continuer;
connection default;
disconnect stopper;
SET @@SESSION.debug_sync= RESET;
include/wait_for_slave_sql_to_stop.inc
RESET SLAVE ALL;
43 changes: 43 additions & 0 deletions mysql-test/main/rli_run_lock.test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# MDEV-40298
# Use-After-Free when SQL Thread stops in the middle of SHOW SLAVE STATUS
#
# Test that the correct synchronization is performed

--source include/have_debug_sync.inc
--source include/have_binlog_format_mixed.inc # No actual replication required
CHANGE MASTER TO master_host='127.0.0.1', master_user='root'; # basic setup

START SLAVE SQL_THREAD;
--source include/wait_for_slave_sql_to_start.inc


SET @@SESSION.debug_sync=
'hold_sss_with_run_lock SIGNAL sss_got_run_lock WAIT_FOR sss_continue';
--send SHOW SLAVE STATUS

--connect (stopper, 127.0.0.1, root, , , $SERVER_MYPORT_1)
SET @@SESSION.debug_sync= 'now WAIT_FOR sss_got_run_lock';
--send STOP SLAVE SQL_THREAD
# Wait a bit for the thread to "shut down"
# (It should not shut down, but block waiting for the ongoing SSS;
# the tested bug instead pulled this carpet under the SSS's feet.)
--sleep 1

--connect (continuer, 127.0.0.1, root, , , $SERVER_MYPORT_1)
SET @@SESSION.debug_sync= 'now SIGNAL sss_continue';
--connection stopper
--disconnect continuer

--reap
--connection default
--disconnect stopper


--disable_result_log
--reap
--enable_result_log

# Clean-up
SET @@SESSION.debug_sync= RESET;
--source include/wait_for_slave_sql_to_stop.inc
RESET SLAVE ALL;
5 changes: 4 additions & 1 deletion sql/slave.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3301,10 +3301,13 @@ static bool send_show_master_info_data(THD *thd, Master_info *mi, bool full,
protocol->store(mi->connection_name.str, mi->connection_name.length,
&my_charset_bin);

mysql_mutex_lock(&mi->run_lock);
mysql_mutex_lock(&mi->rli.run_lock);
THD *sql_thd= mi->rli.sql_driver_thd;
DEBUG_SYNC(thd, "hold_sss_with_run_lock");
const char *slave_sql_running_state=
sql_thd ? sql_thd->get_proc_info() : "";
mysql_mutex_unlock(&mi->rli.run_lock);
mysql_mutex_lock(&mi->run_lock);
THD *io_thd= mi->io_thd;
const char *slave_io_running_state= io_thd ? io_thd->get_proc_info() : "";
mysql_mutex_unlock(&mi->run_lock);
Comment on lines +3304 to 3313

@gemini-code-assist gemini-code-assist Bot Jul 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

While holding mi->rli.run_lock and mi->run_lock protects sql_thd and io_thd from being destroyed during the call to get_proc_info(), once the locks are released, the threads can terminate and their THD objects can be destroyed. If get_proc_info() returns a dynamically allocated string (e.g., allocated on the thread's mem_root), the pointers slave_sql_running_state and slave_io_running_state will become dangling pointers, leading to a Use-After-Free when they are later sent to the client via protocol->store.

To prevent this, we should copy the state strings into local String buffers while holding the respective locks.

    String sql_state_buf;
    const char *slave_sql_running_state= "";
    mysql_mutex_lock(&mi->rli.run_lock);
    THD *sql_thd= mi->rli.sql_driver_thd;
    DBUG_EXECUTE_IF("hold_sss_with_run_lock",
      DBUG_ASSERT(!debug_sync_set_action(thd,
        STRING_WITH_LEN("now SIGNAL sss_got_run_lock WAIT_FOR sss_continue")));
    );
    if (sql_thd)
    {
      const char *state= sql_thd->get_proc_info();
      if (state)
      {
        sql_state_buf.copy(state, strlen(state), &my_charset_bin);
        slave_sql_running_state= sql_state_buf.c_ptr_safe();
      }
      else
        slave_sql_running_state= nullptr;
    }
    mysql_mutex_unlock(&mi->rli.run_lock);
    String io_state_buf;
    const char *slave_io_running_state= "";
    mysql_mutex_lock(&mi->run_lock);
    THD *io_thd= mi->io_thd;
    if (io_thd)
    {
      const char *state= io_thd->get_proc_info();
      if (state)
      {
        io_state_buf.copy(state, strlen(state), &my_charset_bin);
        slave_io_running_state= io_state_buf.c_ptr_safe();
      }
      else
        slave_io_running_state= nullptr;
    }
    mysql_mutex_unlock(&mi->run_lock);

@ParadoxV5 ParadoxV5 Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If get_proc_info() returns a dynamically allocated string (e.g., allocated on the thread's mem_root)

Then this is UB due to violation of contract, because THD::proc_info is forbidden to return a dynamic string.

  /*
    ...
    This member is accessed and assigned without any synchronization.
    Therefore, it may point only to constant (statically
    allocated) strings, which memory won't go away over time.
  */
  const char *proc_info;

MDEV-36839 is a prior incident.

@ParadoxV5 ParadoxV5 Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 knielsen Jul 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand Down