MDEV-24758 heap-use-after-poison in innobase_add_instant_try/rec_copy

This is a backport of
commit fd9ca2a742 (MDEV-23295) and
commit 9a156e1a23 (MDEV-23345) to 10.3.

An instant ADD/DROP/reorder column could create a dummy table
object with the wrong ROW_FORMAT when innodb_default_row_format
was changed between CREATE TABLE and ALTER TABLE.

prepare_inplace_alter_table_dict(): If we had promised that
ALGORITHM=INPLACE is supported, we must preserve the ROW_FORMAT.

The rest of the changes are related to adding
Alter_inplace_info::inplace_supported to cache the return value of
handler::check_if_supported_inplace_alter().

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

@@ -82,4 +82,14 @@ SHOW TABLE STATUS LIKE 't1';
 Name	Engine	Version	Row_format	Rows	Avg_row_length	Data_length	Max_data_length	Index_length	Data_free	Auto_increment	Create_time	Update_time	Check_time	Collation	Checksum	Create_options	Comment	Max_index_length	Temporary
 t1	InnoDB	#	Compact	#	#	#	#	#	#	NULL	#	#	NULL	latin1_swedish_ci	NULL			0	N
 DROP TABLE t1;
+#
+# MDEV-24758 heap-use-after-poison in innobase_add_instant_try/rec_copy
+#
+CREATE TABLE t1 (pk INT PRIMARY KEY) CHARACTER SET utf8 ENGINE=InnoDB;
+INSERT INTO t1 VALUES (1);
+SET GLOBAL innodb_default_row_format = REDUNDANT;
+ALTER TABLE t1 ADD a CHAR(8) DEFAULT '';
+DROP TABLE t1;
+SET GLOBAL innodb_default_row_format = @row_format;
+# End of 10.3 tests
 SET GLOBAL innodb_default_row_format = @row_format;

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

@@ -1,4 +1,5 @@
 --source include/have_innodb.inc
+--source include/innodb_row_format.inc
 
 SET @row_format = @@GLOBAL.innodb_default_row_format;
 
@@ -95,4 +96,17 @@ ALTER TABLE t1 DROP INDEX k1;
 SHOW TABLE STATUS LIKE 't1';
 DROP TABLE t1;
 
+--echo #
+--echo # MDEV-24758 heap-use-after-poison in innobase_add_instant_try/rec_copy
+--echo #
+
+CREATE TABLE t1 (pk INT PRIMARY KEY) CHARACTER SET utf8 ENGINE=InnoDB;
+INSERT INTO t1 VALUES (1);
+SET GLOBAL innodb_default_row_format = REDUNDANT;
+ALTER TABLE t1 ADD a CHAR(8) DEFAULT '';
+DROP TABLE t1;
+SET GLOBAL innodb_default_row_format = @row_format;
+
+--echo # End of 10.3 tests
+
 SET GLOBAL innodb_default_row_format = @row_format;

sql/handler.h

@@ -2347,6 +2347,9 @@ public:
   /** true for online operation (LOCK=NONE) */
   bool online;
 
+  /** which ALGORITHM and LOCK are supported by the storage engine */
+  enum_alter_inplace_result inplace_supported;
+
   /**
      Can be set by handler to describe why a given operation cannot be done
      in-place (HA_ALTER_INPLACE_NOT_SUPPORTED) or why it cannot be done

sql/sql_alter.cc

@@ -1,5 +1,5 @@
 /* Copyright (c) 2010, Oracle and/or its affiliates. All rights reserved.
-   Copyright (c) 2016, 2018, MariaDB Corporation
+   Copyright (c) 2016, 2020, MariaDB Corporation
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -127,10 +127,10 @@ const char* Alter_info::lock() const
 }
 
 
-bool Alter_info::supports_algorithm(THD *thd, enum_alter_inplace_result result,
+bool Alter_info::supports_algorithm(THD *thd,
                                     const Alter_inplace_info *ha_alter_info)
 {
-  switch (result) {
+  switch (ha_alter_info->inplace_supported) {
   case HA_ALTER_INPLACE_EXCLUSIVE_LOCK:
   case HA_ALTER_INPLACE_SHARED_LOCK:
   case HA_ALTER_INPLACE_NO_LOCK:
@@ -171,10 +171,10 @@ bool Alter_info::supports_algorithm(THD *thd, enum_alter_inplace_result result,
 }
 
 
-bool Alter_info::supports_lock(THD *thd, enum_alter_inplace_result result,
+bool Alter_info::supports_lock(THD *thd,
                                const Alter_inplace_info *ha_alter_info)
 {
-  switch (result) {
+  switch (ha_alter_info->inplace_supported) {
   case HA_ALTER_INPLACE_EXCLUSIVE_LOCK:
     // If SHARED lock and no particular algorithm was requested, use COPY.
     if (requested_lock == Alter_info::ALTER_TABLE_LOCK_SHARED &&

sql/sql_alter.h

@@ -1,5 +1,5 @@
 /* Copyright (c) 2010, 2014, Oracle and/or its affiliates.
-   Copyright (c) 2013, 2018, MariaDB Corporation.
+   Copyright (c) 2013, 2020, MariaDB Corporation.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -204,29 +204,26 @@ public:
      with the specified user alter algorithm.
 
      @param  thd            Thread handle
-     @param  result         Operation supported for inplace alter
      @param  ha_alter_info  Structure describing changes to be done
                             by ALTER TABLE and holding data during
                             in-place alter
      @retval false  Supported operation
      @retval true   Not supported value
   */
-  bool supports_algorithm(THD *thd, enum_alter_inplace_result result,
+  bool supports_algorithm(THD *thd,
                           const Alter_inplace_info *ha_alter_info);
 
   /**
      Check whether the given result can be supported
      with the specified user lock type.
 
-     @param  result         Operation supported for inplace alter
      @param  ha_alter_info  Structure describing changes to be done
                             by ALTER TABLE and holding data during
                             in-place alter
      @retval false  Supported lock type
      @retval true   Not supported value
   */
-  bool supports_lock(THD *thd, enum_alter_inplace_result result,
+  bool supports_lock(THD *thd, const Alter_inplace_info *ha_alter_info);
-                     const Alter_inplace_info *ha_alter_info);
 
   /**
     Return user requested algorithm. If user does not specify

sql/sql_table.cc

@@ -7549,7 +7549,6 @@ static bool is_inplace_alter_impossible(TABLE *table,
   @param ha_alter_info      Structure describing ALTER TABLE to be carried
                             out and serving as a storage place for data
                             used during different phases.
-  @param inplace_supported  Enum describing the locking requirements.
   @param target_mdl_request Metadata request/lock on the target table name.
   @param alter_ctx          ALTER TABLE runtime context.
 
@@ -7574,7 +7573,6 @@ static bool mysql_inplace_alter_table(THD *thd,
                                       TABLE *table,
                                       TABLE *altered_table,
                                       Alter_inplace_info *ha_alter_info,
-                                      enum_alter_inplace_result inplace_supported,
                                       MDL_request *target_mdl_request,
                                       Alter_table_ctx *alter_ctx)
 {
@@ -7585,6 +7583,8 @@ static bool mysql_inplace_alter_table(THD *thd,
   Alter_info *alter_info= ha_alter_info->alter_info;
   bool reopen_tables= false;
   bool res;
+  const enum_alter_inplace_result inplace_supported=
+    ha_alter_info->inplace_supported;
 
   DBUG_ENTER("mysql_inplace_alter_table");
 
@@ -10006,19 +10006,19 @@ do_continue:;
     if (alter_info->requested_lock == Alter_info::ALTER_TABLE_LOCK_NONE)
       ha_alter_info.online= true;
     // Ask storage engine whether to use copy or in-place
-    enum_alter_inplace_result inplace_supported=
+    ha_alter_info.inplace_supported=
       table->file->check_if_supported_inplace_alter(altered_table,
                                                     &ha_alter_info);
 
-    if (alter_info->supports_algorithm(thd, inplace_supported, &ha_alter_info) ||
+    if (alter_info->supports_algorithm(thd, &ha_alter_info) ||
-        alter_info->supports_lock(thd, inplace_supported, &ha_alter_info))
+        alter_info->supports_lock(thd, &ha_alter_info))
     {
       thd->drop_temporary_table(altered_table, NULL, false);
       goto err_new_table_cleanup;
     }
 
     // If SHARED lock and no particular algorithm was requested, use COPY.
-    if (inplace_supported == HA_ALTER_INPLACE_EXCLUSIVE_LOCK &&
+    if (ha_alter_info.inplace_supported == HA_ALTER_INPLACE_EXCLUSIVE_LOCK &&
         alter_info->requested_lock == Alter_info::ALTER_TABLE_LOCK_SHARED &&
          alter_info->algorithm(thd) ==
                  Alter_info::ALTER_TABLE_ALGORITHM_DEFAULT &&
@@ -10026,7 +10026,7 @@ do_continue:;
                  Alter_info::ALTER_TABLE_ALGORITHM_DEFAULT)
       use_inplace= false;
 
-    if (inplace_supported == HA_ALTER_INPLACE_NOT_SUPPORTED)
+    if (ha_alter_info.inplace_supported == HA_ALTER_INPLACE_NOT_SUPPORTED)
       use_inplace= false;
 
     if (use_inplace)
@@ -10038,7 +10038,7 @@ do_continue:;
       */
       Check_level_instant_set check_level_save(thd, CHECK_FIELD_WARN);
       int res= mysql_inplace_alter_table(thd, table_list, table, altered_table,
-                                         &ha_alter_info, inplace_supported,
+                                         &ha_alter_info,
                                          &target_mdl_request, &alter_ctx);
       my_free(const_cast<uchar*>(frm.str));
 

storage/innobase/handler/handler0alter.cc

@@ -5003,6 +5003,19 @@ prepare_inplace_alter_table_dict(
 
 	user_table = ctx->new_table;
 
+	switch (ha_alter_info->inplace_supported) {
+	default: break;
+	case HA_ALTER_INPLACE_INSTANT:
+	case HA_ALTER_INPLACE_NOCOPY_LOCK:
+	case HA_ALTER_INPLACE_NOCOPY_NO_LOCK:
+		/* If we promised ALGORITHM=NOCOPY or ALGORITHM=INSTANT,
+		we must retain the original ROW_FORMAT of the table. */
+		flags = (user_table->flags & (DICT_TF_MASK_COMPACT
+					      | DICT_TF_MASK_ATOMIC_BLOBS))
+			| (flags & ~(DICT_TF_MASK_COMPACT
+				     | DICT_TF_MASK_ATOMIC_BLOBS));
+	}
+
 	trx_start_if_not_started_xa(ctx->prebuilt->trx, true);
 
 	if (ha_alter_info->handler_flags