From dc0a87fdc24ed0859856d243ad68a0c1913db3af Mon Sep 17 00:00:00 2001 From: Gleb Shchepa Date: Fri, 24 Jul 2009 20:58:58 +0500 Subject: 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: 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_thd_data 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 THD::set_query()) method call/LOCK_thd_data 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 THD::set_query() method call/LOCK_thd_data mutex. The THD::LOCK_delete mutex has been renamed to THD::LOCK_thd_data. 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 call/LOCK_thd_data mutex. sql/sql_class.cc: Bug #38816: kill + flush tables with read lock + stored procedures causes crashes! The new THD::LOCK_thd_data mutex and THD::set_query() method has 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. The THD::LOCK_delete mutex has been renamed to THD::LOCK_thd_data. sql/sql_class.h: Bug #38816: kill + flush tables with read lock + stored procedures causes crashes! The new THD::LOCK_thd_data mutex and THD::set_query() method has 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. The THD::LOCK_delete mutex has been renamed to THD::LOCK_thd_data. 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 call/LOCK_thd_data 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() method call/LOCK_thd_data mutex. sql/sql_repl.cc: Bug #38816: kill + flush tables with read lock + stored procedures causes crashes! The THD::LOCK_delete mutex has been renamed to THD::LOCK_thd_data. 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_thd_data mutex in the mysqld_list_processes function. --- sql/sql_class.h | 52 +++++++++++++++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 21 deletions(-) (limited to 'sql/sql_class.h') diff --git a/sql/sql_class.h b/sql/sql_class.h index 82c464cb475..7c747e459a4 100644 --- a/sql/sql_class.h +++ b/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; -- cgit v1.2.1