[MDEV-29258] Failing assertion for name length on RENAME TABLE Created: 2022-08-05  Updated: 2022-08-30  Resolved: 2022-08-30

Status: Closed
Project: MariaDB Server
Component/s: Storage Engine - InnoDB
Affects Version/s: 10.3, 10.4, 10.5, 10.6, 10.7, 10.8, 10.9, 10.10
Fix Version/s: 10.3.37, 10.4.27, 10.5.18, 10.6.10, 10.7.6, 10.8.5, 10.9.3, 10.10.2

Type: Bug Priority: Major
Reporter: Aleksey Midenkov Assignee: Marko Mäkelä
Resolution: Fixed Votes: 0
Labels: None

Issue Links:
Relates
relates to MDEV-28933 CREATE OR REPLACE fails to recreate s... Closed
relates to MDEV-28980 InnoDB: Failing assertion: len <= MAX... Closed
relates to MDEV-29409 ASAN failure on long fk_id when renam... Closed

 Description   

Reproduce

--source include/have_innodb.inc
--source include/not_windows.inc
--source include/not_embedded.inc
 
let $d255=###################################################;
let $d250=##################################################;
let $d245=#################################################;
--replace_result $d255 d255
eval create database `$d255`;
eval create table `$d255`.`$d245` (x int) engine innodb;
 
# CREATE TABLE works
eval create table `$d255`.`$d250` (x int) engine innodb;
eval drop table `$d255`.`$d250`;
 
# RENAME TABLE doesn't work
--error ER_IDENT_CAUSES_TOO_LONG_PATH
eval rename table `$d255`.`$d245` to `$d255`.`$d250`;
 
# and even assertion-fails!
eval rename table `$d255`.`$d245` to a;
 
eval drop database `$d255`;
drop table t;

Result

RENAME TABLE fails to rename to `$d255`.`$d250` while CREATE TABLE succeeds. RENAME TABLE from `$d255`.`$d245` fails with assertion.

#12 0x00007f77f8622e96 in __GI___assert_fail (assertion=0x55cdf1186df3 "len <= (64*3) * 2 + 1", file=0x55cdf11860d0 "../src/storage/innobase/trx/trx0rec.cc", line=1862, function=0x55cdf1186d77 "uint16_t trx_undo_page_report_rename(trx_t *, const dict_table_t *, buf_block_t *, mtr_t *)") at ./assert/assert.c:101
#13 0x000055cdf0a42d21 in trx_undo_page_report_rename (trx=0x7f77eec61680, table=0x7f77b8216ce8, block=0x7f77eeb616c0, mtr=0x7f77ec3b6100) at ../src/storage/innobase/trx/trx0rec.cc:1862
#14 0x000055cdf0a42920 in trx_undo_report_rename (trx=0x7f77eec61680, table=0x7f77b8216ce8) at ../src/storage/innobase/trx/trx0rec.cc:1909
#15 0x000055cdf09aec34 in row_rename_table_for_mysql (old_name=0x7f77ec3b7720 "@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023/@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023@0023", new_name=0x7f77ec3b7920 "test/a", trx=0x7f77eec61680, use_fk=true) at ../src/storage/innobase/row/row0mysql.cc:2655



 Comments   
Comment by Marko Mäkelä [ 2022-08-09 ]

The error is being emitted outside InnoDB:

10.6 558f1eff64e7708b594ef0315e23bdeb1d23ccf7

#0  my_error (nr=1860, MyFlags=0) at /mariadb/10.6/mysys/my_error.c:109
#1  0x0000562ef21ef3a2 in mysql_rename_table (base=base@entry=0x562ef4e0da48, 
    old_db=old_db@entry=0x7fc21801fb58, 
    old_name=old_name@entry=0x7fc22e5b0f80, 
    new_db=new_db@entry=0x7fc218020270, 
    new_name=new_name@entry=0x7fc22e5b0f90, id=id@entry=0x7fc22e5b0fa0, 
    flags=<optimized out>) at /mariadb/10.6/sql/sql_table.cc:4974
#2  0x0000562ef2173510 in do_rename (thd=0x7fc218000c58, param=0x7fc22e5b0f80, 
    ddl_log_state=0x7fc22e5b0f60, ren_table=0x7fc21801fb40, 
    new_db=0x7fc218020270, skip_error=<optimized out>, 
    force_if_exists=<optimized out>) at /mariadb/10.6/sql/sql_rename.cc:383
#3  0x0000562ef217399e in rename_tables (skip_error=false, 
    force_if_exists=0x7fc22e5b0f2f, if_exists=false, 
    ddl_log_state=0x7fc22e5b0f60, table_list=0x7fc21801fb40, 
    thd=0x7fc218000c58) at /mariadb/10.6/sql/sql_rename.cc:533
#4  mysql_rename_tables (thd=thd@entry=0x7fc218000c58, 
    table_list=table_list@entry=0x7fc21801fb40, silent=silent@entry=false, 
    if_exists=false) at /mariadb/10.6/sql/sql_rename.cc:164

I do not think that we should change that. Only the assertion failure in InnoDB needs to be prevented.

Comment by Marko Mäkelä [ 2022-08-30 ]

diff --git a/mysql-test/suite/innodb/t/foreign_key_not_windows.test b/mysql-test/suite/innodb/t/foreign_key_not_windows.test
index 7ad3723d5de..071dc52a4d4 100644
--- a/mysql-test/suite/innodb/t/foreign_key_not_windows.test
+++ b/mysql-test/suite/innodb/t/foreign_key_not_windows.test
@@ -38,6 +38,18 @@ eval CREATE TABLE `$d255`.`_$d250`
 --replace_result $d255 d255
 eval CREATE TABLE `$d255`.`$d250`
 (a INT PRIMARY KEY, FOREIGN KEY(a) REFERENCES test.t(a)) ENGINE=InnoDB;
+
+--echo #
+--echo # MDEV-29258 Failing assertion for name length on RENAME TABLE
+--echo #
+
+let $d245=#################################################;
+eval CREATE TABLE `$d255`.`$d245` (x INT) ENGINE=InnoDB;
+eval DROP TABLE `$d255`.`$d250`;
+
+--error ER_IDENT_CAUSES_TOO_LONG_PATH
+eval RENAME TABLE `$d255`.`$d245` TO `$d255`.`$d250`;
+eval RENAME TABLE `$d255`.`$d245` TO a;
 --replace_result $d255 d255
 eval DROP DATABASE `$d255`;
 DROP TABLE t;

10.3 0d1de5e1d19f1e96058ab5948e184f22e7bdd908

mysqld: /mariadb/10.3/storage/innobase/trx/trx0rec.cc:1870: ulint trx_undo_page_report_rename(trx_t*, const dict_table_t*, buf_block_t*, mtr_t*): Assertion `len <= (64U*3) * 2 + 1' failed.

Comment by Marko Mäkelä [ 2022-08-30 ]

The InnoDB problems in 10.3 were as follows:

diff --git a/storage/innobase/fil/fil0fil.cc b/storage/innobase/fil/fil0fil.cc
index a196303c39f..5d9f80eda70 100644
--- a/storage/innobase/fil/fil0fil.cc
+++ b/storage/innobase/fil/fil0fil.cc
@@ -2105,7 +2105,7 @@ fil_op_write_log(
 	case MLOG_FILE_RENAME2:
 		ut_ad(strchr(new_path, OS_PATH_SEPARATOR) != NULL);
 		len = strlen(new_path) + 1;
-		log_ptr = mlog_open(mtr, 2 + len);
+		log_ptr = mlog_open(mtr, 2);
 		ut_a(log_ptr);
 		mach_write_to_2(log_ptr, len);
 		log_ptr += 2;
diff --git a/storage/innobase/trx/trx0rec.cc b/storage/innobase/trx/trx0rec.cc
index e011b3f5d8e..5c76fb56c48 100644
--- a/storage/innobase/trx/trx0rec.cc
+++ b/storage/innobase/trx/trx0rec.cc
@@ -1867,9 +1867,9 @@ trx_undo_page_report_rename(trx_t* trx, const dict_table_t* table,
 	byte* start = block->frame + first_free;
 	size_t len = strlen(table->name.m_name);
 	const size_t fixed = 2 + 1 + 11 + 11 + 2;
-	ut_ad(len <= NAME_LEN * 2 + 1);
+	ut_ad(len <= NAME_CHAR_LEN * 5 * 2 + 1);
 	/* The -10 is used in trx_undo_left() */
-	compile_time_assert((NAME_LEN * 1) * 2 + fixed
+	compile_time_assert(NAME_CHAR_LEN * 5 * 2 + fixed
 			    + TRX_UNDO_PAGE_HDR + TRX_UNDO_PAGE_HDR_SIZE
 			    < UNIV_PAGE_SIZE_MIN - 10 - FIL_PAGE_DATA_END);
 

With these fixed, the test case would show a buffer overflow in the SQL layer:

10.3 0d1de5e1d19f1e96058ab5948e184f22e7bdd908 with InnoDB patches

==31744==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fa636c472d0 at pc 0x55ae1787c95a bp 0x7fa636c46d10 sp 0x7fa636c46d08
WRITE of size 1 at 0x7fa636c472d0 thread T27
    #0 0x55ae1787c959 in strxmov /mariadb/10.3/strings/strxmov.c:53
    #1 0x55ae165b2563 in rename_file_ext(char const*, char const*, char const*) /mariadb/10.3/sql/table.cc:4202
    #2 0x55ae16532381 in mysql_rename_table(handlerton*, st_mysql_const_lex_string const*, st_mysql_const_lex_string const*, st_mysql_const_lex_string const*, st_mysql_const_lex_string const*, unsigned int) /mariadb/10.3/sql/sql_table.cc:5635
Address 0x7fa636c472d0 is located in stack of thread T27 at offset 1120 in frame
    #0 0x55ae165b24ab in rename_file_ext(char const*, char const*, char const*) /mariadb/10.3/sql/table.cc:4199
 
  This frame has 2 object(s):
    [32, 544) 'from_b' (line 4200)
    [608, 1120) 'to_b' (line 4200) <== Memory access at offset 1120 overflows this variable

The buffer length is only FN_REFLEN (512 bytes). The parameter to points to a string "./@0023…@0023/@0023…@0023" (508 bytes), and the { {ext=".frm"}} would overflow the 512-byte buffer by 1 byte. In the embedded server, the ./ could be replaced with a path to the data directory. If we neglect that, the following fixes the problem and also allows the RENAME to proceed:

diff --git a/sql/table.cc b/sql/table.cc
index 64fb3150a39..f528f12a507 100644
--- a/sql/table.cc
+++ b/sql/table.cc
@@ -4197,7 +4197,8 @@ void update_create_info_from_table(HA_CREATE_INFO *create_info, TABLE *table)
 int
 rename_file_ext(const char * from,const char * to,const char * ext)
 {
-  char from_b[FN_REFLEN],to_b[FN_REFLEN];
+  /* Reserve space for ./databasename/tablename.frm + NUL byte */
+  char from_b[2 + FN_REFLEN + 4 + 1], to_b[2 + FN_REFLEN + 4 + 1];
   (void) strxmov(from_b,from,ext,NullS);
   (void) strxmov(to_b,to,ext,NullS);
   return mysql_file_rename(key_file_frm, from_b, to_b, MYF(0));

The second RENAME in the test has to be adjusted accordingly, because the first RENAME will be accepted:

eval RENAME TABLE `$d255`.`$d245` TO `$d255`.`$d250`;
eval RENAME TABLE `$d255`.`$d250` TO a;

Comment by Marko Mäkelä [ 2022-08-30 ]

Starting with 10.6, mysql_rename_table() will flag the error. The check was tightened in MDEV-23842 Atomic RENAME TABLE:

@@ -4728,37 +4754,20 @@ mysql_rename_table(handlerton *base, const LEX_CSTRING *
old_db,
   length= build_table_filename(to, sizeof(to) - 1, new_db->str,
                                new_name->str, "", flags & FN_TO_IS_TMP);
   // Check if we hit FN_REFLEN bytes along with file extension.
-  if (length+reg_ext_length > FN_REFLEN)
+  if (length+reg_ext_length >= FN_REFLEN)
   {
     my_error(ER_IDENT_CAUSES_TOO_LONG_PATH, MYF(0), (int) sizeof(to)-1, to);
     DBUG_RETURN(TRUE);
   }

I will try to revert that and test it with AddressSanitizer, to prevent a functionality regression.

Generated at Thu Feb 08 10:07:06 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.