From 1b5cf2f7e7995f8ee17da76eb28633f652852d8f Mon Sep 17 00:00:00 2001 From: Sergey Vojtovich Date: Tue, 19 Mar 2019 20:04:10 +0400 Subject: [PATCH] Safe session_track_system_variables snapshot When enabling system variables tracker, make a copy of global_system_variables.session_track_system_variables under LOCK_global_system_variables. This protects from concurrent variable updates and potential freed memory access, as well as from variable reconstruction (which was previously protected by LOCK_plugin). We can also use this copy as a session variable pointer, so that we don't have to allocate memory and reconstruct it every time it is referenced. For this very reason we don't need buffer_length stuff anymore. As well as don't we need to take LOCK_plugin early in ::enable(). Unified ::parse_var_list() to acquire LOCK_plugin unconditionally. For no apparent reason it wasn't previously acquired for global variable update. Part of MDEV-14984 - regression in connect performance --- sql/session_tracker.cc | 93 ++++++++++++++++++++++-------------------- sql/session_tracker.h | 30 ++------------ sql/sql_plugin.cc | 12 ++++++ sql/sys_vars.ic | 2 - 4 files changed, 65 insertions(+), 72 deletions(-) diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc index 81da43a0946..8b0e247767c 100644 --- a/sql/session_tracker.cc +++ b/sql/session_tracker.cc @@ -38,7 +38,6 @@ static const unsigned int EXTRA_ALLOC= 1024; void Session_sysvars_tracker::vars_list::reinit() { - buffer_length= 0; track_all= 0; if (m_registered_sysvars.records) my_hash_reset(&m_registered_sysvars); @@ -58,7 +57,6 @@ void Session_sysvars_tracker::vars_list::copy(vars_list* from, THD *thd) { track_all= from->track_all; free_hash(); - buffer_length= from->buffer_length; m_registered_sysvars= from->m_registered_sysvars; from->init(); } @@ -111,7 +109,6 @@ bool Session_sysvars_tracker::vars_list::insert(const sys_var *svar) in case of invalid/duplicate values. @param char_set [IN] charecter set information used for string manipulations. - @param take_mutex [IN] take LOCK_plugin @return true Error @@ -120,27 +117,21 @@ bool Session_sysvars_tracker::vars_list::insert(const sys_var *svar) bool Session_sysvars_tracker::vars_list::parse_var_list(THD *thd, LEX_STRING var_list, bool throw_error, - CHARSET_INFO *char_set, - bool take_mutex) + CHARSET_INFO *char_set) { const char separator= ','; char *token, *lasts= NULL; size_t rest= var_list.length; if (!var_list.str || var_list.length == 0) - { - buffer_length= 1; return false; - } if(!strcmp(var_list.str, "*")) { track_all= true; - buffer_length= 2; return false; } - buffer_length= var_list.length + 1; token= var_list.str; track_all= false; @@ -150,8 +141,7 @@ bool Session_sysvars_tracker::vars_list::parse_var_list(THD *thd, token value. Hence the mutex is handled here to avoid a performance overhead. */ - if (!thd || take_mutex) - mysql_mutex_lock(&LOCK_plugin); + mysql_mutex_lock(&LOCK_plugin); for (;;) { sys_var *svar; @@ -196,14 +186,12 @@ bool Session_sysvars_tracker::vars_list::parse_var_list(THD *thd, else break; } - if (!thd || take_mutex) - mysql_mutex_unlock(&LOCK_plugin); + mysql_mutex_unlock(&LOCK_plugin); return false; error: - if (!thd || take_mutex) - mysql_mutex_unlock(&LOCK_plugin); + mysql_mutex_unlock(&LOCK_plugin); return true; } @@ -361,6 +349,25 @@ bool Session_sysvars_tracker::vars_list::construct_var_list(char *buf, return false; } + +void Session_sysvars_tracker::init(THD *thd) +{ + DBUG_ASSERT(thd->variables.session_track_system_variables == + global_system_variables.session_track_system_variables); + DBUG_ASSERT(global_system_variables.session_track_system_variables); + thd->variables.session_track_system_variables= + my_strdup(global_system_variables.session_track_system_variables, + MYF(MY_WME | MY_THREAD_SPECIFIC)); +} + + +void Session_sysvars_tracker::deinit(THD *thd) +{ + my_free(thd->variables.session_track_system_variables); + thd->variables.session_track_system_variables= 0; +} + + /** Enable session tracker by parsing global value of tracked variables. @@ -372,21 +379,16 @@ bool Session_sysvars_tracker::vars_list::construct_var_list(char *buf, bool Session_sysvars_tracker::enable(THD *thd) { + LEX_STRING tmp= { thd->variables.session_track_system_variables, + safe_strlen(thd->variables.session_track_system_variables) }; orig_list.reinit(); - mysql_mutex_lock(&LOCK_plugin); - LEX_STRING tmp; - tmp.str= global_system_variables.session_track_system_variables; - tmp.length= safe_strlen(tmp.str); - if (orig_list.parse_var_list(thd, tmp, true, thd->charset(), false) == true) + if (orig_list.parse_var_list(thd, tmp, true, thd->charset()) == true) { - mysql_mutex_unlock(&LOCK_plugin); orig_list.reinit(); m_enabled= false; return true; } - mysql_mutex_unlock(&LOCK_plugin); m_enabled= true; - return false; } @@ -412,10 +414,28 @@ bool Session_sysvars_tracker::enable(THD *thd) bool Session_sysvars_tracker::update(THD *thd, set_var *var) { vars_list tool_list; - if (tool_list.parse_var_list(thd, var->save_result.string_value, true, - thd->charset(), true)) + void *copy= var->save_result.string_value.str ? + my_memdup(var->save_result.string_value.str, + var->save_result.string_value.length + 1, + MYF(MY_WME | MY_THREAD_SPECIFIC)) : + my_strdup("", MYF(MY_WME | MY_THREAD_SPECIFIC)); + + if (!copy) return true; + + if (tool_list.parse_var_list(thd, var->save_result.string_value, true, + thd->charset())) + { + my_free(copy); + return true; + } + + my_free(thd->variables.session_track_system_variables); + thd->variables.session_track_system_variables= static_cast(copy); + orig_list.copy(&tool_list, thd); + orig_list.construct_var_list(thd->variables.session_track_system_variables, + var->save_result.string_value.length + 1); return false; } @@ -562,7 +582,7 @@ bool sysvartrack_global_update(THD *thd, char *str, size_t len) { LEX_STRING tmp= { str, len }; Session_sysvars_tracker::vars_list dummy; - if (!dummy.parse_var_list(thd, tmp, false, system_charset_info, false)) + if (!dummy.parse_var_list(thd, tmp, false, system_charset_info)) { dummy.construct_var_list(str, len + 1); return false; @@ -571,27 +591,12 @@ bool sysvartrack_global_update(THD *thd, char *str, size_t len) } -uchar *sysvartrack_session_value_ptr(THD *thd, const LEX_CSTRING *base) -{ - Session_sysvars_tracker *tracker= static_cast - (thd->session_tracker.get_tracker(SESSION_SYSVARS_TRACKER)); - size_t len= tracker->get_buffer_length(); - char *res= (char*) thd->alloc(len + sizeof(char*)); - if (res) - { - char *buf= res + sizeof(char*); - *((char**) res)= buf; - tracker->construct_var_list(buf, len); - } - return (uchar*) res; -} - - int session_tracker_init() { + DBUG_ASSERT(global_system_variables.session_track_system_variables); if (sysvartrack_validate_value(0, global_system_variables.session_track_system_variables, - safe_strlen(global_system_variables.session_track_system_variables))) + strlen(global_system_variables.session_track_system_variables))) { sql_print_error("The variable session_track_system_variables has " "invalid values."); diff --git a/sql/session_tracker.h b/sql/session_tracker.h index 51e32dde639..55c78a75978 100644 --- a/sql/session_tracker.h +++ b/sql/session_tracker.h @@ -131,8 +131,6 @@ class Session_sysvars_tracker: public State_tracker user. */ HASH m_registered_sysvars; - /** Size of buffer for string representation */ - size_t buffer_length; /** If TRUE then we want to check all session variable. */ @@ -165,15 +163,9 @@ class Session_sysvars_tracker: public State_tracker my_hash_element(&m_registered_sysvars, i)); } public: - vars_list(): buffer_length(0), track_all(false) { init(); } + vars_list(): track_all(false) { init(); } void deinit() { free_hash(); } - size_t get_buffer_length() - { - DBUG_ASSERT(buffer_length != 0); // asked earlier then should - return buffer_length; - } - sysvar_node_st *insert_or_search(const sys_var *svar) { sysvar_node_st *res= search(svar); @@ -197,7 +189,7 @@ class Session_sysvars_tracker: public State_tracker } void copy(vars_list* from, THD *thd); bool parse_var_list(THD *thd, LEX_STRING var_list, bool throw_error, - CHARSET_INFO *char_set, bool take_mutex); + CHARSET_INFO *char_set); bool construct_var_list(char *buf, size_t buf_len); bool store(THD *thd, String *buf); }; @@ -208,15 +200,8 @@ class Session_sysvars_tracker: public State_tracker vars_list orig_list; public: - size_t get_buffer_length() - { - return orig_list.get_buffer_length(); - } - bool construct_var_list(char *buf, size_t buf_len) - { - return orig_list.construct_var_list(buf, buf_len); - } - + void init(THD *thd); + void deinit(THD *thd); bool enable(THD *thd); bool update(THD *thd, set_var *var); bool store(THD *thd, String *buf); @@ -232,7 +217,6 @@ public: bool sysvartrack_validate_value(THD *thd, const char *str, size_t len); bool sysvartrack_global_update(THD *thd, char *str, size_t len); -uchar *sysvartrack_session_value_ptr(THD *thd, const LEX_CSTRING *base); /** @@ -444,12 +428,6 @@ public: m_trackers[i]->enable(thd); } - /** Returns the pointer to the tracker object for the specified tracker. */ - inline State_tracker *get_tracker(enum_session_tracker tracker) const - { - return m_trackers[tracker]; - } - inline void mark_as_changed(THD *thd, enum enum_session_tracker tracker, LEX_CSTRING *data) { diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc index b2ceb1627a1..d0682ecb151 100644 --- a/sql/sql_plugin.cc +++ b/sql/sql_plugin.cc @@ -3149,6 +3149,11 @@ void plugin_thdvar_init(THD *thd) thd->variables.enforced_table_plugin= NULL; cleanup_variables(&thd->variables); + /* This and all other variable cleanups are here for COM_CHANGE_USER :( */ +#ifndef EMBEDDED_LIBRARY + thd->session_tracker.sysvars.deinit(thd); +#endif + thd->variables= global_system_variables; /* we are going to allocate these lazily */ @@ -3170,6 +3175,9 @@ void plugin_thdvar_init(THD *thd) intern_plugin_unlock(NULL, old_enforced_table_plugin); mysql_mutex_unlock(&LOCK_plugin); +#ifndef EMBEDDED_LIBRARY + thd->session_tracker.sysvars.init(thd); +#endif DBUG_VOID_RETURN; } @@ -3235,6 +3243,10 @@ void plugin_thdvar_cleanup(THD *thd) plugin_ref *list; DBUG_ENTER("plugin_thdvar_cleanup"); +#ifndef EMBEDDED_LIBRARY + thd->session_tracker.sysvars.deinit(thd); +#endif + mysql_mutex_lock(&LOCK_plugin); unlock_variables(thd, &thd->variables); diff --git a/sql/sys_vars.ic b/sql/sys_vars.ic index 24fdc2d04a4..d8d95046cc2 100644 --- a/sql/sys_vars.ic +++ b/sql/sys_vars.ic @@ -641,8 +641,6 @@ public: DBUG_ASSERT(res == 0); } } - uchar *session_value_ptr(THD *thd, const LEX_CSTRING *base) - { return sysvartrack_session_value_ptr(thd, base); } }; #endif //EMBEDDED_LIBRARY