From c6c2a2b8d463e0a103997aba81f18c37fcdc0597 Mon Sep 17 00:00:00 2001 From: Sergei Golubchik Date: Tue, 2 Jan 2024 22:23:26 +0100 Subject: [PATCH] MDEV-33150 double-locking of LOCK_thd_kill in performance_schema.session_status perfschema thread walker needs to take thread's LOCK_thd_kill to prevent the thread from disappearing why it's being looked at. But there's no need to lock it for the current thread. In fact, it was harmful as some code down the stack might take LOCK_thd_kill (e.g. set_killed() does it, and my_malloc_size_cb_func() calls set_killed()). And it caused a bunch of mutexes being locked under LOCK_thd_kill, which created problems later when my_malloc_size_cb_func() called set_killed() at some unspecified point under some random mutexes. --- .../perfschema/r/misc_session_status.result | 20 ++++++++++++++++++ .../perfschema/t/misc_session_status.test | 18 ++++++++++++++++ storage/perfschema/pfs_variable.cc | 21 ++++++++++++------- storage/perfschema/pfs_variable.h | 8 +++++-- 4 files changed, 58 insertions(+), 9 deletions(-) create mode 100644 mysql-test/suite/perfschema/r/misc_session_status.result create mode 100644 mysql-test/suite/perfschema/t/misc_session_status.test diff --git a/mysql-test/suite/perfschema/r/misc_session_status.result b/mysql-test/suite/perfschema/r/misc_session_status.result new file mode 100644 index 00000000000..3ce472fc617 --- /dev/null +++ b/mysql-test/suite/perfschema/r/misc_session_status.result @@ -0,0 +1,20 @@ +# +# MDEV-33150 double-locking of LOCK_thd_kill in performance_schema.session_status +# +set @old_innodb_io_capacity=@@global.innodb_io_capacity; +set @old_innodb_io_capacity_max=@@global.innodb_io_capacity_max; +select * from performance_schema.session_status limit 0; +VARIABLE_NAME VARIABLE_VALUE +set max_session_mem_used=32768; +select * from performance_schema.session_status; +ERROR HY000: The MariaDB server is running with the --max-session-mem-used=32768 option so it cannot execute this statement +set global innodb_io_capacity_max=100; +Warnings: +Warning 1210 Setting innodb_io_capacity_max 100 lower than innodb_io_capacity 200. +Warning 1210 Setting innodb_io_capacity to 100 +set max_session_mem_used=default; +set global innodb_io_capacity=@old_innodb_io_capacity; +Warnings: +Warning 1210 Setting innodb_io_capacity to 200 higher than innodb_io_capacity_max 100 +Warning 1210 Setting innodb_max_io_capacity to 400 +set global innodb_io_capacity_max=@old_innodb_io_capacity_max; diff --git a/mysql-test/suite/perfschema/t/misc_session_status.test b/mysql-test/suite/perfschema/t/misc_session_status.test new file mode 100644 index 00000000000..ea662ce6738 --- /dev/null +++ b/mysql-test/suite/perfschema/t/misc_session_status.test @@ -0,0 +1,18 @@ +--source include/not_embedded.inc +--source include/have_perfschema.inc +--echo # +--echo # MDEV-33150 double-locking of LOCK_thd_kill in performance_schema.session_status +--echo # +source include/have_innodb.inc; +set @old_innodb_io_capacity=@@global.innodb_io_capacity; +set @old_innodb_io_capacity_max=@@global.innodb_io_capacity_max; +select * from performance_schema.session_status limit 0; # discover the table +set max_session_mem_used=32768; +--error ER_OPTION_PREVENTS_STATEMENT +# this used to crash, when OOM happened under LOCK_thd_kill +select * from performance_schema.session_status; +# this used to cause mutex lock order violation when OOM happened under LOCK_global_system_variables +set global innodb_io_capacity_max=100; +set max_session_mem_used=default; +set global innodb_io_capacity=@old_innodb_io_capacity; +set global innodb_io_capacity_max=@old_innodb_io_capacity_max; diff --git a/storage/perfschema/pfs_variable.cc b/storage/perfschema/pfs_variable.cc index aa07686139b..e2193cfb8a6 100644 --- a/storage/perfschema/pfs_variable.cc +++ b/storage/perfschema/pfs_variable.cc @@ -254,7 +254,8 @@ int PFS_system_variable_cache::do_materialize_all(THD *unsafe_thd) } /* Release lock taken in get_THD(). */ - mysql_mutex_unlock(&m_safe_thd->LOCK_thd_kill); + if (m_safe_thd != current_thd) + mysql_mutex_unlock(&m_safe_thd->LOCK_thd_kill); m_materialized= true; ret= 0; @@ -354,7 +355,8 @@ int PFS_system_variable_cache::do_materialize_session(PFS_thread *pfs_thread) } /* Release lock taken in get_THD(). */ - mysql_mutex_unlock(&m_safe_thd->LOCK_thd_kill); + if (m_safe_thd != current_thd) + mysql_mutex_unlock(&m_safe_thd->LOCK_thd_kill); m_materialized= true; ret= 0; @@ -407,7 +409,8 @@ int PFS_system_variable_cache::do_materialize_session(PFS_thread *pfs_thread, ui } /* Release lock taken in get_THD(). */ - mysql_mutex_unlock(&m_safe_thd->LOCK_thd_kill); + if (m_safe_thd != current_thd) + mysql_mutex_unlock(&m_safe_thd->LOCK_thd_kill); m_materialized= true; ret= 0; @@ -458,7 +461,8 @@ int PFS_system_variable_cache::do_materialize_session(THD *unsafe_thd) } /* Release lock taken in get_THD(). */ - mysql_mutex_unlock(&m_safe_thd->LOCK_thd_kill); + if (m_safe_thd != current_thd) + mysql_mutex_unlock(&m_safe_thd->LOCK_thd_kill); m_materialized= true; ret= 0; @@ -990,7 +994,8 @@ int PFS_status_variable_cache::do_materialize_all(THD* unsafe_thd) manifest(m_safe_thd, m_show_var_array.front(), status_vars, "", false, false); /* Release lock taken in get_THD(). */ - mysql_mutex_unlock(&m_safe_thd->LOCK_thd_kill); + if (m_safe_thd != current_thd) + mysql_mutex_unlock(&m_safe_thd->LOCK_thd_kill); m_materialized= true; ret= 0; @@ -1036,7 +1041,8 @@ int PFS_status_variable_cache::do_materialize_session(THD* unsafe_thd) manifest(m_safe_thd, m_show_var_array.front(), status_vars, "", false, true); /* Release lock taken in get_THD(). */ - mysql_mutex_unlock(&m_safe_thd->LOCK_thd_kill); + if (m_safe_thd != current_thd) + mysql_mutex_unlock(&m_safe_thd->LOCK_thd_kill); m_materialized= true; ret= 0; @@ -1078,7 +1084,8 @@ int PFS_status_variable_cache::do_materialize_session(PFS_thread *pfs_thread) manifest(m_safe_thd, m_show_var_array.front(), status_vars, "", false, true); /* Release lock taken in get_THD(). */ - mysql_mutex_unlock(&m_safe_thd->LOCK_thd_kill); + if (m_safe_thd != current_thd) + mysql_mutex_unlock(&m_safe_thd->LOCK_thd_kill); m_materialized= true; ret= 0; diff --git a/storage/perfschema/pfs_variable.h b/storage/perfschema/pfs_variable.h index fda8dc692d9..7dc248269b3 100644 --- a/storage/perfschema/pfs_variable.h +++ b/storage/perfschema/pfs_variable.h @@ -211,8 +211,12 @@ public: if (thd != m_unsafe_thd) return false; - /* Hold this lock to keep THD during materialization. */ - mysql_mutex_lock(&thd->LOCK_thd_kill); + /* + Hold this lock to keep THD during materialization. + But don't lock current_thd (to be able to use set_killed() later + */ + if (thd != current_thd) + mysql_mutex_lock(&thd->LOCK_thd_kill); return true; } void set_unsafe_thd(THD *unsafe_thd) { m_unsafe_thd= unsafe_thd; }