From ae0d38ae0c8ab63a31bfd19407990d93ef3d9008 Mon Sep 17 00:00:00 2001 From: Herman Lee Date: Wed, 13 May 2020 12:03:45 -0700 Subject: [PATCH] Prevent flushing nonsuper_connections and Reference count nonsuper_connections Summary: nonsuper_connections was getting underflows because it was getting reset during "flush status". In 8.0, the flags should have been updated to SHOW_LONG_NOFLUSH. It is also possible for privileges to be granted/revoked after connection setup, but the nonsuper_connections count were not adjusted appropriately. On cleanup, the new privileges were used to determine if nonsuper_connections needed to be decremented. Improve the robustness of tracking nonsuper_connections references by storing counts in the THD. Allow each THD to increment nonsuper_connections at most once and track nonsuper_connections increments within the THD. Apply the appropriate decrements when the structure is freed or reinitialized. Switching to atomic counters for nonsuper_connections removes the LOCK_user_conn requirements and reduces the risk of deadlocks when cleaning up THD's. Squash with D13987618 Reviewed By: pradeep1288 Differential Revision: D21562003 fbshipit-source-id: 9e4b8eb4df6 --- sql/auth/sql_authentication.cc | 5 +---- sql/mysqld.cc | 5 +++-- sql/mysqld.h | 3 ++- sql/sql_class.cc | 39 ++++++++++++++++++++++++++++++++++ sql/sql_class.h | 8 +++++++ sql/sql_connect.cc | 10 ++++++--- sql/sql_parse.cc | 4 +--- 7 files changed, 61 insertions(+), 13 deletions(-) diff --git a/sql/auth/sql_authentication.cc b/sql/auth/sql_authentication.cc index 643ecb926ba9..8c31b62ed3bf 100644 --- a/sql/auth/sql_authentication.cc +++ b/sql/auth/sql_authentication.cc @@ -3523,12 +3523,9 @@ int acl_authenticate(THD *thd, enum_server_command command) { mpvio.db.str)); if (!thd->m_main_security_ctx.check_access(SUPER_ACL)) { - mysql_mutex_lock(&LOCK_user_conn); // this is non-super user, increment nonsuper_connections - nonsuper_connections++; const bool limit_reached = - nonsuper_connections > max_nonsuper_connections; - mysql_mutex_unlock(&LOCK_user_conn); + thd->add_nonsuper_connections_ref() > max_nonsuper_connections; if (max_nonsuper_connections && limit_reached) { // max_nonsuper_connections limit reached diff --git a/sql/mysqld.cc b/sql/mysqld.cc index 91f5b7280f08..301d6c2259e0 100644 --- a/sql/mysqld.cc +++ b/sql/mysqld.cc @@ -1262,7 +1262,8 @@ ulong specialflag = 0; ulong binlog_cache_use = 0, binlog_cache_disk_use = 0; ulong binlog_stmt_cache_use = 0, binlog_stmt_cache_disk_use = 0; ulong max_connections, max_connect_errors; -ulong max_nonsuper_connections = 0, nonsuper_connections = 0; +ulong max_nonsuper_connections = 0; +std::atomic nonsuper_connections(0); ulong opt_max_running_queries, opt_max_waiting_queries; ulong rpl_stop_slave_timeout = LONG_TIMEOUT; bool rpl_skip_tx_api = false; @@ -9130,7 +9131,7 @@ SHOW_VAR status_vars[] = { SHOW_SCOPE_GLOBAL}, {"Max_used_connections_time", (char *)&show_max_used_connections_time, SHOW_FUNC, SHOW_SCOPE_GLOBAL}, - {"Non_super_connections", (char *)&nonsuper_connections, SHOW_LONG, + {"Non_super_connections", (char *)&nonsuper_connections, SHOW_LONG_NOFLUSH, SHOW_SCOPE_GLOBAL}, {"Not_flushed_delayed_rows", (char *)&delayed_rows_in_use, SHOW_LONG_NOFLUSH, SHOW_SCOPE_GLOBAL}, diff --git a/sql/mysqld.h b/sql/mysqld.h index 7ce7a6c5fa72..4e74c3f5f267 100644 --- a/sql/mysqld.h +++ b/sql/mysqld.h @@ -331,7 +331,8 @@ extern MYSQL_PLUGIN_IMPORT ulong max_connections; extern ulong opt_max_running_queries, opt_max_waiting_queries; extern ulong max_digest_length; extern ulong max_connect_errors, connect_timeout; -extern ulong max_nonsuper_connections, nonsuper_connections; +extern ulong max_nonsuper_connections; +extern std::atomic nonsuper_connections; extern bool opt_slave_allow_batching; extern ulong slave_trans_retries; extern uint slave_net_timeout; diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 44bff4d75dda..5a484b74e323 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -1135,6 +1135,9 @@ void THD::release_resources() { get_protocol_classic()->end_net(); } + /* Free nonsuper connection reference if needed */ + remove_nonsuper_connections_ref(true); + /* modification plan for UPDATE/DELETE should be freed. */ DBUG_ASSERT(query_plan.get_modification_plan() == NULL); mysql_mutex_unlock(&LOCK_query_plan); @@ -1198,6 +1201,8 @@ THD::~THD() { if (!m_release_resources_done) release_resources(); + DBUG_ASSERT(nonsuper_ref == 0); + clear_next_event_pos(); /* Ensure that no one is using THD */ @@ -2492,6 +2497,40 @@ void THD::increment_questions_counter() { DBUG_VOID_RETURN; } +ulong THD::add_nonsuper_connections_ref() { + DBUG_ENTER("THD::add_nonsuper_connections_ref"); + + // Changing user temporarily is considered two references + // If max_nonsuper_connections is 1, then this could be a problem + nonsuper_ref++; + ulong count = ++nonsuper_connections; + + DBUG_RETURN(count); +} + +void THD::remove_nonsuper_connections_ref(bool clear) { + DBUG_ENTER("THD::remove_nonsuper_connections_ref"); + + if (nonsuper_ref == 0) { + DBUG_VOID_RETURN; + } + + bool underflow; + if (clear) { + underflow = nonsuper_connections < nonsuper_ref; + nonsuper_connections -= nonsuper_ref; + nonsuper_ref = 0; + } else { + underflow = ((nonsuper_connections--) == 0); + nonsuper_ref--; + } + + DBUG_ASSERT(!underflow); + (void)underflow; // prevent compiler warning about unused on non-debug builds + + DBUG_VOID_RETURN; +} + /* Reset per-hour user resource limits when it has been more than an hour since they were last checked diff --git a/sql/sql_class.h b/sql/sql_class.h index 2c7540cc30ad..6f16a16af613 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -930,6 +930,14 @@ class THD : public MDL_context_owner, bool should_write_hlc = false; database_container databases; + /* Nonsuper_connections reference counting */ + private: + ulong nonsuper_ref = 0; + + public: + ulong add_nonsuper_connections_ref(); + void remove_nonsuper_connections_ref(bool clear = false); + /** The function checks whether the thread is processing queries from binlog, as automatically generated by mysqlbinlog. diff --git a/sql/sql_connect.cc b/sql/sql_connect.cc index 571e71d7e5d3..fb8471db7642 100644 --- a/sql/sql_connect.cc +++ b/sql/sql_connect.cc @@ -276,11 +276,15 @@ void release_user_connection(THD *thd) { const USER_CONN *uc = thd->get_user_connect(); DBUG_ENTER("release_user_connection"); + // In 8.0, uc might be null this connection. + // It is also possible for the user's privilege access to have changed + // due to REVOKE after the connection was established. This means + // add_nonsuper_connections_ref was not called (at the time, it had + // SUPER access), but remove_nonsuper_connections_ref() is called here + // during cleanup because the it no longer has SUPER access. if (!thd->m_main_security_ctx.check_access(SUPER_ACL)) { - mysql_mutex_lock(&LOCK_user_conn); // this is non-super user, decrement nonsuper_connections - nonsuper_connections--; - mysql_mutex_unlock(&LOCK_user_conn); + thd->remove_nonsuper_connections_ref(); } if (uc) { diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 4381f7cd0ae4..0b8e93c435c1 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -1782,10 +1782,8 @@ bool dispatch_command(THD *thd, const COM_DATA *com_data, #endif /* HAVE_PSI_THREAD_INTERFACE */ if (!save_security_ctx.check_access(SUPER_ACL)) { - mysql_mutex_lock(&LOCK_user_conn); // previous user was a non-super user, decrement nonsuper_connections - nonsuper_connections--; - mysql_mutex_unlock(&LOCK_user_conn); + thd->remove_nonsuper_connections_ref(); } if (save_user_connect) decrease_user_connections(save_user_connect); mysql_mutex_lock(&thd->LOCK_thd_data);