Description:
###
trx would be used after free in the function `innobase_commit_by_xid` and ``innobase_rollback_by_xid`
* a) The trx object would be used in the destructor of TrxInInnoDB,
which was freed back to the pool in trx_free_for_background.
```cpp
innobase_commit_by_xid() {
if (trx != NULL) {
TrxInInnoDB trx_in_innodb(trx);
trx_free_for_background(trx);
}
```
* b) If another THD reuse the trx just before the destructor of
TrxInInnoDB, then the trx is used by 2 THD's at the same time;
* c) A sequence of 2 THD would break the debug assert or hang in
the release version:
THD A would run the exit() inside the destructor of TrxInInnoDB;
and THD B would run construtor and destructor.
- S1) THD A wait after trx_free_for_background(trx), current in_depth is 1;
- S2) THD B run TrxInInnoDB() -> enter(), which would skip the in_innodb++
since current in_depth is 2;
- S3) THD B run ~TrxInInnoDB() -> exit() -> --in_depth,
after that is in_depth is 1;
- S4) THD A run ~TrxInInnoDB() -> exit() -> --in_depth,
after that is in_depth is 0;
- S5) THD A would decrease in_innodb from 1 to 0;
- S6) THD B would hit the dbug assert since in_innodb is 0 now.
```shell
int
innobase_commit_by_xid(
/*===================*/
handlerton* hton,
XID* xid) /*!< in: X/Open XA transaction identification */
{
DBUG_ASSERT(hton == innodb_hton_ptr);
trx_t* trx = trx_get_trx_by_xid(xid);
if (trx != NULL) {
TrxInInnoDB trx_in_innodb(trx);
innobase_commit_low(trx);
ut_ad(trx->mysql_thd == NULL);
/* use cases are: disconnected xa, slave xa, recovery */
trx_deregister_from_2pc(trx);
ut_ad(!trx->will_lock); /* trx cache requirement */
trx_free_for_background(trx);
return(XA_OK);
} else {
return(XAER_NOTA);
}
}
```
How to repeat:
* a mtr case would trigger this debug assert.
```mtr
# ==== Purpose ====
--source include/have_debug.inc
--source include/have_debug_sync.inc
--source include/have_binlog_format_row.inc
--source include/have_gtid.inc
--source include/master-slave.inc
--source include/rpl_connection_master.inc
--let $saved_debug=`SELECT @@global.DEBUG`
CREATE TABLE t1 (
c1 INT NOT NULL,
PRIMARY KEY(`c1`)
) ENGINE=InnoDB;
INSERT INTO t1 values(1);
--connect(con1,127.0.0.1,root,,,$MASTER_MYPORT,)
--connect(con2,127.0.0.1,root,,,$MASTER_MYPORT,)
--connection con1
XA START 'XA1';
UPDATE t1 set c1=2;
XA END 'XA1';
XA PREPARE 'XA1';
--disconnect con1
--source include/rpl_connection_master1.inc
SET DEBUG_SYNC="ib_commit_by_xid_thd1_before_decrease SIGNAL thd1_before_decrease WAIT_FOR thd2_decrease";
SET DEBUG_SYNC="ib_commit_by_xid_after_decrease SIGNAL thd1_decrease";
SET GLOBAL DEBUG="+d,ib_commit_by_xid_conflict";
--send XA COMMIT 'XA1'
## Make sure that master1 is blocked at `ib_commit_by_xid_thd1_before_decrease`
--source include/rpl_connection_master.inc
SET DEBUG_SYNC="now WAIT_FOR thd1_before_decrease";
--connection con2
SET DEBUG_SYNC="ib_commit_by_xid_after_decrease SIGNAL thd2_decrease WAIT_FOR thd1_decrease";
--send INSERT INTO t1 VALUES(3);
--source include/rpl_connection_master1.inc
--reap
--connection con2
--reap
#
# Cleanup
#
--source include/rpl_connection_master.inc
DROP TABLE t1;
--eval SET GLOBAL DEBUG='$saved_debug'
--source include/sync_slave_sql_with_master.inc
--source include/rpl_end.inc
```
--- a/storage/innobase/include/trx0trx.h
+++ b/storage/innobase/include/trx0trx.h
@@ -1629,8 +1629,17 @@ private:
ut_ad(trx->in_depth > 0);
+ DBUG_EXECUTE_IF("ib_commit_by_xid_triggered", {
+ DBUG_SET_INITIAL("+d,ib_commit_by_xid_first_hit");
+ DEBUG_SYNC_C("ib_commit_by_xid_thd1_before_decrease");
+ });
+
--trx->in_depth;
+ DBUG_EXECUTE_IF("ib_commit_by_xid_first_hit", {
+ DEBUG_SYNC_C("ib_commit_by_xid_after_decrease");
+ });
+
if (trx->in_depth > 0) {
return;
Suggested fix:
* move the `trx_free_for_background(trx);` outside of the range of `TrxInInnoDB trx_in_innodb(trx);`
```cpp
--- a/storage/innobase/handler/ha_innodb.cc
+++ b/storage/innobase/handler/ha_innodb.cc
@@ -18586,13 +18586,19 @@ innobase_commit_by_xid(
trx_t* trx = trx_get_trx_by_xid(xid);
if (trx != NULL) {
- TrxInInnoDB trx_in_innodb(trx);
+ {
+ TrxInInnoDB trx_in_innodb(trx);
- innobase_commit_low(trx);
- ut_ad(trx->mysql_thd == NULL);
- /* use cases are: disconnected xa, slave xa, recovery */
- trx_deregister_from_2pc(trx);
- ut_ad(!trx->will_lock); /* trx cache requirement */
+ innobase_commit_low(trx);
+ ut_ad(trx->mysql_thd == NULL);
+ /* use cases are: disconnected xa, slave xa, recovery */
+ trx_deregister_from_2pc(trx);
+ ut_ad(!trx->will_lock); /* trx cache requirement */
+
+ DBUG_EXECUTE_IF("ib_commit_by_xid_conflict", {
+ DBUG_SET_INITIAL("+d,ib_commit_by_xid_triggered");
+ });
+ }
trx_free_for_background(trx);
return(XA_OK);
@@ -18618,12 +18624,15 @@ innobase_rollback_by_xid(
trx_t* trx = trx_get_trx_by_xid(xid);
if (trx != NULL) {
- TrxInInnoDB trx_in_innodb(trx);
+ int ret = 0;
+ {
+ TrxInInnoDB trx_in_innodb(trx);
- int ret = innobase_rollback_trx(trx);
+ ret = innobase_rollback_trx(trx);
- trx_deregister_from_2pc(trx);
- ut_ad(!trx->will_lock);
+ trx_deregister_from_2pc(trx);
+ ut_ad(!trx->will_lock);
+ }
trx_free_for_background(trx);
return(ret);
```
Description: ### trx would be used after free in the function `innobase_commit_by_xid` and ``innobase_rollback_by_xid` * a) The trx object would be used in the destructor of TrxInInnoDB, which was freed back to the pool in trx_free_for_background. ```cpp innobase_commit_by_xid() { if (trx != NULL) { TrxInInnoDB trx_in_innodb(trx); trx_free_for_background(trx); } ``` * b) If another THD reuse the trx just before the destructor of TrxInInnoDB, then the trx is used by 2 THD's at the same time; * c) A sequence of 2 THD would break the debug assert or hang in the release version: THD A would run the exit() inside the destructor of TrxInInnoDB; and THD B would run construtor and destructor. - S1) THD A wait after trx_free_for_background(trx), current in_depth is 1; - S2) THD B run TrxInInnoDB() -> enter(), which would skip the in_innodb++ since current in_depth is 2; - S3) THD B run ~TrxInInnoDB() -> exit() -> --in_depth, after that is in_depth is 1; - S4) THD A run ~TrxInInnoDB() -> exit() -> --in_depth, after that is in_depth is 0; - S5) THD A would decrease in_innodb from 1 to 0; - S6) THD B would hit the dbug assert since in_innodb is 0 now. ```shell int innobase_commit_by_xid( /*===================*/ handlerton* hton, XID* xid) /*!< in: X/Open XA transaction identification */ { DBUG_ASSERT(hton == innodb_hton_ptr); trx_t* trx = trx_get_trx_by_xid(xid); if (trx != NULL) { TrxInInnoDB trx_in_innodb(trx); innobase_commit_low(trx); ut_ad(trx->mysql_thd == NULL); /* use cases are: disconnected xa, slave xa, recovery */ trx_deregister_from_2pc(trx); ut_ad(!trx->will_lock); /* trx cache requirement */ trx_free_for_background(trx); return(XA_OK); } else { return(XAER_NOTA); } } ``` How to repeat: * a mtr case would trigger this debug assert. ```mtr # ==== Purpose ==== --source include/have_debug.inc --source include/have_debug_sync.inc --source include/have_binlog_format_row.inc --source include/have_gtid.inc --source include/master-slave.inc --source include/rpl_connection_master.inc --let $saved_debug=`SELECT @@global.DEBUG` CREATE TABLE t1 ( c1 INT NOT NULL, PRIMARY KEY(`c1`) ) ENGINE=InnoDB; INSERT INTO t1 values(1); --connect(con1,127.0.0.1,root,,,$MASTER_MYPORT,) --connect(con2,127.0.0.1,root,,,$MASTER_MYPORT,) --connection con1 XA START 'XA1'; UPDATE t1 set c1=2; XA END 'XA1'; XA PREPARE 'XA1'; --disconnect con1 --source include/rpl_connection_master1.inc SET DEBUG_SYNC="ib_commit_by_xid_thd1_before_decrease SIGNAL thd1_before_decrease WAIT_FOR thd2_decrease"; SET DEBUG_SYNC="ib_commit_by_xid_after_decrease SIGNAL thd1_decrease"; SET GLOBAL DEBUG="+d,ib_commit_by_xid_conflict"; --send XA COMMIT 'XA1' ## Make sure that master1 is blocked at `ib_commit_by_xid_thd1_before_decrease` --source include/rpl_connection_master.inc SET DEBUG_SYNC="now WAIT_FOR thd1_before_decrease"; --connection con2 SET DEBUG_SYNC="ib_commit_by_xid_after_decrease SIGNAL thd2_decrease WAIT_FOR thd1_decrease"; --send INSERT INTO t1 VALUES(3); --source include/rpl_connection_master1.inc --reap --connection con2 --reap # # Cleanup # --source include/rpl_connection_master.inc DROP TABLE t1; --eval SET GLOBAL DEBUG='$saved_debug' --source include/sync_slave_sql_with_master.inc --source include/rpl_end.inc ``` --- a/storage/innobase/include/trx0trx.h +++ b/storage/innobase/include/trx0trx.h @@ -1629,8 +1629,17 @@ private: ut_ad(trx->in_depth > 0); + DBUG_EXECUTE_IF("ib_commit_by_xid_triggered", { + DBUG_SET_INITIAL("+d,ib_commit_by_xid_first_hit"); + DEBUG_SYNC_C("ib_commit_by_xid_thd1_before_decrease"); + }); + --trx->in_depth; + DBUG_EXECUTE_IF("ib_commit_by_xid_first_hit", { + DEBUG_SYNC_C("ib_commit_by_xid_after_decrease"); + }); + if (trx->in_depth > 0) { return; Suggested fix: * move the `trx_free_for_background(trx);` outside of the range of `TrxInInnoDB trx_in_innodb(trx);` ```cpp --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -18586,13 +18586,19 @@ innobase_commit_by_xid( trx_t* trx = trx_get_trx_by_xid(xid); if (trx != NULL) { - TrxInInnoDB trx_in_innodb(trx); + { + TrxInInnoDB trx_in_innodb(trx); - innobase_commit_low(trx); - ut_ad(trx->mysql_thd == NULL); - /* use cases are: disconnected xa, slave xa, recovery */ - trx_deregister_from_2pc(trx); - ut_ad(!trx->will_lock); /* trx cache requirement */ + innobase_commit_low(trx); + ut_ad(trx->mysql_thd == NULL); + /* use cases are: disconnected xa, slave xa, recovery */ + trx_deregister_from_2pc(trx); + ut_ad(!trx->will_lock); /* trx cache requirement */ + + DBUG_EXECUTE_IF("ib_commit_by_xid_conflict", { + DBUG_SET_INITIAL("+d,ib_commit_by_xid_triggered"); + }); + } trx_free_for_background(trx); return(XA_OK); @@ -18618,12 +18624,15 @@ innobase_rollback_by_xid( trx_t* trx = trx_get_trx_by_xid(xid); if (trx != NULL) { - TrxInInnoDB trx_in_innodb(trx); + int ret = 0; + { + TrxInInnoDB trx_in_innodb(trx); - int ret = innobase_rollback_trx(trx); + ret = innobase_rollback_trx(trx); - trx_deregister_from_2pc(trx); - ut_ad(!trx->will_lock); + trx_deregister_from_2pc(trx); + ut_ad(!trx->will_lock); + } trx_free_for_background(trx); return(ret); ```