A fix for Bug#19022 "Memory bug when switching db during trigger execution".

No test case as the bug is in an existing test case (rpl_trigger.test
when it is run under valgrind).
The warning was caused by memory corruption in replication slave: thd->db
was pointing at a stack address that was previously used by 
sp_head::execute()::old_db. This happened because mysql_change_db
behaved differently in replication slave and did not make a copy of the 
argument to assign to thd->db. 
The solution is to always free the old value of thd->db and allocate a new
copy, regardless whether we're running in a replication slave or not.

sql/log_event.cc

@@ -1635,14 +1635,33 @@ void Query_log_event::print(FILE* file, PRINT_EVENT_INFO* print_event_info)
 */
 
 #if defined(HAVE_REPLICATION) && !defined(MYSQL_CLIENT)
+
+static const char *rewrite_db(const char *db)
+{
+  if (replicate_rewrite_db.is_empty() || db == NULL)
+    return db;
+  I_List_iterator<i_string_pair> it(replicate_rewrite_db);
+  i_string_pair* tmp;
+
+  while ((tmp=it++))
+  {
+    if (strcmp(tmp->key, db) == 0)
+      return tmp->val;
+  }
+  return db;
+}
+
+
 int Query_log_event::exec_event(struct st_relay_log_info* rli)
 {
   return exec_event(rli, query, q_len);
 }
 
 
-int Query_log_event::exec_event(struct st_relay_log_info* rli, const char *query_arg, uint32 q_len_arg)
+int Query_log_event::exec_event(struct st_relay_log_info* rli,
+                                const char *query_arg, uint32 q_len_arg)
 {
+  const char *new_db= rewrite_db(db);
   int expected_error,actual_error= 0;
   /*
     Colleagues: please never free(thd->catalog) in MySQL. This would lead to
@@ -1651,8 +1670,7 @@ int Query_log_event::exec_event(struct st_relay_log_info* rli, const char *query
     Thank you.
   */
   thd->catalog= catalog_len ? (char *) catalog : (char *)"";
-  thd->db_length= db_len;
-  thd->db= (char*) rewrite_db(db, &thd->db_length);
+  thd->set_db(new_db, strlen(new_db));          /* allocates a copy of 'db' */
   thd->variables.auto_increment_increment= auto_increment_increment;
   thd->variables.auto_increment_offset=    auto_increment_offset;
 
@@ -1869,7 +1887,7 @@ end:
     TABLE uses the db.table syntax.
   */
   thd->catalog= 0;
-  thd->reset_db(NULL, 0);               // prevent db from being freed
+  thd->set_db(NULL, 0);                 /* will free the current database */
   thd->query= 0;			// just to be sure
   thd->query_length= 0;
   VOID(pthread_mutex_unlock(&LOCK_thread_count));
@@ -2817,8 +2835,8 @@ void Load_log_event::set_fields(const char* affected_db,
 int Load_log_event::exec_event(NET* net, struct st_relay_log_info* rli, 
 			       bool use_rli_only_for_errors)
 {
-  thd->db_length= db_len;
-  thd->db= (char*) rewrite_db(db, &thd->db_length);
+  const char *new_db= rewrite_db(db);
+  thd->set_db(new_db, strlen(new_db));
   DBUG_ASSERT(thd->query == 0);
   thd->query_length= 0;                         // Should not be needed
   thd->query_error= 0;
@@ -3018,7 +3036,7 @@ error:
   const char *remember_db= thd->db;
   VOID(pthread_mutex_lock(&LOCK_thread_count));
   thd->catalog= 0;
-  thd->reset_db(NULL, 0);
+  thd->set_db(NULL, 0);                   /* will free the current database */
   thd->query= 0;
   thd->query_length= 0;
   VOID(pthread_mutex_unlock(&LOCK_thread_count));

sql/slave.cc

@@ -1177,24 +1177,6 @@ bool net_request_file(NET* net, const char* fname)
 }
 
 
-const char *rewrite_db(const char* db, uint *new_len)
-{
-  if (replicate_rewrite_db.is_empty() || !db)
-    return db;
-  I_List_iterator<i_string_pair> it(replicate_rewrite_db);
-  i_string_pair* tmp;
-
-  while ((tmp=it++))
-  {
-    if (!strcmp(tmp->key, db))
-    {
-      *new_len= (uint32)strlen(tmp->val);
-      return tmp->val;
-    }
-  }
-  return db;
-}
-
 /*
   From other comments and tests in code, it looks like
   sometimes Query_log_event and Load_log_event can have db == 0

sql/slave.h

@@ -550,7 +550,6 @@ int add_table_rule(HASH* h, const char* table_spec);
 int add_wild_table_rule(DYNAMIC_ARRAY* a, const char* table_spec);
 void init_table_rule_hash(HASH* h, bool* h_inited);
 void init_table_rule_array(DYNAMIC_ARRAY* a, bool* a_inited);
-const char *rewrite_db(const char* db, uint *new_db_len);
 const char *print_slave_db_safe(const char *db);
 int check_expected_error(THD* thd, RELAY_LOG_INFO* rli, int error_code);
 void skip_load_data_infile(NET* net);

sql/sql_class.h

@@ -1574,21 +1574,23 @@ public:
 
   /*
     Initialize the current database from a NULL-terminated string with length
+    If we run out of memory, we free the current database and return TRUE.
+    This way the user will notice the error as there will be no current
+    database selected (in addition to the error message set by malloc).
   */
-  void set_db(const char *new_db, uint new_db_len)
-  {
-    if (new_db)
+  bool set_db(const char *new_db, uint new_db_len)
   {
     /* Do not reallocate memory if current chunk is big enough. */
-      if (db && db_length >= new_db_len)
+    if (db && new_db && db_length >= new_db_len)
       memcpy(db, new_db, new_db_len+1);
     else
     {
-        safeFree(db);
-        db= my_strdup_with_length(new_db, new_db_len, MYF(MY_WME));
-      }
-      db_length= db ? new_db_len: 0;
+      x_free(db);
+      db= new_db ? my_strdup_with_length(new_db, new_db_len, MYF(MY_WME)) :
+                   NULL;
     }
+    db_length= db ? new_db_len : 0;
+    return new_db && !db;
   }
   void reset_db(char *new_db, uint new_db_len)
   {

sql/sql_db.cc

@@ -781,32 +781,13 @@ bool mysql_rm_db(THD *thd,char *db,bool if_exists, bool silent)
 exit:
   (void)sp_drop_db_routines(thd, db); /* QQ Ignore errors for now  */
   /*
-    If this database was the client's selected database, we silently change the
-    client's selected database to nothing (to have an empty SELECT DATABASE()
-    in the future). For this we free() thd->db and set it to 0. But we don't do
-    free() for the slave thread. Indeed, doing a x_free() on it leads to nasty
-    problems (i.e. long painful debugging) because in this thread, thd->db is
-    the same as data_buf and db of the Query_log_event which is dropping the
-    database. So if you free() thd->db, you're freeing data_buf. You set
-    thd->db to 0 but not data_buf (thd->db and data_buf are two distinct
-    pointers which point to the same place). Then in ~Query_log_event(), we
-    have 'if (data_buf) free(data_buf)' data_buf is !=0 so this makes a
-    DOUBLE free().
-    Side effects of this double free() are, randomly (depends on the machine),
-    when the slave is replicating a DROP DATABASE:
-    - garbage characters in the error message:
-    "Error 'Can't drop database 'test2'; database doesn't exist' on query
-    'h4zI'"
-    - segfault
-    - hang in "free(vio)" (yes!) in the I/O or SQL slave threads (so slave
-    server hangs at shutdown etc).
+    If this database was the client's selected database, we silently
+    change the client's selected database to nothing (to have an empty
+    SELECT DATABASE() in the future). For this we free() thd->db and set
+    it to 0.
   */
   if (thd->db && !strcmp(thd->db, db))
-  {
-    if (!(thd->slave_thread)) /* a slave thread will free it itself */
-      x_free(thd->db);
-    thd->reset_db(NULL, 0);
-  }
+    thd->set_db(NULL, 0);
   VOID(pthread_mutex_unlock(&LOCK_mysql_create_db));
   start_waiting_global_read_lock(thd);
 exit2:
@@ -1099,38 +1080,52 @@ err:
 
 
 /*
-  Change default database.
+  Change the current database.
 
   SYNOPSIS
     mysql_change_db()
-    thd			Thread handler
-    name		Databasename
-    no_access_check	True don't do access check. In this case name may be ""
+    thd			thread handle
+    name		database name
+    no_access_check	if TRUE, don't do access check. In this
+                        case name may be ""
 
   DESCRIPTION
-    Becasue the database name may have been given directly from the
-    communication packet (in case of 'connect' or 'COM_INIT_DB')
-    we have to do end space removal in this function.
+    Check that the database name corresponds to a valid and
+    existent database, check access rights (unless called with
+    no_access_check), and set the current database. This function
+    is called to change the current database upon user request
+    (COM_CHANGE_DB command) or temporarily, to execute a stored
+    routine.
 
   NOTES
-    Do as little as possible in this function, as it is not called for the
-    replication slave SQL thread (for that thread, setting of thd->db is done
-    in ::exec_event() methods of log_event.cc).
-
-    This function does not send anything, including error messages to the
-    client, if that should be sent to the client, call net_send_error after
-    this function.
+    This function is not the only way to switch the database that
+    is currently employed. When the replication slave thread
+    switches the database before executing a query, it calls
+    thd->set_db directly. However, if the query, in turn, uses
+    a stored routine, the stored routine will use this function,
+    even if it's run on the slave.
+
+    This function allocates the name of the database on the system
+    heap: this is necessary to be able to uniformly change the
+    database from any module of the server. Up to 5.0 different
+    modules were using different memory to store the name of the
+    database, and this led to memory corruption: a stack pointer
+    set by Stored Procedures was used by replication after the
+    stack address was long gone.
+
+    This function does not send anything, including error
+    messages, to the client. If that should be sent to the client,
+    call net_send_error after this function.
 
   RETURN VALUES
-    0	ok
+    0	OK
     1	error
 */
 
 bool mysql_change_db(THD *thd, const char *name, bool no_access_check)
 {
-  int length, db_length;
-  char *dbname= thd->slave_thread ? (char *) name :
-                                    my_strdup((char *) name, MYF(MY_WME));
+  int path_length, db_length;
+  char *db_name;
   char	path[FN_REFLEN];
   HA_CREATE_INFO create;
   bool system_db= 0;
@@ -1142,32 +1137,35 @@ bool mysql_change_db(THD *thd, const char *name, bool no_access_check)
   DBUG_ENTER("mysql_change_db");
   DBUG_PRINT("enter",("name: '%s'",name));
 
-  LINT_INIT(db_length);
-
-  /* dbname can only be NULL if malloc failed */
-  if (!dbname || !(db_length= strlen(dbname)))
+  if (name == NULL || name[0] == '\0' && no_access_check == FALSE)
   {
-    if (no_access_check && dbname)
+    my_message(ER_NO_DB_ERROR, ER(ER_NO_DB_ERROR), MYF(0));
+    DBUG_RETURN(1);				/* purecov: inspected */
+  }
+  else if (name[0] == '\0')
   {
-      /* Called from SP when orignal database was not set */
+    /* Called from SP to restore the original database, which was NULL */
+    DBUG_ASSERT(no_access_check);
     system_db= 1;
+    db_name= NULL;
+    db_length= 0;
     goto end;
   }
-    if (!(thd->slave_thread))
-      x_free(dbname);				/* purecov: inspected */
-    my_message(ER_NO_DB_ERROR, ER(ER_NO_DB_ERROR),
-               MYF(0));                         /* purecov: inspected */
-    DBUG_RETURN(1);				/* purecov: inspected */
-  }
-  if (check_db_name(dbname))
+  /*
+    Now we need to make a copy because check_db_name requires a
+    non-constant argument. TODO: fix check_db_name.
+  */
+  if ((db_name= my_strdup(name, MYF(MY_WME))) == NULL)
+    DBUG_RETURN(1);                             /* the error is set */
+  db_length= strlen(db_name);
+  if (check_db_name(db_name))
   {
-    my_error(ER_WRONG_DB_NAME, MYF(0), dbname);
-    if (!(thd->slave_thread))
-      my_free(dbname, MYF(0));
+    my_error(ER_WRONG_DB_NAME, MYF(0), db_name);
+    my_free(db_name, MYF(0));
     DBUG_RETURN(1);
   }
-  DBUG_PRINT("info",("Use database: %s", dbname));
-  if (!my_strcasecmp(system_charset_info, dbname, information_schema_name.str))
+  DBUG_PRINT("info",("Use database: %s", db_name));
+  if (!my_strcasecmp(system_charset_info, db_name, information_schema_name.str))
   {
     system_db= 1;
 #ifndef NO_EMBEDDED_ACCESS_CHECKS
@@ -1182,45 +1180,36 @@ bool mysql_change_db(THD *thd, const char *name, bool no_access_check)
     if (test_all_bits(sctx->master_access, DB_ACLS))
       db_access=DB_ACLS;
     else
-      db_access= (acl_get(sctx->host, sctx->ip, sctx->priv_user, dbname, 0) |
+      db_access= (acl_get(sctx->host, sctx->ip, sctx->priv_user, db_name, 0) |
                   sctx->master_access);
     if (!(db_access & DB_ACLS) && (!grant_option ||
-                                   check_grant_db(thd,dbname)))
+                                   check_grant_db(thd,db_name)))
     {
       my_error(ER_DBACCESS_DENIED_ERROR, MYF(0),
                sctx->priv_user,
                sctx->priv_host,
-               dbname);
+               db_name);
       mysql_log.write(thd, COM_INIT_DB, ER(ER_DBACCESS_DENIED_ERROR),
-                      sctx->priv_user, sctx->priv_host, dbname);
-      if (!(thd->slave_thread))
-        my_free(dbname,MYF(0));
+                      sctx->priv_user, sctx->priv_host, db_name);
+      my_free(db_name, MYF(0));
       DBUG_RETURN(1);
     }
   }
 #endif
-  (void) sprintf(path,"%s/%s",mysql_data_home,dbname);
-  length=unpack_dirname(path,path);		// Convert if not unix
-  if (length && path[length-1] == FN_LIBCHAR)
-    path[length-1]=0;				// remove ending '\'
+  (void) sprintf(path,"%s/%s", mysql_data_home, db_name);
+  path_length= unpack_dirname(path, path);      // Convert if not UNIX
+  if (path_length && path[path_length-1] == FN_LIBCHAR)
+    path[path_length-1]= '\0';                  // remove ending '\'
   if (my_access(path,F_OK))
   {
-    my_error(ER_BAD_DB_ERROR, MYF(0), dbname);
-    if (!(thd->slave_thread))
-      my_free(dbname,MYF(0));
+    my_error(ER_BAD_DB_ERROR, MYF(0), db_name);
+    my_free(db_name, MYF(0));
     DBUG_RETURN(1);
   }
 end:
-  if (!(thd->slave_thread))
   x_free(thd->db);
-  if (dbname && dbname[0] == 0)
-  {
-    if (!(thd->slave_thread))
-      my_free(dbname, MYF(0));
-    thd->reset_db(NULL, 0);
-  }
-  else
-    thd->reset_db(dbname, db_length);          // THD::~THD will free this
+  DBUG_ASSERT(db_name == NULL || db_name[0] != '\0');
+  thd->reset_db(db_name, db_length);            // THD::~THD will free this
 #ifndef NO_EMBEDDED_ACCESS_CHECKS
   if (!no_access_check)
     sctx->db_access= db_access;