From 8dc70c862b8ec115fd9a3c2b37c746ffc4f0d3cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Mon, 4 Jun 2018 15:55:37 +0300 Subject: 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(). --- .../suite/gcol/r/innodb_virtual_debug.result | 14 ++++++-- mysql-test/suite/gcol/t/innodb_virtual_debug.test | 15 ++++++-- storage/innobase/dict/dict0dict.cc | 32 +---------------- storage/innobase/handler/handler0alter.cc | 14 +++++++- storage/innobase/include/dict0dict.h | 19 +++------- storage/innobase/include/dict0dict.ic | 24 ------------- storage/innobase/include/dict0mem.h | 42 ++++++++++++++++++++++ storage/innobase/row/row0upd.cc | 2 +- 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( - 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 + (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; } -- cgit v1.2.1