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

In commit 9159970 (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));