Bug #38816: kill + flush tables with read lock + stored

            procedures causes crashes!

The problem of that bugreport was mostly fixed by the
patch for bug 38691.
However, attached test case focused on another crash or
valgrind warning problem: SHOW PROCESSLIST query accesses
freed memory of SP instruction that run in a parallel
connection.

Changes of thd->query/thd->query_length in dangerous
places have been guarded with the per-thread
LOCK_thd_data mutex (the THD::LOCK_delete mutex has been
renamed to THD::LOCK_thd_data).

sql/ha_myisam.cc

@@ -1487,10 +1487,8 @@ bool ha_myisam::check_and_repair(THD *thd)
 
   old_query= thd->query;
   old_query_length= thd->query_length;
-  pthread_mutex_lock(&LOCK_thread_count);
-  thd->query= (char*) table->s->table_name;
-  thd->query_length= (uint32) strlen(table->s->table_name);
-  pthread_mutex_unlock(&LOCK_thread_count);
+  thd->set_query((char*) table->s->table_name,
+                 (uint32) strlen(table->s->table_name));
 
   if ((marked_crashed= mi_is_crashed(file)) || check(thd, &check_opt))
   {
@@ -1503,10 +1501,7 @@ bool ha_myisam::check_and_repair(THD *thd)
     if (repair(thd, &check_opt))
       error=1;
   }
-  pthread_mutex_lock(&LOCK_thread_count);
-  thd->query= old_query;
-  thd->query_length= old_query_length;
-  pthread_mutex_unlock(&LOCK_thread_count);
+  thd->set_query(old_query, old_query_length);
   DBUG_RETURN(error);
 }
 

sql/log_event.cc

@@ -1960,8 +1960,7 @@ int Query_log_event::exec_event(struct st_relay_log_info* rli,
       db_ok(thd->db, replicate_do_db, replicate_ignore_db))
   {
     thd->set_time((time_t)when);
-    thd->query_length= q_len_arg;
-    thd->query= (char*)query_arg;
+    thd->set_query((char*)query_arg, q_len_arg);
     VOID(pthread_mutex_lock(&LOCK_thread_count));
     thd->query_id = next_query_id();
     VOID(pthread_mutex_unlock(&LOCK_thread_count));
@@ -2164,7 +2163,6 @@ Default database: '%s'. Query: '%s'",
   } /* End of if (db_ok(... */
 
 end:
-  VOID(pthread_mutex_lock(&LOCK_thread_count));
   /*
     Probably we have set thd->query, thd->db, thd->catalog to point to places
     in the data_buf of this event. Now the event is going to be deleted
@@ -2177,10 +2175,8 @@ end:
   */
   thd->catalog= 0;
   thd->set_db(NULL, 0);                 /* will free the current database */
+  thd->set_query(NULL, 0);
   DBUG_PRINT("info", ("end: query= 0"));
-  thd->query= 0;			// just to be sure
-  thd->query_length= 0;
-  VOID(pthread_mutex_unlock(&LOCK_thread_count));
   close_thread_tables(thd);      
   free_root(thd->mem_root,MYF(MY_KEEP_PREALLOC));
   /*
@@ -3259,8 +3255,7 @@ int Load_log_event::exec_event(NET* net, struct st_relay_log_info* rli,
       print_query(FALSE, load_data_query, &end, (char **)&thd->lex->fname_start,
                   (char **)&thd->lex->fname_end);
       *end= 0;
-      thd->query_length= (uint) (end - load_data_query);
-      thd->query= load_data_query;
+      thd->set_query(load_data_query, (uint) (end - load_data_query));
 
       if (sql_ex.opt_flags & REPLACE_FLAG)
       {
@@ -3366,12 +3361,9 @@ int Load_log_event::exec_event(NET* net, struct st_relay_log_info* rli,
 error:
   thd->net.vio = 0; 
   const char *remember_db= thd->db;
-  VOID(pthread_mutex_lock(&LOCK_thread_count));
   thd->catalog= 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));
+  thd->set_query(NULL, 0);
   close_thread_tables(thd);
   if (thd->query_error)
   {

sql/slave.cc

@@ -755,7 +755,7 @@ int terminate_slave_thread(THD* thd,
     int error;
     DBUG_PRINT("loop", ("killing slave thread"));
 
-    pthread_mutex_lock(&thd->LOCK_delete);
+    pthread_mutex_lock(&thd->LOCK_thd_data);
 #ifndef DONT_USE_THR_ALARM
     /*
       Error codes from pthread_kill are:
@@ -766,7 +766,7 @@ int terminate_slave_thread(THD* thd,
     DBUG_ASSERT(err != EINVAL);
 #endif
     thd->awake(THD::NOT_KILLED);
-    pthread_mutex_unlock(&thd->LOCK_delete);
+    pthread_mutex_unlock(&thd->LOCK_thd_data);
 
     /*
       There is a small chance that slave thread might miss the first
@@ -1608,15 +1608,13 @@ static int create_table_from_dump(THD* thd, MYSQL *mysql, const char* db,
     DBUG_RETURN(1);
   }
   thd->command = COM_TABLE_DUMP;
-  thd->query_length= packet_len;
-  /* Note that we should not set thd->query until the area is initalized */
   if (!(query = thd->strmake((char*) net->read_pos, packet_len)))
   {
     sql_print_error("create_table_from_dump: out of memory");
     my_message(ER_GET_ERRNO, "Out of memory", MYF(0));
     DBUG_RETURN(1);
   }
-  thd->query= query;
+  thd->set_query(query, packet_len);
   thd->query_error = 0;
   thd->net.no_send_ok = 1;
 
@@ -3867,11 +3865,8 @@ err:
   // print the current replication position
   sql_print_information("Slave I/O thread exiting, read up to log '%s', position %s",
 		  IO_RPL_LOG_NAME, llstr(mi->master_log_pos,llbuff));
-  VOID(pthread_mutex_lock(&LOCK_thread_count));
-  thd->query= 0; // extra safety
-  thd->query_length= 0;
+  thd->set_query(NULL, 0);
   thd->reset_db(NULL, 0);
-  VOID(pthread_mutex_unlock(&LOCK_thread_count));
   if (mysql)
   {
     /*
@@ -4105,17 +4100,14 @@ the slave SQL thread with \"SLAVE START\". We stopped at log \
 		        RPL_LOG_NAME, llstr(rli->group_master_log_pos,llbuff));
 
  err:
-  VOID(pthread_mutex_lock(&LOCK_thread_count));
   /*
     Some extra safety, which should not been needed (normally, event deletion
     should already have done these assignments (each event which sets these
     variables is supposed to set them to 0 before terminating)).
   */
-  thd->catalog= 0; 
+  thd->catalog= 0;
+  thd->set_query(NULL, 0);
   thd->reset_db(NULL, 0);
-  thd->query= 0; 
-  thd->query_length= 0;
-  VOID(pthread_mutex_unlock(&LOCK_thread_count));
   thd_proc_info(thd, "Waiting for slave mutex on exit");
   pthread_mutex_lock(&rli->run_lock);
   /* We need data_lock, at least to wake up any waiting master_pos_wait() */

sql/sp_head.cc

@@ -949,8 +949,7 @@ subst_spvars(THD *thd, sp_instr *instr, LEX_STRING *query_str)
   else
     DBUG_RETURN(TRUE);
 
-  thd->query= pbuf;
-  thd->query_length= qbuf.length();
+  thd->set_query(pbuf, qbuf.length());
 
   DBUG_RETURN(FALSE);
 }
@@ -2654,8 +2653,7 @@ sp_instr_stmt::execute(THD *thd, uint *nextp)
     }
     else
       *nextp= m_ip+1;
-    thd->query= query;
-    thd->query_length= query_length;
+    thd->set_query(query, query_length);
     thd->query_name_consts= 0;
   }
   DBUG_RETURN(res);

sql/sql_class.cc

@@ -259,7 +259,7 @@ THD::THD()
 #ifdef SIGNAL_WITH_VIO_CLOSE
   active_vio = 0;
 #endif
-  pthread_mutex_init(&LOCK_delete, MY_MUTEX_INIT_FAST);
+  pthread_mutex_init(&LOCK_thd_data, MY_MUTEX_INIT_FAST);
 
   /* Variables with default values */
   proc_info="login";
@@ -486,8 +486,8 @@ THD::~THD()
   THD_CHECK_SENTRY(this);
   DBUG_ENTER("~THD()");
   /* Ensure that no one is using THD */
-  pthread_mutex_lock(&LOCK_delete);
-  pthread_mutex_unlock(&LOCK_delete);
+  pthread_mutex_lock(&LOCK_thd_data);
+  pthread_mutex_unlock(&LOCK_thd_data);
   add_to_status(&global_status_var, &status_var);
 
   /* Close connection */
@@ -513,7 +513,7 @@ THD::~THD()
   free_root(&transaction.mem_root,MYF(0));
 #endif
   mysys_var=0;					// Safety (shouldn't be needed)
-  pthread_mutex_destroy(&LOCK_delete);
+  pthread_mutex_destroy(&LOCK_thd_data);
 #ifndef DBUG_OFF
   dbug_sentry= THD_SENTRY_GONE;
 #endif  
@@ -551,7 +551,7 @@ void add_to_status(STATUS_VAR *to_var, STATUS_VAR *from_var)
 void THD::awake(THD::killed_state state_to_set)
 {
   THD_CHECK_SENTRY(this);
-  safe_mutex_assert_owner(&LOCK_delete); 
+  safe_mutex_assert_owner(&LOCK_thd_data);
 
   killed= state_to_set;
   if (state_to_set != THD::KILL_QUERY)
@@ -895,7 +895,7 @@ int THD::send_explain_fields(select_result *result)
 void THD::close_active_vio()
 {
   DBUG_ENTER("close_active_vio");
-  safe_mutex_assert_owner(&LOCK_delete); 
+  safe_mutex_assert_owner(&LOCK_thd_data);
 #ifndef EMBEDDED_LIBRARY
   if (active_vio)
   {
@@ -2323,6 +2323,25 @@ void THD::restore_sub_statement_state(Sub_statement_state *backup)
 }
 
 
+void THD::set_statement(Statement *stmt)
+{
+  pthread_mutex_lock(&LOCK_thd_data);
+  Statement::set_statement(stmt);
+  pthread_mutex_unlock(&LOCK_thd_data);
+}
+
+
+/** Assign a new value to thd->query.  */
+
+void THD::set_query(char *query_arg, uint32 query_length_arg)
+{
+  pthread_mutex_lock(&LOCK_thd_data);
+  query= query_arg;
+  query_length= query_length_arg;
+  pthread_mutex_unlock(&LOCK_thd_data);
+}
+
+
 /**
   Mark transaction to rollback and mark error as fatal to a sub-statement.
 

sql/sql_class.h

@@ -835,22 +835,16 @@ public:
     we need to declare it char * because all table handlers are written
     in C and need to point to it.
 
-    Note that (A) if we set query = NULL, we must at the same time set
-    query_length = 0, and protect the whole operation with the
-    LOCK_thread_count mutex. And (B) we are ONLY allowed to set query to a
-    non-NULL value if its previous value is NULL. We do not need to protect
-    operation (B) with any mutex. To avoid crashes in races, if we do not
-    know that thd->query cannot change at the moment, one should print
+    Note that if we set query = NULL, we must at the same time set
+    query_length = 0, and protect the whole operation with
+    LOCK_thd_data mutex. To avoid crashes in races, if we do not
+    know that thd->query cannot change at the moment, we should print
     thd->query like this:
-      (1) reserve the LOCK_thread_count mutex;
-      (2) check if thd->query is NULL;
-      (3) if not NULL, then print at most thd->query_length characters from
-      it. We will see the query_length field as either 0, or the right value
-      for it.
-    Assuming that the write and read of an n-bit memory field in an n-bit
-    computer is atomic, we can avoid races in the above way.
-    This printing is needed at least in SHOW PROCESSLIST and SHOW INNODB
-    STATUS.
+      (1) reserve the LOCK_thd_data mutex;
+      (2) print or copy the value of query and query_length
+      (3) release LOCK_thd_data mutex.
+    This printing is needed at least in SHOW PROCESSLIST and SHOW
+    ENGINE INNODB STATUS.
   */
   char *query;
   uint32 query_length;                          // current query length
@@ -866,7 +860,7 @@ public:
   virtual ~Statement();
 
   /* Assign execution context (note: not all members) of given stmt to self */
-  void set_statement(Statement *stmt);
+  virtual void set_statement(Statement *stmt);
   void set_n_backup_statement(Statement *stmt, Statement *backup);
   void restore_backup_statement(Statement *stmt, Statement *backup);
   /* return class type */
@@ -1229,7 +1223,15 @@ public:
   THR_LOCK_OWNER main_lock_id;          // To use for conventional queries
   THR_LOCK_OWNER *lock_id;              // If not main_lock_id, points to
                                         // the lock_id of a cursor.
-  pthread_mutex_t LOCK_delete;		// Locked before thd is deleted
+  /**
+    Protects THD data accessed from other threads:
+    - thd->query and thd->query_length (used by SHOW ENGINE
+      INNODB STATUS and SHOW PROCESSLIST
+    - thd->mysys_var (used by KILL statement and shutdown).
+    Is locked when THD is deleted.
+  */
+  pthread_mutex_t LOCK_thd_data;
+
   /* all prepared statements and cursors of this connection */
   Statement_map stmt_map;
   /*
@@ -1637,15 +1639,15 @@ public:
 #ifdef SIGNAL_WITH_VIO_CLOSE
   inline void set_active_vio(Vio* vio)
   {
-    pthread_mutex_lock(&LOCK_delete);
+    pthread_mutex_lock(&LOCK_thd_data);
     active_vio = vio;
-    pthread_mutex_unlock(&LOCK_delete);
+    pthread_mutex_unlock(&LOCK_thd_data);
   }
   inline void clear_active_vio()
   {
-    pthread_mutex_lock(&LOCK_delete);
+    pthread_mutex_lock(&LOCK_thd_data);
     active_vio = 0;
-    pthread_mutex_unlock(&LOCK_delete);
+    pthread_mutex_unlock(&LOCK_thd_data);
   }
   void close_active_vio();
 #endif
@@ -1882,6 +1884,14 @@ public:
   */
   void pop_internal_handler();
 
+  /** Overloaded to guard query/query_length fields */
+  virtual void set_statement(Statement *stmt);
+
+  /**
+    Assign a new value to thd->query.
+    Protected with LOCK_thd_data mutex.
+  */
+  void set_query(char *query_arg, uint32 query_length_arg);
 private:
   /** The current internal error handler for this thread, or NULL. */
   Internal_error_handler *m_internal_handler;

sql/sql_insert.cc

@@ -1854,7 +1854,7 @@ bool delayed_get_table(THD *thd, TABLE_LIST *table_list)
       thread_count++;
       pthread_mutex_unlock(&LOCK_thread_count);
       di->thd.set_db(table_list->db, (uint) strlen(table_list->db));
-      di->thd.query= my_strdup(table_list->table_name, MYF(MY_WME));
+      di->thd.set_query(my_strdup(table_list->table_name, MYF(MY_WME)), 0);
       if (di->thd.db == NULL || di->thd.query == NULL)
       {
         /* The error is reported */

sql/sql_parse.cc

@@ -1106,8 +1106,7 @@ void execute_init_command(THD *thd, sys_var_str *init_command_var,
     values of init_command_var can't be changed
   */
   rw_rdlock(var_mutex);
-  thd->query= init_command_var->value;
-  thd->query_length= init_command_var->value_length;
+  thd->set_query(init_command_var->value, init_command_var->value_length);
   save_client_capabilities= thd->client_capabilities;
   thd->client_capabilities|= CLIENT_MULTI_QUERIES;
   /*
@@ -1326,6 +1325,7 @@ pthread_handler_t handle_bootstrap(void *arg)
   thd->init_for_queries();
   while (fgets(buff, thd->net.max_packet, file))
   {
+    char *query;
    ulong length= (ulong) strlen(buff);
    while (buff[length-1] != '\n' && !feof(file))
    {
@@ -1350,10 +1350,9 @@ pthread_handler_t handle_bootstrap(void *arg)
            buff[length-1] == ';'))
       length--;
     buff[length]=0;
-    thd->query_length=length;
-    thd->query= thd->memdup_w_gap(buff, length+1, 
-				  thd->db_length+1+QUERY_CACHE_FLAGS_SIZE);
-    thd->query[length] = '\0';
+    query= thd->memdup_w_gap(buff, length + 1,
+                             thd->db_length + 1 + QUERY_CACHE_FLAGS_SIZE);
+    thd->set_query(query, length);
     DBUG_PRINT("query",("%-.4096s",thd->query));
 #if defined(ENABLED_PROFILING) && defined(COMMUNITY_SERVER)
     thd->profiling.set_query_source(thd->query, length);
@@ -1463,8 +1462,7 @@ int mysql_table_dump(THD* thd, char* db, char* tbl_name)
   if (check_one_table_access(thd, SELECT_ACL, table_list))
     goto err;
   thd->free_list = 0;
-  thd->query_length=(uint) strlen(tbl_name);
-  thd->query = tbl_name;
+  thd->set_query(tbl_name, (uint) strlen(tbl_name));
   if ((error = mysqld_dump_create_info(thd, table_list, -1)))
   {
     my_error(ER_GET_ERRNO, MYF(0), my_errno);
@@ -1987,9 +1985,8 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
       thd->profiling.set_query_source(next_packet, length);
 #endif
 
+      thd->set_query(next_packet, length);
       VOID(pthread_mutex_lock(&LOCK_thread_count));
-      thd->query_length= length;
-      thd->query= next_packet;
       /*
         Count each statement from the client.
       */
@@ -2041,9 +2038,10 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
         table_list.schema_table= schema_table;
     }
 
-    thd->query_length= (uint) strlen(packet);       // for simplicity: don't optimize
-    if (!(thd->query=fields=thd->memdup(packet,thd->query_length+1)))
+    uint query_length= (uint) strlen(packet);
+    if (!(fields= thd->memdup(packet, query_length + 1)))
       break;
+    thd->set_query(fields, query_length);
     mysql_log.write(thd,command,"%s %s",table_list.table_name, fields);
     if (lower_case_table_names)
       my_casedn_str(files_charset_info, table_list.table_name);
@@ -2327,13 +2325,12 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
   log_slow_statement(thd);
 
   thd_proc_info(thd, "cleaning up");
-  VOID(pthread_mutex_lock(&LOCK_thread_count)); // For process list
-  thd_proc_info(thd, 0);
+  thd->set_query(NULL, 0);
   thd->command=COM_SLEEP;
-  thd->query=0;
-  thd->query_length=0;
+  VOID(pthread_mutex_lock(&LOCK_thread_count)); // For process list
   thread_running--;
   VOID(pthread_mutex_unlock(&LOCK_thread_count));
+  thd_proc_info(thd, 0);
   thd->packet.shrink(thd->variables.net_buffer_length);	// Reclaim some memory
   free_root(thd->mem_root,MYF(MY_KEEP_PREALLOC));
   DBUG_RETURN(error);
@@ -2536,6 +2533,7 @@ int prepare_schema_table(THD *thd, LEX *lex, Table_ident *table_ident,
 
 bool alloc_query(THD *thd, const char *packet, uint packet_length)
 {
+  char *query;
   packet_length--;				// Remove end null
   /* Remove garbage at start and end of query */
   while (my_isspace(thd->charset(),packet[0]) && packet_length > 0)
@@ -2551,14 +2549,13 @@ bool alloc_query(THD *thd, const char *packet, uint packet_length)
     packet_length--;
   }
   /* We must allocate some extra memory for query cache */
-  thd->query_length= 0;                        // Extra safety: Avoid races
-  if (!(thd->query= (char*) thd->memdup_w_gap((gptr) (packet),
-					      packet_length,
-					      thd->db_length+ 1 +
-					      QUERY_CACHE_FLAGS_SIZE)))
-    return TRUE;
-  thd->query[packet_length]=0;
-  thd->query_length= packet_length;
+  if (! (query= (char*) thd->memdup_w_gap(packet,
+                                          packet_length,
+                                          1 + thd->db_length +
+                                          QUERY_CACHE_FLAGS_SIZE)))
+      return TRUE;
+  query[packet_length]= '\0';
+  thd->set_query(query, packet_length);
 
   /* Reclaim some memory */
   thd->packet.shrink(thd->variables.net_buffer_length);
@@ -7506,7 +7503,7 @@ void kill_one_thread(THD *thd, ulong id, bool only_kill_query)
   {
     if (tmp->thread_id == id)
     {
-      pthread_mutex_lock(&tmp->LOCK_delete);	// Lock from delete
+      pthread_mutex_lock(&tmp->LOCK_thd_data);	// Lock from delete
       break;
     }
   }
@@ -7539,7 +7536,7 @@ void kill_one_thread(THD *thd, ulong id, bool only_kill_query)
     }
     else
       error=ER_KILL_DENIED_ERROR;
-    pthread_mutex_unlock(&tmp->LOCK_delete);
+    pthread_mutex_unlock(&tmp->LOCK_thd_data);
   }
 
   if (!error)

sql/sql_repl.cc

@@ -1044,7 +1044,7 @@ void kill_zombie_dump_threads(uint32 slave_server_id)
     if (tmp->command == COM_BINLOG_DUMP &&
        tmp->server_id == slave_server_id)
     {
-      pthread_mutex_lock(&tmp->LOCK_delete);	// Lock from delete
+      pthread_mutex_lock(&tmp->LOCK_thd_data);	// Lock from delete
       break;
     }
   }
@@ -1057,7 +1057,7 @@ void kill_zombie_dump_threads(uint32 slave_server_id)
       again. We just to do kill the thread ourselves.
     */
     tmp->awake(THD::KILL_QUERY);
-    pthread_mutex_unlock(&tmp->LOCK_delete);
+    pthread_mutex_unlock(&tmp->LOCK_thd_data);
   }
 }
 

sql/sql_show.cc

@@ -1410,16 +1410,14 @@ void mysqld_list_processes(THD *thd,const char *user, bool verbose)
         thd_info->start_time= tmp->start_time;
 #endif
         thd_info->query=0;
+        /* Lock THD mutex that protects its data when looking at it. */
+        pthread_mutex_lock(&tmp->LOCK_thd_data);
         if (tmp->query)
         {
-	  /* 
-            query_length is always set to 0 when we set query = NULL; see
-	    the comment in sql_class.h why this prevents crashes in possible
-            races with query_length
-          */
           uint length= min(max_query_length, tmp->query_length);
           thd_info->query=(char*) thd->strmake(tmp->query,length);
         }
+        pthread_mutex_unlock(&tmp->LOCK_thd_data);
         thread_infos.append(thd_info);
       }
     }