MDEV-23824 SIGSEGV in end_io_cache on REPAIR LOCAL TABLE for Aria table

Bugs fixed:
- prepare_for_repair() didn't close all open files if table opened failed
  because of out-of-memory
- If dd_recreate_table() failed, the data file was not properly restored
  from it's temporary name
- Aria repair initializing code didn't properly clear all used structs
  before calling error, which caused crashed in memory-free calls.
- maria_delete_table() didn't register if table open failed. This could
  calls my_error() to be called without returning 1 to the caller, which
  cased failures in my_ok()

Note when merging to 10.5:
- Remove the #if MYSQL_VERSION from sql_admin.cc

Author: montywi

mysql-test/suite/maria/repair.result

@@ -22,3 +22,19 @@ i
 1
 UNLOCK TABLES;
 DROP TABLE t1;
+#
+# MDEV-23824 SIGSEGV in end_io_cache on REPAIR LOCAL TABLE for Aria table
+#
+CREATE TABLE t1 (i INT) ENGINE=Aria;
+INSERT INTO t1 VALUES (1);
+SET max_session_mem_used=50000;
+REPAIR LOCAL TABLE t1 USE_FRM;
+Table	Op	Msg_type	Msg_text
+t1	repair	error	Failed to open partially repaired table
+Warnings:
+Error	1290	The MariaDB server is running with the --max-thread-mem-used=50000 option so it cannot execute this statement
+REPAIR LOCAL TABLE t1;
+Table	Op	Msg_type	Msg_text
+test.t1	repair	Error	The MariaDB server is running with the --max-thread-mem-used=50000 option so it cannot execute this statement
+test.t1	repair	error	Corrupt
+DROP TABLE t1;

mysql-test/suite/maria/repair.test

@@ -22,3 +22,14 @@ SELECT * FROM INFORMATION_SCHEMA.TABLES;
 SELECT * FROM t1;
 UNLOCK TABLES;
 DROP TABLE t1;
+
+--echo #
+--echo # MDEV-23824 SIGSEGV in end_io_cache on REPAIR LOCAL TABLE for Aria table
+--echo #
+
+CREATE TABLE t1 (i INT) ENGINE=Aria;
+INSERT INTO t1 VALUES (1);
+SET max_session_mem_used=50000;
+REPAIR LOCAL TABLE t1 USE_FRM;
+REPAIR LOCAL TABLE t1;
+DROP TABLE t1;

sql/sql_admin.cc

@@ -91,10 +91,10 @@ static int send_check_errmsg(THD *thd, TABLE_LIST* table,
 static int prepare_for_repair(THD *thd, TABLE_LIST *table_list,
 			      HA_CHECK_OPT *check_opt)
 {
-  int error= 0;
+  int error= 0, create_error= 0;
   TABLE tmp_table, *table;
   TABLE_LIST *pos_in_locked_tables= 0;
-  TABLE_SHARE *share;
+  TABLE_SHARE *share= 0;
   bool has_mdl_lock= FALSE;
   char from[FN_REFLEN],tmp[FN_REFLEN+32];
   const char **ext;
@@ -207,6 +207,23 @@ static int prepare_for_repair(THD *thd, TABLE_LIST *table_list,
                               HA_EXTRA_NOT_USED, NULL);
     table_list->table= 0;
   }
+  else
+  {
+    /*
+      Table open failed, maybe because we run out of memory.
+      Close all open tables and relaese all MDL locks
+    */
+#if MYSQL_VERSION < 100500
+    tdc_remove_table(thd, TDC_RT_REMOVE_UNUSED,
+                     table->s->db.str, table->s->table_name.str,
+                     TRUE);
+#else
+    tdc_release_share(share);
+    share->tdc->flush(thd, true);
+    share= 0;
+#endif
+  }
+
   /*
     After this point we have an exclusive metadata lock on our table
     in both cases when table was successfully open in mysql_admin_table()
@@ -220,11 +237,8 @@ static int prepare_for_repair(THD *thd, TABLE_LIST *table_list,
     goto end;
   }
   if (dd_recreate_table(thd, table_list->db.str, table_list->table_name.str))
-  {
-    error= send_check_errmsg(thd, table_list, "repair",
-			     "Failed generating table from .frm file");
-    goto end;
-  }
+    create_error= send_check_errmsg(thd, table_list, "repair",
+                                    "Failed generating table from .frm file");
   /*
     'FALSE' for 'using_transactions' means don't postpone
     invalidation till the end of a transaction, but do it
@@ -237,6 +251,8 @@ static int prepare_for_repair(THD *thd, TABLE_LIST *table_list,
 			     "Failed restoring .MYD file");
     goto end;
   }
+  if (create_error)
+    goto end;
 
   if (thd->locked_tables_list.locked_tables())
   {
@@ -264,7 +280,8 @@ static int prepare_for_repair(THD *thd, TABLE_LIST *table_list,
   if (table == &tmp_table)
   {
     closefrm(table);
-    tdc_release_share(table->s);
+    if (share)
+      tdc_release_share(share);
   }
   /* In case of a temporary table there will be no metadata lock. */
   if (unlikely(error) && has_mdl_lock)
@@ -592,6 +609,12 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables,
 #endif
     DBUG_PRINT("admin", ("table: %p", table->table));
 
+    if (table->schema_table)
+    {
+      result_code= HA_ADMIN_NOT_IMPLEMENTED;
+      goto send_result;
+    }
+
     if (prepare_func)
     {
       DBUG_PRINT("admin", ("calling prepare_func"));
@@ -650,12 +673,6 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables,
       goto send_result;
     }
 
-    if (table->schema_table)
-    {
-      result_code= HA_ADMIN_NOT_IMPLEMENTED;
-      goto send_result;
-    }
-
     if ((table->table->db_stat & HA_READ_ONLY) && open_for_modify)
     {
       /* purecov: begin inspected */

storage/maria/ma_check.c

@@ -2344,6 +2344,14 @@ static int initialize_variables_for_repair(HA_CHECK *param,
 {
   MARIA_SHARE *share= info->s;
 
+  /*
+    We have to clear these variables first, as the cleanup-in-case-of-error
+    handling may touch these.
+  */
+  bzero((char*) sort_info,  sizeof(*sort_info));
+  bzero((char*) sort_param, sizeof(*sort_param));
+  bzero(&info->rec_cache, sizeof(info->rec_cache));
+
   if (share->data_file_type == NO_RECORD)
   {
     _ma_check_print_error(param,
@@ -2358,9 +2366,6 @@ static int initialize_variables_for_repair(HA_CHECK *param,
   if (share->lock.update_status)
     (*share->lock.update_status)(info);
 
-  bzero((char*) sort_info,  sizeof(*sort_info));
-  bzero((char*) sort_param, sizeof(*sort_param));
-
   param->testflag|= T_REP;                     /* for easy checking */
   if (share->options & (HA_OPTION_CHECKSUM | HA_OPTION_COMPRESS_RECORD))
     param->testflag|= T_CALC_CHECKSUM;
@@ -2388,7 +2393,6 @@ static int initialize_variables_for_repair(HA_CHECK *param,
   set_data_file_type(sort_info, info->s);
   sort_info->org_data_file_type= share->data_file_type;
 
-  bzero(&info->rec_cache, sizeof(info->rec_cache));
   info->rec_cache.file= info->dfile.file;
   info->update= (short) (HA_STATE_CHANGED | HA_STATE_ROW_CHANGED);
 
@@ -2869,9 +2873,13 @@ int maria_repair(HA_CHECK *param, register MARIA_HA *info,
   _ma_reset_state(info);
 
   end_io_cache(&param->read_cache);
-  end_io_cache(&sort_info.new_info->rec_cache);
+  if (sort_info.new_info)
+  {
+    end_io_cache(&sort_info.new_info->rec_cache);
+    sort_info.new_info->opt_flag&= ~(READ_CACHE_USED | WRITE_CACHE_USED);
+  }
   info->opt_flag&= ~(READ_CACHE_USED | WRITE_CACHE_USED);
-  sort_info.new_info->opt_flag&= ~(READ_CACHE_USED | WRITE_CACHE_USED);
+
   sort_param.sort_info->info->in_check_table= 0;
   /* this below could fail, shouldn't we detect error? */
   if (got_error)
@@ -4086,10 +4094,13 @@ int maria_repair_by_sort(HA_CHECK *param, register MARIA_HA *info,
     maria_scan_end(sort_info.info);
   _ma_reset_state(info);
 
-  end_io_cache(&sort_info.new_info->rec_cache);
+  if (sort_info.new_info)
+  {
+    end_io_cache(&sort_info.new_info->rec_cache);
+    sort_info.new_info->opt_flag&= ~(READ_CACHE_USED | WRITE_CACHE_USED);
+  }
   end_io_cache(&param->read_cache);
   info->opt_flag&= ~(READ_CACHE_USED | WRITE_CACHE_USED);
-  sort_info.new_info->opt_flag&= ~(READ_CACHE_USED | WRITE_CACHE_USED);
   if (got_error)
   {
     if (! param->error_printed)
@@ -4618,10 +4629,13 @@ int maria_repair_parallel(HA_CHECK *param, register MARIA_HA *info,
     the share by remove_io_thread() or it was not yet started (if the
     error happend before creating the thread).
   */
-  end_io_cache(&sort_info.new_info->rec_cache);
+  if (sort_info.new_info)
+  {
+    end_io_cache(&sort_info.new_info->rec_cache);
+    sort_info.new_info->opt_flag&= ~(READ_CACHE_USED | WRITE_CACHE_USED);
+  }
   end_io_cache(&param->read_cache);
   info->opt_flag&= ~(READ_CACHE_USED | WRITE_CACHE_USED);
-  sort_info.new_info->opt_flag&= ~(READ_CACHE_USED | WRITE_CACHE_USED);
   /*
     Destroy the new data cache in case of non-quick repair. All slave
     threads did either detach from the share by remove_io_thread()

storage/maria/ma_delete_table.c

@@ -30,6 +30,7 @@ int maria_delete_table(const char *name)
 {
   MARIA_HA *info;
   myf sync_dir;
+  int got_error= 0, error;
   DBUG_ENTER("maria_delete_table");
 
 #ifdef EXTRA_DEBUG
@@ -41,9 +42,13 @@ int maria_delete_table(const char *name)
     Unfortunately it is necessary to open the table just to check this. We use
     'open_for_repair' to be able to open even a crashed table.
   */
+  my_errno= 0;
   if (!(info= maria_open(name, O_RDONLY, HA_OPEN_FOR_REPAIR)))
   {
     sync_dir= 0;
+    /* Ignore not found errors and wrong symlink errors */
+    if (my_errno != ENOENT && my_errno != HA_WRONG_CREATE_OPTION)
+      got_error= my_errno;;
   }
   else
   {
@@ -78,7 +83,9 @@ int maria_delete_table(const char *name)
       DBUG_RETURN(1);
   }
 
-  DBUG_RETURN(maria_delete_table_files(name, 0, sync_dir));
+  if (!(error= maria_delete_table_files(name, 0, sync_dir)))
+    error= got_error;
+  DBUG_RETURN(error);
 }
 
 

storage/maria/ma_open.c

@@ -1334,7 +1334,7 @@ static void setup_key_functions(register MARIA_KEYDEF *keyinfo)
 
 
 /**
-   @brief Function to save and store the header in the index file (.MYI)
+   @brief Function to save and store the header in the index file (.MAI)
 
    Operates under MARIA_SHARE::intern_lock if requested.
    Sets MARIA_SHARE::MARIA_STATE_INFO::is_of_horizon if transactional table.