Commit cacdcfd0 authored by Sujatha's avatar Sujatha

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.
parent c59d6395
......@@ -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,11 +916,15 @@ 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)
{
if ((err= table->file->ha_index_init(0, 0)))
{
table->file->print_error(err, MYF(0));
goto end;
}
index_inited= true;
}
cur = *list_ptr;
cur_ptr_ptr = 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)))
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment