MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Gleb Shchepa Date:June 10 2009 10:28pm
Subject:bzr commit into mysql-5.0-bugteam branch (gshchepa:2768) Bug#38816
View as plain text  
#At file:///home/uchum/work/bzr/38816-5.0/ based on revid:azundris@stripped

 2768 Gleb Shchepa	2009-06-11
      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 new per-thread
      LOCK_query_string mutex.
     @ sql/ha_myisam.cc
        Bug #38816: kill + flush tables with read lock + stored
                    procedures causes crashes!
        
        Modification of THD::query/query_length has been guarded
        with the a THD::set_query() method call/LOCK_query_string
        mutex.
        Unnecessary locking with the global LOCK_thread_count
        mutex has been removed.
     @ sql/log_event.cc
        Bug #38816: kill + flush tables with read lock + stored
                    procedures causes crashes!
        
        Modification of THD::query/query_length has been guarded
        with the a THD::set_query()/reset_query() method calls/
        LOCK_query_string mutex.
     @ sql/slave.cc
        Bug #38816: kill + flush tables with read lock + stored
                    procedures causes crashes!
        
        Modification of THD::query/query_length has been guarded
        with the a THD::set_query()/reset_query() method calls/
        LOCK_query_string mutex.
     @ sql/sp_head.cc
        Bug #38816: kill + flush tables with read lock + stored
                    procedures causes crashes!
        
        Modification of THD::query/query_length has been guarded
        with the a THD::set_query() method calls/ LOCK_query_string
        mutex.
     @ sql/sql_class.cc
        Bug #38816: kill + flush tables with read lock + stored
                    procedures causes crashes!
        
        The new THD::LOCK_query_string mutex and THD::set_query(),
        THD::memdup_query_w_gap() and THD::reset_query() methods
        have been added to guard modifications of THD::query/
        THD::query_length fields, also the Statement::set_statement()
        method has been overloaded in the THD class.
     @ sql/sql_class.h
        Bug #38816: kill + flush tables with read lock + stored
                    procedures causes crashes!
        
        The new THD::LOCK_query_string mutex and THD::set_query(),
        THD::memdup_query_w_gap() and THD::reset_query() methods
        have been added to guard modifications of THD::query/
        THD::query_length fields, also the Statement::set_statement()
        method has been overloaded in the THD class.
     @ sql/sql_insert.cc
        Bug #38816: kill + flush tables with read lock + stored
                    procedures causes crashes!
        
        Modification of THD::query/query_length has been guarded
        with the a THD::set_query() method calls/ LOCK_query_string
        mutex.
     @ sql/sql_parse.cc
        Bug #38816: kill + flush tables with read lock + stored
                    procedures causes crashes!
        
        Modification of THD::query/query_length has been guarded
        with the a THD::set_query()/memdup_query_w_gap()/reset_query()
        method calls/LOCK_query_string mutex.
     @ sql/sql_show.cc
        Bug #38816: kill + flush tables with read lock + stored
                    procedures causes crashes!
        
        Inter-thread read of THD::query/query_length field has
        been protected with a new per-thread LOCK_query_string
        mutex in the mysqld_list_processes function.

    modified:
      sql/ha_myisam.cc
      sql/log_event.cc
      sql/slave.cc
      sql/sp_head.cc
      sql/sql_class.cc
      sql/sql_class.h
      sql/sql_insert.cc
      sql/sql_parse.cc
      sql/sql_show.cc
=== modified file 'sql/ha_myisam.cc'
--- a/sql/ha_myisam.cc	2009-06-04 22:23:08 +0000
+++ b/sql/ha_myisam.cc	2009-06-10 22:28:04 +0000
@@ -1487,10 +1487,8 @@ bool ha_myisam::check_and_repair(THD *th
 
   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 *th
     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);
 }
 

=== modified file 'sql/log_event.cc'
--- a/sql/log_event.cc	2009-05-31 03:26:58 +0000
+++ b/sql/log_event.cc	2009-06-10 22:28:04 +0000
@@ -1959,8 +1959,7 @@ int Query_log_event::exec_event(struct s
       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));
@@ -2177,8 +2176,7 @@ end:
   thd->catalog= 0;
   thd->set_db(NULL, 0);                 /* will free the current database */
   DBUG_PRINT("info", ("end: query= 0"));
-  thd->query= 0;			// just to be sure
-  thd->query_length= 0;
+  thd->reset_query();
   VOID(pthread_mutex_unlock(&LOCK_thread_count));
   close_thread_tables(thd);      
   free_root(thd->mem_root,MYF(MY_KEEP_PREALLOC));
@@ -3258,8 +3256,7 @@ int Load_log_event::exec_event(NET* net,
       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)
       {
@@ -3368,8 +3365,7 @@ error:
   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;
+  thd->reset_query();
   VOID(pthread_mutex_unlock(&LOCK_thread_count));
   close_thread_tables(thd);
   if (thd->query_error)

=== modified file 'sql/slave.cc'
--- a/sql/slave.cc	2009-05-22 23:15:21 +0000
+++ b/sql/slave.cc	2009-06-10 22:28:04 +0000
@@ -1608,15 +1608,13 @@ static int create_table_from_dump(THD* t
     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;
 
@@ -3868,8 +3866,7 @@ err:
   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->reset_query();
   thd->reset_db(NULL, 0);
   VOID(pthread_mutex_unlock(&LOCK_thread_count));
   if (mysql)
@@ -4113,8 +4110,7 @@ the slave SQL thread with \"SLAVE START\
   */
   thd->catalog= 0; 
   thd->reset_db(NULL, 0);
-  thd->query= 0; 
-  thd->query_length= 0;
+  thd->reset_query();
   VOID(pthread_mutex_unlock(&LOCK_thread_count));
   thd_proc_info(thd, "Waiting for slave mutex on exit");
   pthread_mutex_lock(&rli->run_lock);

=== modified file 'sql/sp_head.cc'
--- a/sql/sp_head.cc	2009-05-06 13:06:32 +0000
+++ b/sql/sp_head.cc	2009-06-10 22:28:04 +0000
@@ -949,8 +949,7 @@ subst_spvars(THD *thd, sp_instr *instr, 
   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 *n
     }
     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);

=== modified file 'sql/sql_class.cc'
--- a/sql/sql_class.cc	2009-05-15 09:29:41 +0000
+++ b/sql/sql_class.cc	2009-06-10 22:28:04 +0000
@@ -260,6 +260,7 @@ THD::THD()
   active_vio = 0;
 #endif
   pthread_mutex_init(&LOCK_delete, MY_MUTEX_INIT_FAST);
+  pthread_mutex_init(&LOCK_query_string, MY_MUTEX_INIT_FAST);
 
   /* Variables with default values */
   proc_info="login";
@@ -513,6 +514,7 @@ THD::~THD()
   free_root(&transaction.mem_root,MYF(0));
 #endif
   mysys_var=0;					// Safety (shouldn't be needed)
+  pthread_mutex_destroy(&LOCK_query_string);
   pthread_mutex_destroy(&LOCK_delete);
 #ifndef DBUG_OFF
   dbug_sentry= THD_SENTRY_GONE;
@@ -2422,3 +2424,31 @@ void xid_cache_delete(XID_STATE *xid_sta
   pthread_mutex_unlock(&LOCK_xid_cache);
 }
 
+
+void THD::set_statement(Statement *stmt)
+{
+  pthread_mutex_lock(&LOCK_query_string);
+  Statement::set_statement(stmt);
+  pthread_mutex_unlock(&LOCK_query_string);
+}
+ 
+
+void THD::set_query(char *str, uint32 length)
+{
+  pthread_mutex_lock(&LOCK_query_string);
+  query= str;
+  query_length= length;
+  pthread_mutex_unlock(&LOCK_query_string);
+}
+
+
+bool THD::memdup_query_w_gap(const char *str, uint32 size, uint gap)
+{
+  char *s= (char*) memdup_w_gap((gptr) (str), size, gap);
+  if (!s)
+    return TRUE;
+  s[size]= 0;
+  set_query(s, size);
+  return FALSE;
+}
+

=== modified file 'sql/sql_class.h'
--- a/sql/sql_class.h	2009-05-05 21:03:23 +0000
+++ b/sql/sql_class.h	2009-06-10 22:28:04 +0000
@@ -866,7 +866,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 */
@@ -1230,6 +1230,7 @@ public:
   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
+  pthread_mutex_t LOCK_query_string;	// Guard query & query_length fields
   /* all prepared statements and cursors of this connection */
   Statement_map stmt_map;
   /*
@@ -1882,6 +1883,17 @@ public:
   */
   void pop_internal_handler();
 
+  /* Overloaded to guard query/query_length fields */
+  virtual void set_statement(Statement *stmt);
+
+  /*
+    Functions to protect modifications of query and query_length fields
+    with LOCK_query_string.
+  */
+  void set_query(char *str, uint32 length);
+  bool memdup_query_w_gap(const char *str, uint32 size, uint gap);
+  void reset_query() { set_query(NULL, 0); }
+
 private:
   /** The current internal error handler for this thread, or NULL. */
   Internal_error_handler *m_internal_handler;

=== modified file 'sql/sql_insert.cc'
--- a/sql/sql_insert.cc	2009-06-04 22:23:08 +0000
+++ b/sql/sql_insert.cc	2009-06-10 22:28:04 +0000
@@ -1827,7 +1827,7 @@ bool delayed_get_table(THD *thd, TABLE_L
       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 */

=== modified file 'sql/sql_parse.cc'
--- a/sql/sql_parse.cc	2009-05-06 13:06:32 +0000
+++ b/sql/sql_parse.cc	2009-06-10 22:28:04 +0000
@@ -1106,8 +1106,7 @@ void execute_init_command(THD *thd, sys_
     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;
   /*
@@ -1350,10 +1349,8 @@ pthread_handler_t handle_bootstrap(void 
            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';
+    thd->memdup_query_w_gap(buff, length, 
+                            thd->db_length + 1 + QUERY_CACHE_FLAGS_SIZE + 1);
     DBUG_PRINT("query",("%-.4096s",thd->query));
 #if defined(ENABLED_PROFILING) && defined(COMMUNITY_SERVER)
     thd->profiling.set_query_source(thd->query, length);
@@ -1463,8 +1460,7 @@ int mysql_table_dump(THD* thd, char* db,
   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);
@@ -1988,8 +1984,7 @@ bool dispatch_command(enum enum_server_c
 #endif
 
       VOID(pthread_mutex_lock(&LOCK_thread_count));
-      thd->query_length= length;
-      thd->query= next_packet;
+      thd->set_query(next_packet, length);
       /*
         Count each statement from the client.
       */
@@ -2041,9 +2036,10 @@ bool dispatch_command(enum enum_server_c
         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);
@@ -2330,8 +2326,7 @@ bool dispatch_command(enum enum_server_c
   VOID(pthread_mutex_lock(&LOCK_thread_count)); // For process list
   thd_proc_info(thd, 0);
   thd->command=COM_SLEEP;
-  thd->query=0;
-  thd->query_length=0;
+  thd->reset_query();
   thread_running--;
   VOID(pthread_mutex_unlock(&LOCK_thread_count));
   thd->packet.shrink(thd->variables.net_buffer_length);	// Reclaim some memory
@@ -2551,14 +2546,9 @@ bool alloc_query(THD *thd, const char *p
     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)))
+  if (thd->memdup_query_w_gap(packet, packet_length,
+                              thd->db_length + 1 + QUERY_CACHE_FLAGS_SIZE))
     return TRUE;
-  thd->query[packet_length]=0;
-  thd->query_length= packet_length;
 
   /* Reclaim some memory */
   thd->packet.shrink(thd->variables.net_buffer_length);

=== modified file 'sql/sql_show.cc'
--- a/sql/sql_show.cc	2009-05-05 21:03:23 +0000
+++ b/sql/sql_show.cc	2009-06-10 22:28:04 +0000
@@ -1417,8 +1417,10 @@ void mysqld_list_processes(THD *thd,cons
 	    the comment in sql_class.h why this prevents crashes in possible
             races with query_length
           */
+          pthread_mutex_lock(&tmp->LOCK_query_string);
           uint length= min(max_query_length, tmp->query_length);
           thd_info->query=(char*) thd->strmake(tmp->query,length);
+          pthread_mutex_unlock(&tmp->LOCK_query_string);
         }
         thread_infos.append(thd_info);
       }


Attachment: [text/bzr-bundle] bzr/gshchepa@mysql.com-20090610222804-s37tq9darlt9n9k0.bundle
Thread
bzr commit into mysql-5.0-bugteam branch (gshchepa:2768) Bug#38816Gleb Shchepa11 Jun
  • Re: bzr commit into mysql-5.0-bugteam branch (gshchepa:2768) Bug#38816Konstantin Osipov23 Jul
    • Re: bzr commit into mysql-5.0-bugteam branch (gshchepa:2768)Bug#38816Sergei Golubchik23 Jul
      • Re: bzr commit into mysql-5.0-bugteam branch (gshchepa:2768) Bug#38816Konstantin Osipov23 Jul