MDEV-22782 AddressSanitizer race condition in trx_free()

In trx_free() we used to declare the entire trx_t unaccessible
and then declare that some data members are accessible.
This involves a race condition with other threads that may concurrently
access the data members that must remain accessible.
One type of error is "AddressSanitizer: unknown-crash", whose
exact cause we have not determined.

Another type of error (reported in MDEV-23472) is "use-after-poison",
where the reported shadow bytes would in fact be 00, indicating that
the memory was no longer poisoned. The poison-access-unpoison race
condition was confirmed by "rr replay".

We eliminate the race condition by invoking MEM_NOACCESS on each
individual data member of trx_t before freeing the memory to the pool.
The memory would not be unpoisoned until the pool is freed
or the memory is being reused for another allocation.

trx_t::free(): Replaces trx_free().

trx_t::active_commit_ordered: Changed to bool, so that MEM_NOACCESS
can be invoked. Removed some accessor functions.

Pool: Remove all MEM_ instrumentation.

TrxFactory: Move the MEM_ instrumentation from Pool.

TrxFactory::debug(): Removed. Moved to trx_t::free(). Because
the memory was already marked unaccessible in trx_t::free(), the
Factory::debug() call in Pool::putl() would be unable to access it.

trx_allocate_for_background(): Replaces trx_create_low().

trx_t::free(): Perform all consistency checks while avoiding
duplication, and declare most data members unaccessible.

storage/innobase/fts/fts0fts.cc

@@ -4234,7 +4234,7 @@ fts_sync_commit(
 			<< " ins/sec";
 	}
 
-	/* Avoid assertion in trx_free(). */
+	/* Avoid assertion in trx_t::free(). */
 	trx->dict_operation_lock_mode = 0;
 	trx_free_for_background(trx);
 
@@ -4288,7 +4288,7 @@ fts_sync_rollback(
 
 	fts_sql_rollback(trx);
 
-	/* Avoid assertion in trx_free(). */
+	/* Avoid assertion in trx_t::free(). */
 	trx->dict_operation_lock_mode = 0;
 	trx_free_for_background(trx);
 }

storage/innobase/handler/ha_innodb.cc

@@ -2800,18 +2800,6 @@ trx_is_registered_for_2pc(
 	return(trx->is_registered == 1);
 }
 
-/*********************************************************************//**
-Note that innobase_commit_ordered() was run. */
-static inline
-void
-trx_set_active_commit_ordered(
-/*==========================*/
-	trx_t*	trx)	/* in: transaction */
-{
-	ut_a(trx_is_registered_for_2pc(trx));
-	trx->active_commit_ordered = 1;
-}
-
 /*********************************************************************//**
 Note that a transaction has been registered with MySQL 2PC coordinator. */
 static inline
@@ -2821,7 +2809,7 @@ trx_register_for_2pc(
 	trx_t*	trx)	/* in: transaction */
 {
 	trx->is_registered = 1;
-	ut_ad(trx->active_commit_ordered == 0);
+	ut_ad(!trx->active_commit_ordered);
 }
 
 /*********************************************************************//**
@@ -2832,19 +2820,8 @@ trx_deregister_from_2pc(
 /*====================*/
 	trx_t*	trx)	/* in: transaction */
 {
-	trx->is_registered = 0;
-	trx->active_commit_ordered = 0;
-}
-
-/*********************************************************************//**
-Check whether a transaction has active_commit_ordered set */
-static inline
-bool
-trx_is_active_commit_ordered(
-/*=========================*/
-	const trx_t*	trx)	/* in: transaction */
-{
-	return(trx->active_commit_ordered == 1);
+  trx->is_registered= false;
+  trx->active_commit_ordered= false;
 }
 
 /*********************************************************************//**
@@ -4617,8 +4594,7 @@ innobase_commit_ordered(
 		(!thd_test_options(thd, OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)));
 
 	innobase_commit_ordered_2(trx, thd);
-
-	trx_set_active_commit_ordered(trx);
+	trx->active_commit_ordered = true;
 
 	DBUG_VOID_RETURN;
 }
@@ -4671,7 +4647,7 @@ innobase_commit(
 				DBUG_SUICIDE(););
 
 		/* Run the fast part of commit if we did not already. */
-		if (!trx_is_active_commit_ordered(trx)) {
+		if (!trx->active_commit_ordered) {
 			innobase_commit_ordered_2(trx, thd);
 
 		}
@@ -5185,12 +5161,12 @@ static void innobase_kill_query(handlerton*, THD* thd, enum thd_kill_levels)
     trx_mutex_enter(trx);
     /* It is possible that innobase_close_connection() is concurrently
     being executed on our victim. In that case, trx->mysql_thd would
-    be reset before invoking trx_free(). Even if the trx object is later
+    be reset before invoking trx_t::free(). Even if the trx object is later
     reused for another client connection or a background transaction,
     its trx->mysql_thd will differ from our thd.
 
     If trx never performed any changes, nothing is really protecting
-    the trx_free() call or the changes of trx_t::state when the
+    the trx_t::free() call or the changes of trx_t::state when the
     transaction is being rolled back and trx_commit_low() is being
     executed.
 

storage/innobase/include/trx0trx.h

@@ -73,12 +73,9 @@ Creates a transaction object for MySQL.
 trx_t*
 trx_allocate_for_mysql(void);
 /*========================*/
-/********************************************************************//**
-Creates a transaction object for background operations by the master thread.
-@return own: transaction object */
-trx_t*
-trx_allocate_for_background(void);
-/*=============================*/
+
+/** @return allocated transaction object for internal operations */
+trx_t *trx_allocate_for_background();
 
 /** Frees and initialize a transaction object instantinated during recovery.
 @param trx trx object to free and initialize during recovery */
@@ -917,7 +914,8 @@ struct trx_t {
 					the coordinator using the XA API, and
 					is set to false  after commit or
 					rollback. */
-	unsigned	active_commit_ordered:1;/* 1 if owns prepare mutex */
+	/** whether this is holding the prepare mutex */
+	bool		active_commit_ordered;
 	/*------------------------------*/
 	bool		check_unique_secondary;
 					/*!< normally TRUE, but if the user
@@ -1204,6 +1202,9 @@ struct trx_t {
     ut_ad(old_n_ref > 0);
   }
 
+  /** Free the memory to trx_pools */
+  inline void free();
+
 
 private:
 	/** Assign a rollback segment for modifying temporary tables.

storage/innobase/include/ut0pool.h

@@ -87,15 +87,6 @@ struct Pool {
 		for (Element* elem = m_start; elem != m_last; ++elem) {
 
 			ut_ad(elem->m_pool == this);
-#ifdef __SANITIZE_ADDRESS__
-			/* Unpoison the memory for AddressSanitizer */
-			MEM_UNDEFINED(&elem->m_type, sizeof elem->m_type);
-#endif
-#ifdef HAVE_valgrind
-			/* Declare the contents as initialized for Valgrind;
-			we checked this in mem_free(). */
-			MEM_MAKE_DEFINED(&elem->m_type, sizeof elem->m_type);
-#endif
 			Factory::destroy(&elem->m_type);
 		}
 
@@ -130,22 +121,6 @@ struct Pool {
 			elem = NULL;
 		}
 
-#if defined HAVE_valgrind || defined __SANITIZE_ADDRESS__
-		if (elem) {
-# ifdef __SANITIZE_ADDRESS__
-			/* Unpoison the memory for AddressSanitizer */
-			MEM_UNDEFINED(&elem->m_type, sizeof elem->m_type);
-# endif
-# ifdef HAVE_valgrind
-			/* Declare the memory initialized for Valgrind.
-			The trx_t that are released to the pool are
-			actually initialized; we checked that by
-			MEM_CHECK_DEFINED() in mem_free() below. */
-# endif
-			MEM_MAKE_DEFINED(&elem->m_type, sizeof elem->m_type);
-		}
-#endif
-
 		m_lock_strategy.exit();
 		return elem ? &elem->m_type : NULL;
 	}
@@ -158,12 +133,10 @@ struct Pool {
 		byte*		p = reinterpret_cast<byte*>(ptr + 1);
 
 		elem = reinterpret_cast<Element*>(p - sizeof(*elem));
-		MEM_CHECK_DEFINED(&elem->m_type, sizeof elem->m_type);
 
 		elem->m_pool->m_lock_strategy.enter();
 
 		elem->m_pool->putl(elem);
-		MEM_NOACCESS(&elem->m_type, sizeof elem->m_type);
 
 		elem->m_pool->m_lock_strategy.exit();
 	}
@@ -186,9 +159,6 @@ struct Pool {
 	void putl(Element* elem)
 	{
 		ut_ad(elem >= m_start && elem < m_last);
-
-		ut_ad(Factory::debug(&elem->m_type));
-
 		m_pqueue.push(elem);
 	}