revision-id: e6b4aedae703d7c4d239ce16c41e117d89cee409 (mariadb-10.4.4-93-ge6b4aedae70) parent(s): ea771624528794449444b2c066ca6171388cdf37 author: Sujatha committer: Sujatha timestamp: 2019-05-16 13:12:21 +0530 message: MDEV-18970: uninited var can be read in gtid_delete_pending() Problem: ======== gcc 8 -O2 seems to indicate a real error for this code: direct_pos= table->file->ha_table_flags() & HA_PRIMARY_KEY_REQUIRED_FOR_POSITION; the warning: /mariadb/10.4/sql/rpl_gtid.cc:980:7: warning: 'direct_pos' may be used uninitialized in this function [-Wmaybe-uninitialized] Analysis: ========= 'direct_pos' is a variable which holds 'table_flags'. If this flag is set it means that a record within a table can be directly located by using its position. If this flag is set to '0' means there is no direct access is available, hence index scan must be initiated to locate the record. This direct_pos is used to locate a row within mysql.gtid_slave_pos table for deletion. Prior to the initialization of 'direct_pos' following steps take place. 1. mysql.gtid_slave_pos table is opened and 'table_opened' flag is set to true. 2. State check for mysql.gtid_slave_pos table is initiated. If there is a failure during step2 code will be redirected to the error handling part. This error handling code will access uninitialized value of 'direct_pos'. This results in above mentioned warning. Another issue found during analysis is the error handling code uses '!direct_pos' to identify if the index is initialized or not. This is incorrect. The index initialization code is shown below. if (!direct_pos && (err= table->file->ha_index_init(0, 0))) { table->file->print_error(err, MYF(0)); goto end; } In case there is a failure during ha_index_init code will be redirected to end part which tries to close the uninitialized index. It will result in an assert 10.4/sql/handler.h:3186: int handler::ha_index_end(): Assertion `inited==INDEX' failed. Fix: === Introduce a new variable named 'index_inited'. Set this variable upon successful initialization of index initialization otherwise by default it is false. Use this variable during error handling. --- sql/rpl_gtid.cc | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/sql/rpl_gtid.cc b/sql/rpl_gtid.cc index 17f474c2acf..2ba9dd87a8f 100644 --- a/sql/rpl_gtid.cc +++ b/sql/rpl_gtid.cc @@ -877,6 +877,7 @@ rpl_slave_state::gtid_delete_pending(THD *thd, handler::Table_flags direct_pos; list_element *cur, **cur_ptr_ptr; bool table_opened= false; + bool index_inited= false; void *hton= (*list_ptr)->hton; thd->reset_for_next_command(); @@ -915,10 +916,14 @@ rpl_slave_state::gtid_delete_pending(THD *thd, bitmap_set_bit(table->read_set, table->field[0]->field_index); bitmap_set_bit(table->read_set, table->field[1]->field_index); - if (!direct_pos && (err= table->file->ha_index_init(0, 0))) + if (!direct_pos) { - table->file->print_error(err, MYF(0)); - goto end; + if ((err= table->file->ha_index_init(0, 0))) + { + table->file->print_error(err, MYF(0)); + goto end; + } + index_inited= true; } cur = *list_ptr; @@ -977,7 +982,14 @@ rpl_slave_state::gtid_delete_pending(THD *thd, end: if (table_opened) { - if (!direct_pos) + DBUG_ASSERT(direct_pos || index_inited || err); + /* + Index may not be initialized if there was a failure during + 'ha_index_init'. Hence check if index initialization is successful and + then invoke ha_index_end(). Ending an index which is not initialized + will lead to assert. + */ + if (index_inited) table->file->ha_index_end(); if (err || (err= ha_commit_trans(thd, FALSE)))