MDEV-17092 use-after-poison around lock_trx_handle_wait_low

There was a race condition where the connection of the
victim of a KILL statement is disconnected while the
KILL statement is executing.

As a side effect of this fix, we will make XA PREPARE
transactions immune to KILL statements.

Starting with MariaDB 10.2, we have a pool of trx_t objects.
trx_free() would only free memory to the pool. We poison the
contents of freed objects in the pool in order to catch misuse.

trx_free(): Unpoison also trx->mysql_thd and trx->state.
This is to counter the poisoning of *trx in trx_pools->mem_free().
Unpoison only on AddressSanitizer or Valgrind, but not on MemorySanitizer.

Pool: Unpoison allocated objects only on AddressSanitizer or
Valgrind, but not on MemorySanitizer.

innobase_kill_query(): Properly protect trx, acquiring also
trx_sys_t::mutex and checking trx_t::mysql_thd and trx_t::state.

storage/innobase/handler/ha_innodb.cc

@@ -5201,13 +5201,43 @@ static void innobase_kill_query(handlerton*, THD* thd, enum thd_kill_levels)
 	}
 #endif /* WITH_WSREP */
 
-	if (trx_t* trx = thd_to_trx(thd)) {
-		ut_ad(trx->mysql_thd == thd);
-		/* Cancel a pending lock request if there are any */
-		lock_trx_handle_wait(trx);
-	}
+  if (trx_t* trx= thd_to_trx(thd))
+  {
+    lock_mutex_enter();
+    trx_sys_mutex_enter();
+    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
+    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
+    transaction is being rolled back and trx_commit_low() is being
+    executed.
+
+    The function trx_allocate_for_mysql() acquires
+    trx_sys_t::mutex, but trx_allocate_for_background() will not.
+    Luckily, background transactions cannot be read-only, because
+    for read-only transactions, trx_start_low() will avoid acquiring
+    any of the trx_sys_t::mutex, lock_sys_t::mutex, trx_t::mutex before
+    assigning trx_t::state.
+
+    At this point, trx may have been reallocated for another client
+    connection, or for a background operation. In that case, either
+    trx_t::state or trx_t::mysql_thd should not match our expectations. */
+    bool cancel= trx->mysql_thd == thd && trx->state == TRX_STATE_ACTIVE &&
+      !trx->lock.was_chosen_as_deadlock_victim;
+    trx_sys_mutex_exit();
+    if (!cancel);
+    else if (lock_t *lock= trx->lock.wait_lock)
+      lock_cancel_waiting_and_release(lock);
+    lock_mutex_exit();
+    trx_mutex_exit(trx);
+  }
 
-	DBUG_VOID_RETURN;
+  DBUG_VOID_RETURN;
 }
 
 

storage/innobase/include/ut0pool.h

 /*****************************************************************************
 
 Copyright (c) 2013, 2014, Oracle and/or its affiliates. All Rights Reserved.
+Copyright (c) 2018, 2020, 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
@@ -86,11 +87,15 @@ 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(). */
 			UNIV_MEM_VALID(&elem->m_type, sizeof elem->m_type);
+#endif
 			Factory::destroy(&elem->m_type);
 		}
 
@@ -127,13 +132,18 @@ struct Pool {
 
 #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
 			UNIV_MEM_ASSERT_RW() in mem_free() below. */
 			UNIV_MEM_VALID(&elem->m_type, sizeof elem->m_type);
+# endif
 		}
 #endif
 

storage/innobase/trx/trx0trx.cc

@@ -408,14 +408,26 @@ trx_free(trx_t*& trx)
 	ut_ad(trx->will_lock == 0);
 
 	trx_pools->mem_free(trx);
+#ifdef __SANITIZE_ADDRESS__
 	/* Unpoison the memory for innodb_monitor_set_option;
 	it is operating also on the freed transaction objects. */
 	MEM_UNDEFINED(&trx->mutex, sizeof trx->mutex);
 	MEM_UNDEFINED(&trx->undo_mutex, sizeof trx->undo_mutex);
-	/* Declare the contents as initialized for Valgrind;
-	we checked that it was initialized in trx_pools->mem_free(trx). */
+	/* For innobase_kill_connection() */
+	MEM_UNDEFINED(&trx->state, sizeof trx->state);
+	MEM_UNDEFINED(&trx->mysql_thd, sizeof trx->mysql_thd);
+#endif
+#ifdef HAVE_valgrind
+	/* Unpoison the memory for innodb_monitor_set_option;
+	it is operating also on the freed transaction objects.
+	We checked that these were initialized in
+	trx_pools->mem_free(trx). */
 	UNIV_MEM_VALID(&trx->mutex, sizeof trx->mutex);
 	UNIV_MEM_VALID(&trx->undo_mutex, sizeof trx->undo_mutex);
+	/* For innobase_kill_connection() */
+	UNIV_MEM_VALID(&trx->state, sizeof trx->state);
+	UNIV_MEM_VALID(&trx->mysql_thd, sizeof trx->mysql_thd);
+#endif
 
 	trx = NULL;
 }