Ubuntu 17.04 but most probably unimportant.

I believe that this is a race condition between innobase_kill_query() and trx_free(). The thread associated with the thd->real_id is idle outside InnoDB, so it must have been freed the transaction object already:

#3  0x000055ad8c93443a in cache_thread (thd=0x62a0001e0208)

    at /work_m/bb-10.2-marko/sql/mysqld.cc:2972

#4  0x000055ad8c9349e0 in one_thread_per_connection_end (thd=0x62a0001e0208, 

    put_in_cache=true) at /work_m/bb-10.2-marko/sql/mysqld.cc:3061

Also, thd_to_trx() would now return NULL to innobase_kill_query() in the core dump:

(gdb) p thd->ha_data[innodb_hton_ptr->slot].ha_ptr

$7 = (void *) 0x0

It seems to me that in 10.2, we should probably acquire trx->mutex and trx_sys->mutex after invoking thd_to_trx(), then recheck thd_to_trx(), and finally acquire lock_sys->mutex.

In 10.3, trx_sys->mutex does not exist any more, so we must skip acquiring it. Perhaps we should do something smart about trx->id or other fields instead? trx_free() is not acquiring any mutex in 10.3.

Acquiring trx->mutex could trigger use-after-poison, so perhaps we should rewrite the poisoning so that it would always leave the mutex alone?

	trx_pools->mem_free(trx);

	/* Unpoison the memory for innodb_monitor_set_option;

	it is operating also on the freed transaction objects. */

	MEM_UNDEFINED(&trx->mutex, sizeof trx->mutex);

I debugged this a bit further in a short rr replay trace. Based on a disassembly of the code that called __asan_report_load1(), I found out the address of the ASAN poison bits and set an access watchpoint:

p &((char*)0x7fff8000)[((ulint)&trx.lock.was_chosen_as_deadlock_victim) / 8]

Armed with awatch, I narrowed the problem to the following area:

Thread 1 hit Breakpoint 6, innobase_kill_query (thd=0x62a0001fe208)

    at /home/mleich/10.2/storage/innobase/handler/ha_innodb.cc:5190

5190	{

(rr) 

Continuing.

[Switching to Thread 98211.100335]Thread 35 hit Breakpoint 5, innobase_close_connection (hton=0x614000001248, 

    thd=0x62a0001fe208)

    at /home/mleich/10.2/storage/innobase/handler/ha_innodb.cc:5177

5177				innobase_rollback_trx(trx);

Here, KILL is invoked slightly before the client disconnects. The innobase_close_connection() would invoke trx_free(), and this would happen before lock_trx_handle_wait_low() dereferences the trx->lock.was_chosen_as_deadlock_victim.

I believe that the correct fix is to check somewhere in innobase_kill_query() that the trx actually is registered in the system. This bug is somewhat similar to MDEV-15326.

I believe that in MariaDB 10.1 and MySQL 5.6 and earlier, this race condition between KILL and client disconnect may cause memory corruption, because free() would be invoked on the trx_t object.

A side effect of this fix is that transactions in the XA PREPARE state will be immune to the KILL statement.

Sad to see 5530a93f4. My comments are based on 10.5.

First of all it doesn't really fix the broader problem, but rather silences symptoms.

+  if (trx_t* trx= thd_to_trx(thd))

+  {

 ---- THE GAP ---

+    lock_mutex_enter();

+    trx_sys_mutex_enter();

While going through THE GAP concurrent thread may release (or even free?) trx back to trx_pool and another thread may reuse it. In effect it may break innocent transaction, even worse it may break innocent transaction of another user which it is not permitted to kill.

Secondly, trx is found via server_threads/thd->ha_data and not via trx_sys.trx_list. The latter is officially protected by trx_sys.mutex, the former is not. The fact that trx_sys.mutex helps here is pure coincidence and can in no way be considered as meaningful design.

Thirdly, the bug is in the server and not in InnoDB. Consider the following...

KILL thread:

rdlock(server_threads.rwlock);

thd= server_threads.find();

lock(thd->LOCK_thd_kill);

unlock(server_thread.rwlock);

thd->awake()/.../innobase_kill_query();

unlock(thd->LOCK_thd_kill);

InnoDB thd->ha_data is reset only upon disconnect, in ha_close_connection(). Relevant pseudo code:

wrlock(server_thread.rwlock);

server_threads.erase(thd);

unlock(server_thread.rwlock);

thd->free_connection()/ha_close_connection()/innobase_close_connection()/trx_free()

If you look carefully into the above, you can conclude that thd->free_connection() can be called concurrently with KILL/thd->awake(). Which is the bug. And it is partially fixed in THD::~THD(), that is destructor waits for KILL completion:

  /*

    Other threads may have a lock on LOCK_thd_kill to ensure that this

    THD is not deleted while they access it. The following mutex_lock

    ensures that no one else is using this THD and it's now safe to delete

  */

  if (WSREP_NNULL(this)) mysql_mutex_lock(&LOCK_thd_data);

  mysql_mutex_lock(&LOCK_thd_kill);

  mysql_mutex_unlock(&LOCK_thd_kill);

  if (WSREP_NNULL(this)) mysql_mutex_unlock(&LOCK_thd_data);

However this only works when THD is actually destroyed, whereas it must've been called when THD is released to the thread cache.

If the above empty critical section would've been moved to thd->free_connection(), the whole 5530a93f4 would not be needed.

I must revoke my first claim because:

• trx objects seem to be freed only on shutdown
• even though another trx->mysql_thd == thd binding is not impossible, it can't be established while we're holding thd->LOCK_thd_kill.

So there seem to be no practical issues with the fix, despite absence of meaningful design: the fact that KILL may not be completed up through victim THD being released to thread cache/stay cached/re-initialised for another connection sounds fragile.

svoj, sorry, I only now noticed your comments. I agree that the homebrew memory pool is a somewhat unfortunate design (which MariaDB inherited from MySQL). We might want to revise it at some point. Maybe trx_t should be a part of the InnoDB data of THD. Maybe removing unnecessary bloat from it would fix any memory allocator overhead.

I am not proud of my solution, but I did not want to touch any code outside InnoDB. Do you have a better solution in mind? (Note: MDEV-21910 may have changed this flow again, without fixing the reported issue yet.)

@marko, yes, there's better solution that I've explained in my previous comment. InnoDB code should stay intact (as if fix for this bug was not pushed) and the real problem should be fixed in the server.

I'm concerned (as far as I can be in the current situation) that trx_sys.mutex (or whatever it is called now) is abused for unrelated matters.

A healthy code that we've designed back in May (with Eugene IIRC) was supposed to encapsulate trx_sys.trx_list and trx_sys.mutex so that the latter exclusively protects the former. Now this code looks less healthy because it got infected by another sick code.

I filed MDEV-23536 for the better suggestion.