Details
-
Bug
-
Status: Closed (View Workflow)
-
Critical
-
Resolution: Fixed
-
10.4(EOL), 10.5, 10.6, 10.7(EOL), 10.8(EOL), 10.10(EOL), 10.11
-
None
Description
trx->mysql_thd can be zeroed-out between thd_get_thread_id() and thd_query_safe() calls in fill_trx_row(). trx_disconnect_prepared() zeroes out trx->mysql_thd. And this can cause null pointer dereferencing in fill_trx_row().
The bug can be reproduced with the following new sync point:
diff --git a/storage/innobase/trx/trx0i_s.cc b/storage/innobase/trx/trx0i_s.cc
|
index 2dc39118d3d..c2cc8c970b0 100644 |
--- a/storage/innobase/trx/trx0i_s.cc
|
+++ b/storage/innobase/trx/trx0i_s.cc
|
@@ -461,6 +461,8 @@ fill_trx_row( |
row->trx_mysql_thread_id = thd_get_thread_id(trx->mysql_thd);
|
|
char query[TRX_I_S_TRX_QUERY_MAX_LEN + 1]; |
+ ut_d(if (trx->state == TRX_STATE_PREPARED) |
+ DEBUG_SYNC_C("fill_trx_row_before_query_request")); |
if (size_t stmt_len = thd_query_safe(trx->mysql_thd, query, |
sizeof query)) {
|
row->trx_query = static_cast<const char*>( |
diff --git a/storage/innobase/trx/trx0trx.cc b/storage/innobase/trx/trx0trx.cc
|
index 3b19d213d5a..92bf9de375a 100644 |
--- a/storage/innobase/trx/trx0trx.cc
|
+++ b/storage/innobase/trx/trx0trx.cc
|
@@ -550,6 +550,7 @@ void trx_disconnect_prepared(trx_t *trx) |
trx->read_view.close();
|
trx->is_recovered= true; |
trx->mysql_thd= NULL;
|
+ DEBUG_SYNC_C("trx_disconnect_prepared_reset_thd"); |
/* todo/fixme: suggest to do it at innodb prepare */ |
trx->will_lock= false; |
trx_sys.rw_trx_hash.put_pins(trx);
|
and the following test case:
--source include/have_innodb.inc
|
--source include/have_debug.inc
|
--source include/have_debug_sync.inc
|
--source include/count_sessions.inc
|
|
--connection default |
create table t (a int) engine=innodb; |
insert into t values(1); |
|
--connect (con_xa, localhost, root,,)
|
SET DEBUG_SYNC="trx_disconnect_prepared_reset_thd SIGNAL thd_reset"; |
xa start '1'; |
insert into t values(1); |
xa end '1'; |
xa prepare '1'; |
|
--connection default |
SET DEBUG_SYNC="fill_trx_row_before_query_request SIGNAL reached WAIT_FOR fill_row_cont"; |
--send select * from information_schema.innodb_trx;
|
|
--connect (con_sync, localhost, root,,)
|
SET DEBUG_SYNC="now WAIT_FOR reached"; |
--disconnect con_xa
|
SET DEBUG_SYNC="now WAIT_FOR thd_reset"; |
SET DEBUG_SYNC="now SIGNAL fill_row_cont"; |
--disconnect con_sync
|
|
--connection default |
--disable_result_log
|
# Must crash here with SIGSEGV if not fixed |
--reap;
|
--enable_result_log
|
xa commit '1'; |
drop table t;
|
SET DEBUG_SYNC="RESET"; |
--source include/wait_until_count_sessions.inc
|
It does not affect 10.3 as 10.3 does not detach XA on disconnection (compare THD::cleanup() in 10.3 and 10.4+ and see trans_xa_detach() in 10.4+ for details).
Until MDEV-29368 is fixed the workaround is not to use innodb_trx, innodb_locks and innodb_lock_waits from information_schema along with detached XA's.
Attachments
Issue Links
- blocks
-
-
- Closed
-
- is blocked by
-
-
- Closed
-
- is caused by
-
MDEV-15773 Simplify away trx_sys_t::m_views
-
- Closed
-
Activity
10.6 code analyses:
fill_trx_row() is invoked from fetch_data_into_cache(), which, in turns, iterates transactions with trx_sys.trx_list.for_each() function, which holds trx_sys.trx_list.mutex during the iteration.
At the other hand, innobase_close_connection() invokes trx->free(), which removes transaction from transactions container trx_sys.deregister_trx(this), which acquires trx_sys.trx_list.mutex too. And after this trx_t::free() zeroes out trx->mysql_thd. That's why we don't catch the bug with usual disconnection.
trx_disconnect_prepared() is invoked only from innobase_close_connection(). I think trx_disconnect_prepared() must zero out trx->mysql_thd under trx_sys.trx_list.mutex(), copying the above logic of trx_t::free(). thread_safe_trx_ilist_t::freeze()/unfreeze() functions could be used for that.
The same is true for 10.4. See fetch_data_into_cache() for details.
The bug does not affect 10.3, because the following commit:
commit 0993d6b81b6cf7a5fc0710d99e962a8271018b9d
|
Author: Sergey Vojtovich <svoj@mariadb.org>
|
Date: Fri Mar 30 00:33:58 2018 +0400
|
|
|
MDEV-15773 - trx_sys.mysql_trx_list -> trx_sys.trx_list
|
|
Replaced "list of transactions created for MySQL" with "list of all
|
transactions". This simplifies code and allows further removal of
|
trx_sys.m_views.
|
exists only for 10.4. And trx_disconnect_prepared() is just not invoked with the above test in 10.3. But we will also fix 10.3 to be sure, it will not be broken, if somebody decide to backport the above commit to 10.3.
The bug is caused by the following commit:
commit 0993d6b81b6cf7a5fc0710d99e962a8271018b9d
|
Author: Sergey Vojtovich <svoj@mariadb.org>
|
Date: Fri Mar 30 00:33:58 2018 +0400
|
|
|
MDEV-15773 - trx_sys.mysql_trx_list -> trx_sys.trx_list
|
|
Replaced "list of transactions created for MySQL" with "list of all
|
transactions". This simplifies code and allows further removal of
|
trx_sys.m_views.
|
Because before the fix trx_disconnect_from_mysql() contained the critical section. But after the fix, that function was replaced with trx_disconnect_prepared(), which does not contain the critical section.
Useful comment from marko:
A possible fix might be to acquire THD::LOCK_thd_data in a safe way, and to ensure that a disconnect would be blocked it. Currently it is not the case; see
MDEV-29368.The function fill_trx_row() does check if trx_t::mysql_thd is a null pointer, but race conditions after that point are possible. Holding exclusive lock_sys.latch (or before 10.6, lock_sys.mutex) will block lock_release() during trx_t::release_locks() but not the transaction state change.
When it comes to race conditions with innobase_close_connection(), there is no way to synchronize with trx_disconnect_prepared(). The innobase_rollback_trx() would be blocked by lock_sys.latch and trx_t::mutex.