MDEV-16136: Prevent wrong reuse in trx_reference()

If trx_free() and trx_create_low() were called while a call to
trx_reference() was pending, we could get a reference to a wrong
transaction object.

trx_reference(): Return NULL if the trx->id no longer matches.

lock_trx_release_locks(): Assign trx->id = 0, so that trx_reference()
will not return a reference to this object.

trx_cleanup_at_db_startup(): Assign trx->id = 0.

assert_trx_is_free(): Assert !trx->n_ref. Assert trx->id == 0,
now that it will be cleared as part of a transaction commit.

5 files changed, 40 insertions, 53 deletions

diff --git a/storage/innobase/include/trx0sys.ic b/storage/innobase/include/trx0sys.ic
index af6fb63d9b4..861800ef40e 100644
--- a/storage/innobase/include/trx0sys.ic
+++ b/storage/innobase/include/trx0sys.ic
@@ -1,6 +1,7 @@
 /*****************************************************************************
 
 Copyright (c) 1996, 2015, Oracle and/or its affiliates. All Rights Reserved.
+Copyright (c) 2018, MariaDB Corporation.
 
 This program is free software; you can redistribute it and/or modify it under
 the terms of the GNU General Public License as published by the Free Software
@@ -360,14 +361,14 @@ trx_rw_is_active(
 	bool		do_ref_count)	/*!< in: if true then increment the
 					trx_t::n_ref_count */
 {
-	trx_t*		trx;
+	ut_ad(trx_id);
 
 	trx_sys_mutex_enter();
 
-	trx = trx_rw_is_active_low(trx_id, corrupt);
+	trx_t* trx = trx_rw_is_active_low(trx_id, corrupt);
 
-	if (trx != 0) {
-		trx = trx_reference(trx, do_ref_count);
+	if (trx) {
+		trx = trx_reference(do_ref_count ? trx_id : 0, trx);
 	}
 
 	trx_sys_mutex_exit();
diff --git a/storage/innobase/include/trx0trx.h b/storage/innobase/include/trx0trx.h
index 01670119381..917222477b1 100644
--- a/storage/innobase/include/trx0trx.h
+++ b/storage/innobase/include/trx0trx.h
@@ -519,19 +519,6 @@ trx_set_rw_mode(
 	trx_t*		trx);
 
 /**
-Increase the reference count. If the transaction is in state
-TRX_STATE_COMMITTED_IN_MEMORY then the transaction is considered
-committed and the reference count is not incremented.
-@param trx Transaction that is being referenced
-@param do_ref_count Increment the reference iff this is true
-@return transaction instance if it is not committed */
-UNIV_INLINE
-trx_t*
-trx_reference(
-	trx_t*		trx,
-	bool		do_ref_count);
-
-/**
 Release the transaction. Decrease the reference count.
 @param trx Transaction that is being released */
 UNIV_INLINE
@@ -600,7 +587,9 @@ Check transaction state */
 @param t transaction handle */
 #define	assert_trx_is_free(t)	do {					\
 	ut_ad(trx_state_eq((t), TRX_STATE_NOT_STARTED));		\
-	ut_ad(!trx->has_logged());					\
+	ut_ad(!(t)->id);						\
+	ut_ad(!(t)->has_logged());					\
+	ut_ad(!(t)->n_ref);						\
 	ut_ad(!MVCC::is_view_active((t)->read_view));			\
 	ut_ad((t)->lock.wait_thr == NULL);				\
 	ut_ad(UT_LIST_GET_LEN((t)->lock.trx_locks) == 0);		\
@@ -1277,6 +1266,32 @@ struct commit_node_t{
 	mutex_exit(&t->mutex);			\
 } while (0)
 
+/**
+Increase the reference count. If the transaction is in state
+TRX_STATE_COMMITTED_IN_MEMORY then the transaction is considered
+committed and the reference count is not incremented.
+@param id the transaction ID; 0 if not to increment the reference count
+@param trx Transaction that is being referenced
+@return trx
+@retval	NULL	if the transaction is no longer active */
+inline trx_t* trx_reference(trx_id_t id, trx_t* trx)
+{
+	trx_mutex_enter(trx);
+
+	if (trx_state_eq(trx, TRX_STATE_COMMITTED_IN_MEMORY)) {
+		trx = NULL;
+	} else if (!id) {
+	} else if (trx->id != id) {
+		trx = NULL;
+	} else {
+		ut_ad(trx->n_ref >= 0);
+		++trx->n_ref;
+	}
+
+	trx_mutex_exit(trx);
+	return(trx);
+}
+
 #include "trx0trx.ic"
 
 #endif
diff --git a/storage/innobase/include/trx0trx.ic b/storage/innobase/include/trx0trx.ic
index 7721e28bfb6..dd42c8b8368 100644
--- a/storage/innobase/include/trx0trx.ic
+++ b/storage/innobase/include/trx0trx.ic
@@ -211,35 +211,6 @@ ok:
 }
 
 /**
-Increase the reference count. If the transaction is in state
-TRX_STATE_COMMITTED_IN_MEMORY then the transaction is considered
-committed and the reference count is not incremented.
-@param trx Transaction that is being referenced
-@param do_ref_count Increment the reference iff this is true
-@return transaction instance if it is not committed */
-UNIV_INLINE
-trx_t*
-trx_reference(
-	trx_t*		trx,
-	bool		do_ref_count)
-{
-	trx_mutex_enter(trx);
-
-	if (trx_state_eq(trx, TRX_STATE_COMMITTED_IN_MEMORY)) {
-		trx_mutex_exit(trx);
-		trx = NULL;
-	} else if (do_ref_count) {
-		ut_ad(trx->n_ref >= 0);
-		++trx->n_ref;
-		trx_mutex_exit(trx);
-	} else {
-		trx_mutex_exit(trx);
-	}
-
-	return(trx);
-}
-
-/**
 Release the transaction. Decrease the reference count.
 @param trx Transaction that is being released */
 UNIV_INLINE
diff --git a/storage/innobase/lock/lock0lock.cc b/storage/innobase/lock/lock0lock.cc
index 90cae86073a..b82f323d8f9 100644
--- a/storage/innobase/lock/lock0lock.cc
+++ b/storage/innobase/lock/lock0lock.cc
@@ -6892,6 +6892,8 @@ lock_trx_release_locks(
 	the is_recovered flag. */
 
 	trx->is_recovered = false;
+	/* Ensure that trx_reference() will not find this transaction. */
+	trx->id = 0;
 
 	trx_mutex_exit(trx);
 
diff --git a/storage/innobase/trx/trx0trx.cc b/storage/innobase/trx/trx0trx.cc
index b79c9403c94..e4d9c0800c3 100644
--- a/storage/innobase/trx/trx0trx.cc
+++ b/storage/innobase/trx/trx0trx.cc
@@ -111,8 +111,6 @@ trx_init(
 /*=====*/
 	trx_t*	trx)
 {
-	trx->id = 0;
-
 	trx->no = TRX_ID_MAX;
 
 	trx->state = TRX_STATE_NOT_STARTED;
@@ -1223,10 +1221,7 @@ trx_start_low(
 		ut_ad(trx_sys_validate_trx_list());
 
 		trx_sys_mutex_exit();
-
 	} else {
-		trx->id = 0;
-
 		if (!trx_is_autocommit_non_locking(trx)) {
 
 			/* If this is a read-only transaction that is writing
@@ -1668,7 +1663,10 @@ trx_commit_in_memory(
 			trx_erase_lists(trx, serialised);
 		}
 
+		/* trx->id will be cleared in lock_trx_release_locks(trx). */
+		ut_ad(trx->read_only || !trx->rsegs.m_redo.rseg || trx->id);
 		lock_trx_release_locks(trx);
+		ut_ad(trx->id == 0);
 
 		/* Remove the transaction from the list of active
 		transactions now that it no longer holds any user locks. */
@@ -1685,7 +1683,6 @@ trx_commit_in_memory(
 			}
 
 		} else {
-			ut_ad(trx->id > 0);
 			MONITOR_INC(MONITOR_TRX_RW_COMMIT);
 		}
 	}
@@ -1956,6 +1953,7 @@ trx_cleanup_at_db_startup(
 	ut_ad(!trx->in_rw_trx_list);
 	ut_ad(!trx->in_mysql_trx_list);
 	DBUG_LOG("trx", "Cleanup at startup: " << trx);
+	trx->id = 0;
 	trx->state = TRX_STATE_NOT_STARTED;
 }