MDEV-16376 ASAN: heap-use-after-free in gcol.innodb_virtual_debug

After a failed ADD INDEX, dict_index_remove_from_cache_low()
could iterate the index fields and dereference a freed virtual
column object when trying to remove the index from the v_indexes
of the virtual column.

This regression was caused by a merge of
MDEV-16119 InnoDB lock->index refers to a freed object.

ha_innobase_inplace_ctx::clear_added_indexes(): Detach the
indexes of uncommitted indexes from virtual columns, so that
the iteration in dict_index_remove_from_cache_low() can be avoided.

ha_innobase::prepare_inplace_alter_table(): Ignore uncommitted
corrupted indexes when rejecting ALTER TABLE. (This minor bug was
revealed by the extension of the test case.)

dict_index_t::detach_columns(): Detach an index from virtual columns.
Invoked by both dict_index_remove_from_cache_low() and
ha_innobase_inplace_ctx::clear_added_indexes().

dict_col_t::detach(const dict_index_t& index): Detach an index from
a column.

dict_col_t::is_virtual(): Replaces dict_col_is_virtual().

dict_index_t::has_virtual(): Replaces dict_index_has_virtual().

8 files changed, 85 insertions, 77 deletions

diff --git a/mysql-test/suite/gcol/r/innodb_virtual_debug.result b/mysql-test/suite/gcol/r/innodb_virtual_debug.result
index 7774c6c347c..50b714566d9 100644
--- a/mysql-test/suite/gcol/r/innodb_virtual_debug.result
+++ b/mysql-test/suite/gcol/r/innodb_virtual_debug.result
@@ -64,11 +64,19 @@ INSERT INTO t VALUES (18, 1, DEFAULT, 'mm');
 INSERT INTO t VALUES (28, 1, DEFAULT, 'mm');
 INSERT INTO t VALUES (null, null, DEFAULT, 'mm');
 CREATE INDEX idx_1 on t(c);
-SET SESSION debug_dbug="+d,create_index_fail";
-ALTER TABLE t ADD COLUMN x INT GENERATED ALWAYS AS(a+b), ADD INDEX idx (x);
+SET @saved_dbug = @@SESSION.debug_dbug;
+SET debug_dbug = '+d,create_index_fail';
+ALTER TABLE t ADD COLUMN x INT GENERATED ALWAYS AS(a+b), ADD INDEX idx (x),
+ADD INDEX idcx (c,x);
 ERROR 23000: Duplicate entry '' for key '*UNKNOWN*'
-SET SESSION debug_dbug="";
+UPDATE t SET a=a+1;
+affected rows: 3
+info: Rows matched: 4  Changed: 3  Warnings: 0
+ALTER TABLE t ADD INDEX idc(c);
+ERROR 23000: Duplicate entry '' for key '*UNKNOWN*'
+SET debug_dbug = @saved_dbug;
 affected rows: 0
+UPDATE t SET b=b-1;
 SHOW CREATE TABLE t;
 Table	Create Table
 t	CREATE TABLE `t` (
diff --git a/mysql-test/suite/gcol/t/innodb_virtual_debug.test b/mysql-test/suite/gcol/t/innodb_virtual_debug.test
index 3870f84e066..ccdd16c9ebe 100644
--- a/mysql-test/suite/gcol/t/innodb_virtual_debug.test
+++ b/mysql-test/suite/gcol/t/innodb_virtual_debug.test
@@ -119,14 +119,23 @@ INSERT INTO t VALUES (null, null, DEFAULT, 'mm');
 
 CREATE INDEX idx_1 on t(c);
 
-SET SESSION debug_dbug="+d,create_index_fail";
+SET @saved_dbug = @@SESSION.debug_dbug;
+SET debug_dbug = '+d,create_index_fail';
 
 --enable_info
 --error ER_DUP_ENTRY
-ALTER TABLE t ADD COLUMN x INT GENERATED ALWAYS AS(a+b), ADD INDEX idx (x);
-SET SESSION debug_dbug="";
+ALTER TABLE t ADD COLUMN x INT GENERATED ALWAYS AS(a+b), ADD INDEX idx (x),
+ADD INDEX idcx (c,x);
+
+UPDATE t SET a=a+1;
+
+--error ER_DUP_ENTRY
+ALTER TABLE t ADD INDEX idc(c);
+SET debug_dbug = @saved_dbug;
 --disable_info
 
+UPDATE t SET b=b-1;
+
 SHOW CREATE TABLE t;
 
 SELECT c FROM t;
diff --git a/storage/innobase/dict/dict0dict.cc b/storage/innobase/dict/dict0dict.cc
index 9a7d9eef092..f34da118a31 100644
--- a/storage/innobase/dict/dict0dict.cc
+++ b/storage/innobase/dict/dict0dict.cc
@@ -2676,37 +2676,7 @@ dict_index_remove_from_cache_low(
 	UT_LIST_REMOVE(table->indexes, index);
 
 	/* Remove the index from affected virtual column index list */
-	if (dict_index_has_virtual(index)) {
-		const dict_col_t*	col;
-		const dict_v_col_t*	vcol;
-
-		for (ulint i = 0; i < dict_index_get_n_fields(index); i++) {
-			col =  dict_index_get_nth_col(index, i);
-			if (dict_col_is_virtual(col)) {
-				vcol = reinterpret_cast<const dict_v_col_t*>(
-					col);
-
-				/* This could be NULL, when we do add virtual
-				column, add index together. We do not need to
-				track this virtual column's index */
-				if (vcol->v_indexes == NULL) {
-					continue;
-				}
-
-				dict_v_idx_list::iterator	it;
-
-				for (it = vcol->v_indexes->begin();
-				     it != vcol->v_indexes->end(); ++it) {
-					dict_v_idx_t	v_index = *it;
-					if (v_index.index == index) {
-						vcol->v_indexes->erase(it);
-						break;
-					}
-				}
-			}
-
-		}
-	}
+	index->detach_columns();
 
 	dict_mem_index_free(index);
 }
diff --git a/storage/innobase/handler/handler0alter.cc b/storage/innobase/handler/handler0alter.cc
index bc88001edf6..517f20006af 100644
--- a/storage/innobase/handler/handler0alter.cc
+++ b/storage/innobase/handler/handler0alter.cc
@@ -245,6 +245,16 @@ struct ha_innobase_inplace_ctx : public inplace_alter_handler_ctx
 	@return whether the table will be rebuilt */
 	bool need_rebuild () const { return(old_table != new_table); }
 
+	/** Clear uncommmitted added indexes after a failed operation. */
+	void clear_added_indexes()
+	{
+		for (ulint i = 0; i < num_to_add_index; i++) {
+			if (!add_index[i]->is_committed()) {
+				add_index[i]->detach_columns();
+			}
+		}
+	}
+
 private:
 	// Disable copying
 	ha_innobase_inplace_ctx(const ha_innobase_inplace_ctx&);
@@ -5996,7 +6006,8 @@ check_if_can_drop_indexes:
 		for (dict_index_t* index = dict_table_get_first_index(indexed_table);
 		     index != NULL; index = dict_table_get_next_index(index)) {
 
-			if (!index->to_be_dropped && index->is_corrupted()) {
+			if (!index->to_be_dropped && index->is_committed()
+			    && index->is_corrupted()) {
 				my_error(ER_INDEX_CORRUPT, MYF(0), index->name());
 				goto err_exit;
 			}
@@ -6567,6 +6578,7 @@ oom:
 	that we hold at most a shared lock on the table. */
 	m_prebuilt->trx->error_info = NULL;
 	ctx->trx->error_state = DB_SUCCESS;
+	ctx->clear_added_indexes();
 
 	DBUG_RETURN(true);
 }
diff --git a/storage/innobase/include/dict0dict.h b/storage/innobase/include/dict0dict.h
index 51ce248b98d..9822b57d190 100644
--- a/storage/innobase/include/dict0dict.h
+++ b/storage/innobase/include/dict0dict.h
@@ -750,13 +750,9 @@ dict_index_is_spatial(
 /*==================*/
 	const dict_index_t*	index)	/*!< in: index */
 	MY_ATTRIBUTE((warn_unused_result));
-/** Check whether the index contains a virtual column.
-@param[in]	index	index
-@return	nonzero for index on virtual column, zero for other indexes */
-UNIV_INLINE
-ulint
-dict_index_has_virtual(
-	const dict_index_t*	index);
+
+#define dict_index_has_virtual(index) (index)->has_virtual()
+
 /********************************************************************//**
 Check whether the index is the insert buffer tree.
 @return nonzero for insert buffer, zero for other indexes */
@@ -1964,13 +1960,8 @@ dict_index_node_ptr_max_size(
 /*=========================*/
 	const dict_index_t*	index)	/*!< in: index */
 	MY_ATTRIBUTE((warn_unused_result));
-/** Check if a column is a virtual column
-@param[in]	col	column
-@return true if it is a virtual column, false otherwise */
-UNIV_INLINE
-bool
-dict_col_is_virtual(
-	const dict_col_t*	col);
+
+#define dict_col_is_virtual(col) (col)->is_virtual()
 
 /** encode number of columns and number of virtual columns in one
 4 bytes value. We could do this because the number of columns in
diff --git a/storage/innobase/include/dict0dict.ic b/storage/innobase/include/dict0dict.ic
index e20da7c708a..ca2ea769612 100644
--- a/storage/innobase/include/dict0dict.ic
+++ b/storage/innobase/include/dict0dict.ic
@@ -72,16 +72,6 @@ dict_col_copy_type(
 	type->mbminlen = col->mbminlen;
 	type->mbmaxlen = col->mbmaxlen;
 }
-/** Check if a column is a virtual column
-@param[in]      col     column
-@return true if it is a virtual column, false otherwise */
-UNIV_INLINE
-bool
-dict_col_is_virtual(
-	const dict_col_t*	col)
-{
-	return(col->prtype & DATA_VIRTUAL);
-}
 
 #ifdef UNIV_DEBUG
 /*********************************************************************//**
@@ -325,20 +315,6 @@ dict_index_is_spatial(
 	return(index->type & DICT_SPATIAL);
 }
 
-/** Check whether the index contains a virtual column
-@param[in]	index	index
-@return	nonzero for the index has virtual column, zero for other indexes */
-UNIV_INLINE
-ulint
-dict_index_has_virtual(
-	const dict_index_t*	index)
-{
-	ut_ad(index);
-	ut_ad(index->magic_n == DICT_INDEX_MAGIC_N);
-
-	return(index->type & DICT_VIRTUAL);
-}
-
 /********************************************************************//**
 Check whether the index is the insert buffer tree.
 @return nonzero for insert buffer, zero for other indexes */
diff --git a/storage/innobase/include/dict0mem.h b/storage/innobase/include/dict0mem.h
index 18e600c25d3..a18d6c1abdd 100644
--- a/storage/innobase/include/dict0mem.h
+++ b/storage/innobase/include/dict0mem.h
@@ -612,6 +612,13 @@ struct dict_col_t{
 	unsigned	max_prefix:12;	/*!< maximum index prefix length on
 					this column. Our current max limit is
 					3072 for Barracuda table */
+
+	/** @return whether this is a virtual column */
+	bool is_virtual() const { return prtype & DATA_VIRTUAL; }
+
+	/** Detach the column from an index.
+	@param[in]	index	index to be detached from */
+	inline void detach(const dict_index_t& index);
 };
 
 /** Index information put in a list of virtual column structure. Index
@@ -981,10 +988,45 @@ struct dict_index_t{
 		return DICT_CLUSTERED == (type & (DICT_CLUSTERED | DICT_IBUF));
 	}
 
+	/** @return whether the index includes virtual columns */
+	bool has_virtual() const { return type & DICT_VIRTUAL; }
+
 	/** @return whether the index is corrupted */
 	inline bool is_corrupted() const;
+
+	/** Detach the columns from the index that is to be freed. */
+	void detach_columns()
+	{
+		if (has_virtual()) {
+			for (unsigned i = 0; i < n_fields; i++) {
+				fields[i].col->detach(*this);
+			}
+
+			n_fields = 0;
+		}
+	}
 };
 
+/** Detach a column from an index.
+@param[in]	index	index to be detached from */
+inline void dict_col_t::detach(const dict_index_t& index)
+{
+	if (!is_virtual()) {
+		return;
+	}
+
+	if (dict_v_idx_list* v_indexes = reinterpret_cast<const dict_v_col_t*>
+	    (this)->v_indexes) {
+		for (dict_v_idx_list::iterator i = v_indexes->begin();
+		     i != v_indexes->end(); i++) {
+			if (i->index == &index) {
+				v_indexes->erase(i);
+				return;
+			}
+		}
+	}
+}
+
 /** The status of online index creation */
 enum online_index_status {
 	/** the index is complete and ready for access */
diff --git a/storage/innobase/row/row0upd.cc b/storage/innobase/row/row0upd.cc
index 946bbae9534..18a6a26f3ce 100644
--- a/storage/innobase/row/row0upd.cc
+++ b/storage/innobase/row/row0upd.cc
@@ -593,7 +593,7 @@ row_upd_changes_field_size_or_external(
 		/* We should ignore virtual field if the index is not
 		a virtual index */
 		if (upd_fld_is_virtual_col(upd_field)
-		    && dict_index_has_virtual(index) != DICT_VIRTUAL) {
+		    && !index->has_virtual()) {
 			continue;
 		}