MDEV-16986 Unitialized mutex, SIGSEGV and assorted assertion failures in Aria code

This was introduced by two pointers I added to TRN
as part of MDEV-16421 Make system tables crash safe

- Added code to ensure that trn_prev is not pointing
  to wrong object
- A lot of new asserts and more code comments
- Simplified code in _ma_trnman_end_trans_hook()
  - New back link allowed me to remove a loop

5 files changed, 66 insertions, 29 deletions

diff --git a/storage/maria/ha_maria.cc b/storage/maria/ha_maria.cc
index 6a1a70702b9..e91074d176a 100644
--- a/storage/maria/ha_maria.cc
+++ b/storage/maria/ha_maria.cc
@@ -1006,6 +1006,8 @@ handler *ha_maria::clone(const char *name, MEM_ROOT *mem_root)
     new_handler->file->state= file->state;
     /* maria_create_trn_for_mysql() is never called for clone() tables */
     new_handler->file->trn= file->trn;
+    DBUG_ASSERT(new_handler->file->trn_prev == 0 &&
+                new_handler->file->trn_next == 0);
   }
   return new_handler;
 }
@@ -1272,6 +1274,7 @@ int ha_maria::close(void)
   if (!tmp)
     return 0;
   DBUG_ASSERT(file->trn == 0 || file->trn == &dummy_transaction_object);
+  DBUG_ASSERT(file->trn_next == 0 && file->trn_prev == 0);
   file= 0;
   return maria_close(tmp);
 }
@@ -1397,7 +1400,10 @@ int ha_maria::check(THD * thd, HA_CHECK_OPT * check_opt)
 
   /* Reset trn, that may have been set by repair */
   if (old_trn && old_trn != file->trn)
+  {
+    DBUG_ASSERT(old_trn->used_instances == 0);
     _ma_set_trn_for_table(file, old_trn);
+  }
   thd_proc_info(thd, old_proc_info);
   thd_progress_end(thd);
   return error ? HA_ADMIN_CORRUPT : HA_ADMIN_OK;
@@ -2613,14 +2619,20 @@ int ha_maria::extra(enum ha_extra_function operation)
        operation == HA_EXTRA_PREPARE_FOR_FORCED_CLOSE))
   {
     THD *thd= table->in_use;
-    TRN *trn= THD_TRN;
-    _ma_set_tmp_trn_for_table(file, trn);
+    file->trn= THD_TRN;
   }
   DBUG_ASSERT(file->s->base.born_transactional || file->trn == 0 ||
               file->trn == &dummy_transaction_object);
 
   tmp= maria_extra(file, operation, 0);
-  file->trn= old_trn;                           // Reset trn if was used
+  /*
+    Restore trn if it was changed above.
+    Note that table could be removed from trn->used_tables and
+    trn->used_instances if trn was set and some of the above operations
+    was used. This is ok as the table should not be part of any transaction
+    after this and thus doesn't need to be part of any of the above lists.
+  */
+  file->trn= old_trn;
   return tmp;
 }
 
@@ -2856,9 +2868,12 @@ static void reset_thd_trn(THD *thd, MARIA_HA *first_table)
 {
   DBUG_ENTER("reset_thd_trn");
   THD_TRN= NULL;
-  for (MARIA_HA *table= first_table; table ;
-       table= table->trn_next)
+  MARIA_HA *next;
+  for (MARIA_HA *table= first_table; table ; table= next)
+  {
+    next= table->trn_next;
     _ma_reset_trn_for_table(table);
+  }
   DBUG_VOID_RETURN;
 }
 
@@ -2905,9 +2920,11 @@ int ha_maria::implicit_commit(THD *thd, bool new_trn)
     DBUG_RETURN(0);
   }
 
+  /* Prepare to move used_instances and locked tables to new TRN object */
   locked_tables= trnman_has_locked_tables(trn);
+  trnman_reset_locked_tables(trn, 0);
+  relink_trn_used_instances(&used_tables, trn);
 
-  used_tables= (MARIA_HA*) trn->used_instances;
   error= 0;
   if (unlikely(ma_commit(trn)))
     error= 1;
@@ -3332,6 +3349,8 @@ static int maria_commit(handlerton *hton __attribute__ ((unused)),
 {
   TRN *trn= THD_TRN;
   DBUG_ENTER("maria_commit");
+
+  DBUG_ASSERT(trnman_has_locked_tables(trn) == 0);
   trnman_reset_locked_tables(trn, 0);
   trnman_set_flags(trn, trnman_get_flags(trn) & ~TRN_STATE_INFO_LOGGED);
 
@@ -3349,9 +3368,12 @@ static int maria_rollback(handlerton *hton __attribute__ ((unused)),
 {
   TRN *trn= THD_TRN;
   DBUG_ENTER("maria_rollback");
+
+  DBUG_ASSERT(trnman_has_locked_tables(trn) == 0);
   trnman_reset_locked_tables(trn, 0);
   /* statement or transaction ? */
-  if ((thd->variables.option_bits & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)) && !all)
+  if ((thd->variables.option_bits & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)) &&
+      !all)
   {
     trnman_rollback_statement(trn);
     DBUG_RETURN(0); // end of statement
diff --git a/storage/maria/ma_commit.c b/storage/maria/ma_commit.c
index 0ae3868dbf6..f557cd211e2 100644
--- a/storage/maria/ma_commit.c
+++ b/storage/maria/ma_commit.c
@@ -98,7 +98,12 @@ int maria_commit(MARIA_HA *info)
   if (!info->s->now_transactional)
     return 0;
   trn= info->trn;
-  info->trn= 0;                                 /* checked in maria_close() */
+  /*
+    trn is reset as it's checked in maria_close
+    Note that info is still linked in info->trn->used_instances, as this is
+    used in ha_maria::implicit_commit()
+  */
+  info->trn= 0;
   return ma_commit(trn);
 }
 
diff --git a/storage/maria/ma_state.c b/storage/maria/ma_state.c
index 23cb625fc58..c658b9e667c 100644
--- a/storage/maria/ma_state.c
+++ b/storage/maria/ma_state.c
@@ -30,6 +30,7 @@
 
 #include "maria_def.h"
 #include "trnman.h"
+#include "ma_trnman.h"
 #include "ma_blockrec.h"
 
 /**
@@ -571,7 +572,6 @@ void _ma_remove_table_from_trnman(MARIA_HA *info)
   MARIA_SHARE *share= info->s;
   TRN *trn= info->trn;
   MARIA_USED_TABLES *tables, **prev;
-  MARIA_HA *handler, **prev_file;
   DBUG_ENTER("_ma_remove_table_from_trnman");
   DBUG_PRINT("enter", ("trn: %p  used_tables: %p  share: %p  in_trans: %d",
                        trn, trn->used_tables, share, share->in_trans));
@@ -603,26 +603,9 @@ void _ma_remove_table_from_trnman(MARIA_HA *info)
     DBUG_PRINT("warning", ("share: %p where not in used_tables_list", share));
   }
 
-  /* unlink table from used_instances */
-  for (prev_file= (MARIA_HA**) &trn->used_instances;
-       (handler= *prev_file);
-       prev_file= &handler->trn_next)
-  {
-    if (handler == info)
-    {
-      *prev_file= info->trn_next;
-      break;
-    }
-  }
-  if (handler != 0)
-  {
-    /*
-      This can only happens in case of rename of intermediate table as
-      part of alter table
-    */
-    DBUG_PRINT("warning", ("table: %p where not in used_instances", info));
-  }
-  info->trn= 0;                                 /* Not part of trans anymore */
+  /* Reset trn and remove table from used_instances */
+  _ma_reset_trn_for_table(info);
+
   DBUG_VOID_RETURN;
 }
 
diff --git a/storage/maria/ma_trnman.h b/storage/maria/ma_trnman.h
index 767d00ccccc..5b6d0e9f60d 100644
--- a/storage/maria/ma_trnman.h
+++ b/storage/maria/ma_trnman.h
@@ -53,6 +53,7 @@ static inline void _ma_set_tmp_trn_for_table(MARIA_HA *tbl, TRN *newtrn)
                      tbl, tbl->trn, newtrn));
   tbl->trn= newtrn;
   tbl->trn_prev= 0;
+  tbl->trn_next= 0;               /* To avoid assert in ha_maria::close() */
 }
 
 
@@ -63,13 +64,36 @@ static inline void _ma_set_tmp_trn_for_table(MARIA_HA *tbl, TRN *newtrn)
 static inline void _ma_reset_trn_for_table(MARIA_HA *tbl)
 {
   DBUG_PRINT("info",("table: %p  trn: %p -> NULL", tbl, tbl->trn));
+
   /* The following is only false if tbl->trn == &dummy_transaction_object */
   if (tbl->trn_prev)
   {
+    if (tbl->trn_next)
+      tbl->trn_next->trn_prev= tbl->trn_prev;
     *tbl->trn_prev= tbl->trn_next;
     tbl->trn_prev= 0;
+    tbl->trn_next= 0;
   }
   tbl->trn= 0;
 }
 
+
+/*
+  Take over the used_instances link from a trn object
+  Reset the link in the trn object
+*/
+
+static inline void relink_trn_used_instances(MARIA_HA **used_tables, TRN *trn)
+{
+  if (likely(*used_tables= (MARIA_HA*) trn->used_instances))
+  {
+    /* Check that first back link is correct */
+    DBUG_ASSERT((*used_tables)->trn_prev == (MARIA_HA **)&trn->used_instances);
+
+    /* Fix back link to point to new base for the list */
+    (*used_tables)->trn_prev= used_tables;
+    trn->used_instances= 0;
+  }
+}
+
 #endif /* _ma_trnman_h */
diff --git a/storage/maria/trnman.c b/storage/maria/trnman.c
index 5b3c9f0287a..3c5ce831f95 100644
--- a/storage/maria/trnman.c
+++ b/storage/maria/trnman.c
@@ -413,6 +413,7 @@ my_bool trnman_end_trn(TRN *trn, my_bool commit)
   /* if a rollback, all UNDO records should have been executed */
   DBUG_ASSERT(commit || trn->undo_lsn == 0);
   DBUG_ASSERT(trn != &dummy_transaction_object);
+  DBUG_ASSERT(trn->locked_tables == 0 && trn->used_instances == 0);
   DBUG_PRINT("info", ("mysql_mutex_lock LOCK_trn_list"));
 
   mysql_mutex_lock(&LOCK_trn_list);
@@ -529,6 +530,8 @@ static void trnman_free_trn(TRN *trn)
   */
   union { TRN *trn; void *v; } tmp;
 
+  DBUG_ASSERT(trn != &dummy_transaction_object);
+
   mysql_mutex_lock(&trn->state_lock);
   trn->short_id= 0;
   mysql_mutex_unlock(&trn->state_lock);