MDEV-25524 heap-use-after-free in fil_space_t::rename()

In commit 91599701 (MDEV-25312)
some recovery code for TRUNCATE TABLE was broken
causing a regression in a case where undo log for a RENAME TABLE
operation had been durably written but the tablespace had not been
renamed yet.

row_rename_table_for_mysql(): Add a DEBUG_SYNC point for the
test case, and simplify the logic and trim the error messages.

fil_space_t::rename(): Simplify the operation. Merge the necessary
part of fil_rename_tablespace_check(). If there is no change to
the file name, do nothing.

dict_table_t::rename_tablespace(): Refactored from
dict_table_rename_in_cache().

row_undo_ins_parse_undo_rec(): On rolling back TRX_UNDO_RENAME_TABLE,
invoke dict_table_t::rename_tablespace() even if the table name matches.

os_file_rename_func(): Temporarily relax an assertion that would
fail during the recovery in the test innodb.truncate_crash.

mysql-test/suite/innodb/r/rename_table_debug.result

@@ -11,4 +11,21 @@ disconnect con1;
 SELECT * FROM t1;
 a	b	c	d
 1	NULL	NULL	NULL
-DROP TABLE t1;
+CREATE TABLE t2 (a INT PRIMARY KEY) ENGINE=InnoDB;
+BEGIN;
+INSERT INTO t2 VALUES(1);
+connect  con1,localhost,root,,test;
+SET DEBUG_SYNC='innodb_rename_in_cache SIGNAL committed WAIT_FOR ever';
+RENAME TABLE t1 TO t3;
+connection default;
+SET DEBUG_SYNC='now WAIT_FOR committed';
+COMMIT;
+# restart
+disconnect con1;
+SELECT * FROM t1;
+a	b	c	d
+1	NULL	NULL	NULL
+SELECT * FROM t2;
+a
+1
+DROP TABLE t1,t2;

mysql-test/suite/innodb/t/alter_rename_existing.test

@@ -88,6 +88,6 @@ SELECT * from t1;
 DROP TABLE t1;
 
 --disable_query_log
-call mtr.add_suppression("\\[ERROR\\] InnoDB: Cannot rename '.*' to '.*' because the target file exists. Remove the target file and try again");
+call mtr.add_suppression("\\[ERROR\\] InnoDB: Cannot rename '.*' to '.*' because the target file exists");
 SET GLOBAL innodb_file_per_table = @old_innodb_file_per_table;
 --enable_query_log

mysql-test/suite/innodb/t/foreign_key.test

@@ -148,9 +148,7 @@ SET FOREIGN_KEY_CHECKS=1;
 --echo #
 
 --disable_query_log
-call mtr.add_suppression("InnoDB: Possible reasons:");
-call mtr.add_suppression("InnoDB: \\([12]\\) Table ");
-call mtr.add_suppression("InnoDB: If table `test`\\.`t2` is a temporary table");
+call mtr.add_suppression("InnoDB: Table rename might cause two FOREIGN KEY");
 call mtr.add_suppression("InnoDB: Cannot delete/update rows with cascading foreign key constraints that exceed max depth of 15\\.");
 --enable_query_log
 

mysql-test/suite/innodb/t/rename_table_debug.test

@@ -19,4 +19,23 @@ SET DEBUG_SYNC='now WAIT_FOR renamed';
 --source include/restart_mysqld.inc
 --disconnect con1
 SELECT * FROM t1;
-DROP TABLE t1;
+
+CREATE TABLE t2 (a INT PRIMARY KEY) ENGINE=InnoDB;
+BEGIN;
+INSERT INTO t2 VALUES(1);
+
+--connect (con1,localhost,root,,test)
+SET DEBUG_SYNC='innodb_rename_in_cache SIGNAL committed WAIT_FOR ever';
+--send
+RENAME TABLE t1 TO t3;
+--connection default
+SET DEBUG_SYNC='now WAIT_FOR committed';
+COMMIT;
+
+--let $shutdown_timeout=0
+--source include/restart_mysqld.inc
+--disconnect con1
+SELECT * FROM t1;
+SELECT * FROM t2;
+
+DROP TABLE t1,t2;

storage/innobase/dict/dict0dict.cc

@@ -1543,6 +1543,66 @@ struct dict_foreign_remove_partial
 	}
 };
 
+/** Rename the data file.
+@param new_name     name of the table
+@param replace      whether to replace the file with the new name
+                    (as part of rolling back TRUNCATE) */
+dberr_t
+dict_table_t::rename_tablespace(const char *new_name, bool replace) const
+{
+  ut_ad(dict_table_is_file_per_table(this));
+  ut_ad(!is_temporary());
+
+  if (!space)
+  {
+    const char *data_dir= DICT_TF_HAS_DATA_DIR(flags)
+      ? data_dir_path : nullptr;
+    ut_ad(data_dir || !DICT_TF_HAS_DATA_DIR(flags));
+
+    if (char *filepath= fil_make_filepath(data_dir, name, IBD,
+                                          data_dir != nullptr))
+    {
+      fil_delete_tablespace(space_id, true);
+      os_file_type_t ftype;
+      bool exists;
+      /* Delete any temp file hanging around. */
+      if (os_file_status(filepath, &exists, &ftype) && exists &&
+          !os_file_delete_if_exists(innodb_temp_file_key, filepath, nullptr))
+        ib::info() << "Delete of " << filepath << " failed.";
+      ut_free(filepath);
+    }
+    return DB_SUCCESS;
+  }
+
+  const char *old_path= UT_LIST_GET_FIRST(space->chain)->name;
+  fil_space_t::name_type space_name{new_name, strlen(new_name)};
+  const bool data_dir= DICT_TF_HAS_DATA_DIR(flags);
+  char *path= data_dir
+    ? os_file_make_new_pathname(old_path, new_name)
+    : fil_make_filepath(nullptr, space_name, IBD, false);
+  dberr_t err;
+  if (!path)
+    err= DB_OUT_OF_MEMORY;
+  else if (!strcmp(path, old_path))
+    err= DB_SUCCESS;
+  else if (data_dir &&
+           DB_SUCCESS != RemoteDatafile::create_link_file(space_name, path))
+    err= DB_TABLESPACE_EXISTS;
+  else
+  {
+    err= space->rename(path, true, replace);
+    if (data_dir)
+    {
+      if (err == DB_SUCCESS)
+        space_name= {name.m_name, strlen(name.m_name)};
+      RemoteDatafile::delete_link_file(space_name);
+    }
+  }
+
+  ut_free(path);
+  return err;
+}
+
 /**********************************************************************//**
 Renames a table object.
 @return TRUE if success */
@@ -1560,11 +1620,9 @@ dict_table_rename_in_cache(
 					file with the new name
 					(as part of rolling back TRUNCATE) */
 {
-	dberr_t		err;
 	dict_foreign_t*	foreign;
 	ulint		fold;
 	char		old_name[MAX_FULL_NAME_LEN + 1];
-	os_file_type_t	ftype;
 
 	dict_sys.assert_locked();
 
@@ -1590,88 +1648,10 @@ dict_table_rename_in_cache(
 		return(DB_ERROR);
 	}
 
-	/* If the table is stored in a single-table tablespace, rename the
-	.ibd file and rebuild the .isl file if needed. */
-
-	if (!table->space) {
-		bool		exists;
-		char*		filepath;
-
-		ut_ad(dict_table_is_file_per_table(table));
-		ut_ad(!table->is_temporary());
-
-		/* Make sure the data_dir_path is set. */
-		dict_get_and_save_data_dir_path(table, true);
-
-		const char* data_dir = DICT_TF_HAS_DATA_DIR(table->flags)
-			? table->data_dir_path : nullptr;
-		ut_ad(data_dir || !DICT_TF_HAS_DATA_DIR(table->flags));
-
-		filepath = fil_make_filepath(data_dir, table->name, IBD,
-					     data_dir != nullptr);
-
-		if (filepath == NULL) {
-			return(DB_OUT_OF_MEMORY);
-		}
-
-		fil_delete_tablespace(table->space_id, !table->space);
-
-		/* Delete any temp file hanging around. */
-		if (os_file_status(filepath, &exists, &ftype)
-		    && exists
-		    && !os_file_delete_if_exists(innodb_temp_file_key,
-						 filepath, NULL)) {
-
-			ib::info() << "Delete of " << filepath << " failed.";
-		}
-		ut_free(filepath);
-
-	} else if (dict_table_is_file_per_table(table)) {
-		char*	new_path;
-		const char* old_path = UT_LIST_GET_FIRST(table->space->chain)
-			->name;
-
-		ut_ad(!table->is_temporary());
-
-		const fil_space_t::name_type new_space_name{
-			new_name, strlen(new_name)};
-
-		if (DICT_TF_HAS_DATA_DIR(table->flags)) {
-			new_path = os_file_make_new_pathname(
-				old_path, new_name);
-			err = RemoteDatafile::create_link_file(
-				new_space_name, new_path);
-
-			if (err != DB_SUCCESS) {
-				ut_free(new_path);
-				return(DB_TABLESPACE_EXISTS);
-			}
-		} else {
-			new_path = fil_make_filepath(
-				NULL, new_space_name, IBD, false);
-		}
-
-		/* New filepath must not exist. */
-		err = table->space->rename(new_path, true, replace_new_file);
-		ut_free(new_path);
-
-		/* If the tablespace is remote, a new .isl file was created
-		If success, delete the old one. If not, delete the new one. */
-
-		if (err != DB_SUCCESS) {
-			if (DICT_TF_HAS_DATA_DIR(table->flags)) {
-				RemoteDatafile::delete_link_file(
-					new_space_name);
-			}
-
-			return err;
-		}
-
-		if (DICT_TF_HAS_DATA_DIR(table->flags)) {
-			RemoteDatafile::delete_link_file(
-				{old_name, strlen(old_name)});
-		}
-
+	if (!dict_table_is_file_per_table(table)) {
+	} else if (dberr_t err = table->rename_tablespace(new_name,
+							  replace_new_file)) {
+		return err;
 	}
 
 	/* Remove table from the hash tables of tables */

storage/innobase/fil/fil0fil.cc

@@ -1867,88 +1867,41 @@ char *fil_make_filepath(const char* path, const table_name_t name,
                            suffix, strip_name);
 }
 
-/** Test if a tablespace file can be renamed to a new filepath by checking
-if that the old filepath exists and the new filepath does not exist.
-@param[in]	old_path	old filepath
-@param[in]	new_path	new filepath
-@param[in]	replace_new	whether to ignore the existence of new_path
-@return innodb error code */
-static dberr_t
-fil_rename_tablespace_check(
-	const char*	old_path,
-	const char*	new_path,
-	bool		replace_new)
+dberr_t fil_space_t::rename(const char *path, bool log, bool replace)
 {
-	bool	exists = false;
-	os_file_type_t	ftype;
-
-	if (os_file_status(old_path, &exists, &ftype) && !exists) {
-		ib::error() << "Cannot rename '" << old_path
-			<< "' to '" << new_path
-			<< "' because the source file"
-			<< " does not exist.";
-		return(DB_TABLESPACE_NOT_FOUND);
-	}
+  ut_ad(UT_LIST_GET_LEN(chain) == 1);
+  ut_ad(!is_system_tablespace(id));
 
-	exists = false;
-	if (os_file_status(new_path, &exists, &ftype) && !exists) {
-		return DB_SUCCESS;
-	}
+  const char *old_path= chain.start->name;
 
-	if (!replace_new) {
-		ib::error() << "Cannot rename '" << old_path
-			<< "' to '" << new_path
-			<< "' because the target file exists."
-			" Remove the target file and try again.";
-		return(DB_TABLESPACE_EXISTS);
-	}
+  if (!strcmp(path, old_path))
+    return DB_SUCCESS;
 
-	/* This must be during the ROLLBACK of TRUNCATE TABLE.
-	Because InnoDB only allows at most one data dictionary
-	transaction at a time, and because this incomplete TRUNCATE
-	would have created a new tablespace file, we must remove
-	a possibly existing tablespace that is associated with the
-	new tablespace file. */
-retry:
-	mysql_mutex_lock(&fil_system.mutex);
-	for (fil_space_t& space : fil_system.space_list) {
-		ulint id = space.id;
-		if (id
-		    && space.purpose == FIL_TYPE_TABLESPACE
-		    && !strcmp(new_path,
-			       UT_LIST_GET_FIRST(space.chain)->name)) {
-			ib::info() << "TRUNCATE rollback: " << id
-				<< "," << new_path;
-			mysql_mutex_unlock(&fil_system.mutex);
-			dberr_t err = fil_delete_tablespace(id);
-			if (err != DB_SUCCESS) {
-				return err;
-			}
-			goto retry;
-		}
-	}
-	mysql_mutex_unlock(&fil_system.mutex);
-	fil_delete_file(new_path);
+  if (log)
+  {
+    bool exists= false;
+    os_file_type_t ftype;
 
-	return(DB_SUCCESS);
-}
+    if (os_file_status(old_path, &exists, &ftype) && !exists)
+    {
+      ib::error() << "Cannot rename '" << old_path << "' to '" << path
+                  << "' because the source file does not exist.";
+      return DB_TABLESPACE_NOT_FOUND;
+    }
 
-dberr_t fil_space_t::rename(const char* path, bool log, bool replace)
-{
-	ut_ad(UT_LIST_GET_LEN(chain) == 1);
-	ut_ad(!is_system_tablespace(id));
+    exists= false;
+    if (replace);
+    else if (!os_file_status(path, &exists, &ftype) || exists)
+    {
+      ib::error() << "Cannot rename '" << old_path << "' to '" << path
+                  << "' because the target file exists.";
+      return DB_TABLESPACE_EXISTS;
+    }
 
-	if (log) {
-		dberr_t err = fil_rename_tablespace_check(
-			chain.start->name, path, replace);
-		if (err != DB_SUCCESS) {
-			return(err);
-		}
-		fil_name_write_rename(id, chain.start->name, path);
-	}
+    fil_name_write_rename(id, old_path, path);
+  }
 
-	return fil_rename_tablespace(id, chain.start->name, path)
-		? DB_SUCCESS : DB_ERROR;
+  return fil_rename_tablespace(id, old_path, path) ? DB_SUCCESS : DB_ERROR;
 }
 
 /** Rename a single-table tablespace.

storage/innobase/include/dict0mem.h

@@ -2045,6 +2045,12 @@ struct dict_table_t {
   void stats_mutex_lock() { lock_mutex_lock(); }
   void stats_mutex_unlock() { lock_mutex_unlock(); }
 
+  /** Rename the data file.
+  @param new_name     name of the table
+  @param replace      whether to replace the file with the new name
+                      (as part of rolling back TRUNCATE) */
+  dberr_t rename_tablespace(const char *new_name, bool replace) const;
+
 private:
 	/** Initialize instant->field_map.
 	@param[in]	table	table definition to copy from */

storage/innobase/os/os0file.cc

@@ -1622,7 +1622,8 @@ os_file_rename_func(
 
 	/* New path must not exist. */
 	ut_ad(os_file_status(newpath, &exists, &type));
-	ut_ad(!exists);
+	/* MDEV-25506 FIXME: Remove the strstr() */
+	ut_ad(!exists || strstr(oldpath, "/" TEMP_FILE_PREFIX_INNODB));
 
 	/* Old path must exist. */
 	ut_ad(os_file_status(oldpath, &exists, &type));

storage/innobase/row/row0mysql.cc

@@ -4093,7 +4093,6 @@ row_rename_table_for_mysql(
 	ibool		old_is_tmp, new_is_tmp;
 	pars_info_t*	info			= NULL;
 	int		retry;
-	bool		aux_fts_rename		= false;
 	char*		is_part 		= NULL;
 
 	ut_a(old_name != NULL);
@@ -4253,7 +4252,7 @@ row_rename_table_for_mysql(
 	if (err != DB_SUCCESS) {
 		// Assume the caller guarantees destination name doesn't exist.
 		ut_ad(err != DB_DUPLICATE_KEY);
-		goto err_exit;
+		goto rollback_and_exit;
 	}
 
 	if (!new_is_tmp) {
@@ -4392,54 +4391,28 @@ row_rename_table_for_mysql(
 	    || DICT_TF2_FLAG_IS_SET(table, DICT_TF2_FTS_HAS_DOC_ID))
 	    && !dict_tables_have_same_db(old_name, new_name)) {
 		err = fts_rename_aux_tables(table, new_name, trx);
-		if (err != DB_TABLE_NOT_FOUND) {
-			aux_fts_rename = true;
-		}
 	}
 
-	if (err != DB_SUCCESS) {
-err_exit:
-		if (err == DB_DUPLICATE_KEY) {
-			ib::error() << "Possible reasons:";
-			ib::error() << "(1) Table rename would cause two"
-				" FOREIGN KEY constraints to have the same"
-				" internal name in case-insensitive"
-				" comparison.";
-			ib::error() << "(2) Table "
-				<< ut_get_name(trx, new_name)
-				<< " exists in the InnoDB internal data"
-				" dictionary though MySQL is trying to rename"
-				" table " << ut_get_name(trx, old_name)
-				<< " to it. Have you deleted the .frm file and"
-				" not used DROP TABLE?";
-			ib::info() << TROUBLESHOOTING_MSG;
-			ib::error() << "If table "
-				<< ut_get_name(trx, new_name)
-				<< " is a temporary table #sql..., then"
-				" it can be that there are still queries"
-				" running on the table, and it will be dropped"
-				" automatically when the queries end. You can"
-				" drop the orphaned table inside InnoDB by"
-				" creating an InnoDB table with the same name"
-				" in another database and copying the .frm file"
-				" to the current database. Then MySQL thinks"
-				" the table exists, and DROP TABLE will"
-				" succeed.";
-		}
+	switch (err) {
+	case DB_DUPLICATE_KEY:
+		ib::error() << "Table rename might cause two"
+			" FOREIGN KEY constraints to have the same"
+			" internal name in case-insensitive comparison.";
+		ib::info() << TROUBLESHOOTING_MSG;
+		/* fall through */
+	rollback_and_exit:
+	default:
 		trx->error_state = DB_SUCCESS;
 		trx->rollback();
 		trx->error_state = DB_SUCCESS;
-	} else {
-		/* The following call will also rename the .ibd data file if
-		the table is stored in a single-table tablespace */
-
+		break;
+	case DB_SUCCESS:
+		DEBUG_SYNC_C("innodb_rename_in_cache");
+		/* The following call will also rename the .ibd file */
 		err = dict_table_rename_in_cache(
 			table, new_name, !new_is_tmp);
 		if (err != DB_SUCCESS) {
-			trx->error_state = DB_SUCCESS;
-			trx->rollback();
-			trx->error_state = DB_SUCCESS;
-			goto funct_exit;
+			goto rollback_and_exit;
 		}
 
 		/* In case of copy alter, template db_name and
@@ -4462,7 +4435,6 @@ row_rename_table_for_mysql(
 			fk_tables);
 
 		if (err != DB_SUCCESS) {
-
 			if (old_is_tmp) {
 				/* In case of copy alter, ignore the
 				loading of foreign key constraint
@@ -4476,7 +4448,7 @@ row_rename_table_for_mysql(
 					" definition.";
 				if (!trx->check_foreigns) {
 					err = DB_SUCCESS;
-					goto funct_exit;
+					break;
 				}
 			} else {
 				ib::error() << "In RENAME TABLE table "
@@ -4486,22 +4458,14 @@ row_rename_table_for_mysql(
 					" with the new table definition.";
 			}
 
-			trx->error_state = DB_SUCCESS;
-			trx->rollback();
-			trx->error_state = DB_SUCCESS;
+			goto rollback_and_exit;
 		}
 
 		/* Check whether virtual column or stored column affects
 		the foreign key constraint of the table. */
-		if (dict_foreigns_has_s_base_col(
-				table->foreign_set, table)) {
+		if (dict_foreigns_has_s_base_col(table->foreign_set, table)) {
 			err = DB_NO_FK_ON_S_BASE_COL;
-			ut_a(DB_SUCCESS == dict_table_rename_in_cache(
-				table, old_name, FALSE));
-			trx->error_state = DB_SUCCESS;
-			trx->rollback();
-			trx->error_state = DB_SUCCESS;
-			goto funct_exit;
+			goto rollback_and_exit;
 		}
 
 		/* Fill the virtual column set in foreign when
@@ -4519,37 +4483,6 @@ row_rename_table_for_mysql(
 	}
 
 funct_exit:
-	if (aux_fts_rename && err != DB_SUCCESS
-	    && table != NULL && (table->space != 0)) {
-
-		char*	orig_name = table->name.m_name;
-		trx_t*	trx_bg = trx_create();
-
-		/* If the first fts_rename fails, the trx would
-		be rolled back and committed, we can't use it any more,
-		so we have to start a new background trx here. */
-		ut_a(trx_state_eq(trx_bg, TRX_STATE_NOT_STARTED));
-		trx_bg->op_info = "Revert the failing rename "
-				  "for fts aux tables";
-		trx_bg->dict_operation_lock_mode = RW_X_LATCH;
-		trx_start_for_ddl(trx_bg, TRX_DICT_OP_TABLE);
-
-		/* If rename fails and table has its own tablespace,
-		we need to call fts_rename_aux_tables again to
-		revert the ibd file rename, which is not under the
-		control of trx. Also notice the parent table name
-		in cache is not changed yet. If the reverting fails,
-		the ibd data may be left in the new database, which
-		can be fixed only manually. */
-		table->name.m_name = const_cast<char*>(new_name);
-		fts_rename_aux_tables(table, old_name, trx_bg);
-		table->name.m_name = orig_name;
-
-		trx_bg->dict_operation_lock_mode = 0;
-		trx_commit_for_mysql(trx_bg);
-		trx_bg->free();
-	}
-
 	if (table != NULL) {
 		if (commit && !table->is_temporary()) {
 			table->stats_bg_flag &= byte(~BG_STAT_SHOULD_QUIT);

storage/innobase/row/row0uins.cc

@@ -398,8 +398,12 @@ static bool row_undo_ins_parse_undo_rec(undo_node_t* node, bool dict_locked)
 		ptr[len] = 0;
 		const char* name = reinterpret_cast<char*>(ptr);
 		if (strcmp(table->name.m_name, name)) {
-			dict_table_rename_in_cache(table, name, false,
-						   table_id != 0);
+			dict_table_rename_in_cache(table, name, false, true);
+		} else if (table->space) {
+			const auto s = table->space->name();
+			if (len != s.size() || memcmp(name, s.data(), len)) {
+				table->rename_tablespace(name, true);
+			}
 		}
 		goto close_table;
 	}