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

@@ -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;
+  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(), true))
+                               thd->charset()))
+  {
+    my_free(copy);
     return true;
+  }
+
+  my_free(thd->variables.session_track_system_variables);
+  thd->variables.session_track_system_variables= static_cast<char*>(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<Session_sysvars_tracker*>
-    (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.");

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 @@ class Session_sysvars_tracker: public State_tracker
 
 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 @@ class Session_tracker
       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)
   {

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

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