Re: [Maria-developers] 5ba0b11c5d1: MDEV-20257 Fix crash in Grant_table_base::init_read_record

Hi, Robert! On Aug 28, Robert Bindar wrote:

revision-id: 5ba0b11c5d1 (mariadb-10.4.7-35-g5ba0b11c5d1) parent(s): 4a490d1a993 author: Robert Bindar committer: Robert Bindar timestamp: 2019-08-27 11:38:52 +0300 message:

MDEV-20257 Fix crash in Grant_table_base::init_read_record

The bug shows up when a 10.4+ server starts on a <10.4 datadir with a crashed mysql.user table. Grant_tables::open_and_lock tries to open the list of requested tables (host, db, global_priv,..) and because global_priv doesn't exist (pre-10.4 datadir), it falls back to opening mysql.user. The first open_tables call for [host, db, global_priv,..] works just fine. The second open_tables call (trying to open mysql.user alone) sees the crashed user table and performs 3 steps: - closes all the open tables - attempts a table fix for mysql.user (succeeds) - opens the tables initially passed as arguments In an ideal world, the first step should only close the tables passed as arguments (i.e. mysql.user), but for some reasons close_tables_for_reopen makes a close_thread_tables call which closes everything in THD::open_tables (nevertheless, this is probably the intended behavior). This side effect causes all the tables opened in the first open_tables call to now be closed and Grant_table_base::init_read_record tries to read from a released table.

diff --git a/mysql-test/main/mdev20257.test b/mysql-test/main/mdev20257.test new file mode 100644 index 00000000000..8506292d57c --- /dev/null +++ b/mysql-test/main/mdev20257.test @@ -0,0 +1,51 @@ +--source include/not_embedded.inc +--echo # +--echo # MDEV 20257 Server crashes in Grant_table_base::init_read_record +--echo # upon crash-upgrade +--echo # + +let $MYSQLD_DATADIR= `select @@datadir`; + +rename table mysql.global_priv to mysql.global_priv_bak; +rename table mysql.user to mysql.user_bak; +rename table mysql.db to mysql.db_bak; +rename table mysql.proxies_priv to mysql.proxies_priv_bak; +rename table mysql.roles_mapping to mysql.roles_mapping_bak; + +--source include/shutdown_mysqld.inc + +# Bring in a crashed user table +# Ideally we should've copied only the crashed mysql.user, but this +# would make mysqld crash in some other place before hitting the +# crash spot described in MDEV-20257 which is what we're trying to +# test against.

I don't understand that. Where does it crash then? What difference does it make that you replace all tables - are they all corrupted?

+--copy_file std_data/mdev20257/user.frm $MYSQLD_DATADIR/mysql/user.frm +--copy_file std_data/mdev20257/user.MYD $MYSQLD_DATADIR/mysql/user.MYD +--copy_file std_data/mdev20257/user.MYI $MYSQLD_DATADIR/mysql/user.MYI + +--copy_file std_data/mdev20257/host.frm $MYSQLD_DATADIR/mysql/host.frm +--copy_file std_data/mdev20257/host.MYD $MYSQLD_DATADIR/mysql/host.MYD +--copy_file std_data/mdev20257/host.MYI $MYSQLD_DATADIR/mysql/host.MYI + +--copy_file std_data/mdev20257/db.frm $MYSQLD_DATADIR/mysql/db.frm +--copy_file std_data/mdev20257/db.MYD $MYSQLD_DATADIR/mysql/db.MYD +--copy_file std_data/mdev20257/db.MYI $MYSQLD_DATADIR/mysql/db.MYI +--copy_file std_data/mdev20257/db.opt $MYSQLD_DATADIR/mysql/db.opt + +--source include/start_mysqld.inc + +call mtr.add_suppression("Table \'\..mysql\.user\' is marked as crashed and should be repaired"); +call mtr.add_suppression("Checking table: \'\..mysql\.user\'"); +call mtr.add_suppression("mysql.user: 1 client is using or hasn't closed the table properly"); +call mtr.add_suppression("Missing system table mysql..*; please run mysql_upgrade to create it"); + +# Cleanup +drop table mysql.user; +drop table mysql.db; +drop table mysql.host; +rename table mysql.global_priv_bak to mysql.global_priv; +rename table mysql.user_bak to mysql.user; +rename table mysql.db_bak to mysql.db; +rename table mysql.proxies_priv_bak to mysql.proxies_priv; +rename table mysql.roles_mapping_bak to mysql.roles_mapping; + diff --git a/mysql-test/std_data/mdev20257/db.opt b/mysql-test/std_data/mdev20257/db.opt --- /dev/null +++ b/mysql-test/std_data/mdev20257/db.opt @@ -0,0 +1,2 @@ +default-character-set=latin1 +default-collation=latin1_swedish_ci

why is that?

diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index 847d2bd777b..e028d084703 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc - DBUG_RETURN(res); + first = build_open_list(tables, which_tables, lock_type, true); + + uint counter; + if (int rv= really_open(thd, first, &counter)) + DBUG_RETURN(rv); + + TABLE *user_table= tables[USER_TABLE].table; + if ((which_tables & Table_user) && !user_table) + { + close_thread_tables(thd); + first = build_open_list(tables, which_tables, lock_type, false);

Hmm... Here you always close/reopen all tables, just to account for the case when the user table is found corrupted. I'd argue that a corrupted user table happens rarely (compared to the non-corrupted), so it makes sense to optimize for the normal use case. That is, only open the user table as before. After it's opened you check if other tables are still opened - if they aren't you can close and reopen everything. This way you only close/reopen all tables if the user table was corrupted. Makes sense?

+ if (int rv= really_open(thd, first, &counter)) + DBUG_RETURN(rv); + + p_user_table= &m_user_table_tabular; + user_table= tables[USER_TABLE + 1].table; + }

Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org