From 8ede9b3ae542068cc02ca27ab32eb76fcfab942f Mon Sep 17 00:00:00 2001 From: Sergei Golubchik Date: Wed, 19 Dec 2018 15:23:54 +0100 Subject: [PATCH 1/4] MDEV-17975 Assertion `! is_set()' or `!is_set() || (m_status == DA_OK_BULK && is_bulk_op())' failed upon REVOKE under LOCK TABLE open_grant_tables() returns -1/0/1, where -1 is an error, while 1 is not. Don't store it's return value in bool --- mysql-test/r/grant5.result | 7 +++++++ mysql-test/t/grant5.test | 10 ++++++++++ sql/sql_acl.cc | 9 +++++---- 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/mysql-test/r/grant5.result b/mysql-test/r/grant5.result index d7f3b6812bb..8c32d90a640 100644 --- a/mysql-test/r/grant5.result +++ b/mysql-test/r/grant5.result @@ -16,3 +16,10 @@ show grants for foo@'%'; ERROR 42000: Access denied for user 'test'@'%' to database 'mysql' drop user test, foo; drop role foo; +CREATE TABLE t1 (a INT); +LOCK TABLE t1 WRITE; +REVOKE EXECUTE ON PROCEDURE sp FROM u; +ERROR HY000: Table 'user' was not locked with LOCK TABLES +REVOKE PROCESS ON *.* FROM u; +ERROR HY000: Table 'user' was not locked with LOCK TABLES +DROP TABLE t1; diff --git a/mysql-test/t/grant5.test b/mysql-test/t/grant5.test index 14f2fd65020..649bba7d1ca 100644 --- a/mysql-test/t/grant5.test +++ b/mysql-test/t/grant5.test @@ -23,3 +23,13 @@ show grants for foo@'%'; # user drop user test, foo; drop role foo; +# +# MDEV-17975 Assertion `! is_set()' or `!is_set() || (m_status == DA_OK_BULK && is_bulk_op())' failed upon REVOKE under LOCK TABLE +# +CREATE TABLE t1 (a INT); +LOCK TABLE t1 WRITE; +--error ER_TABLE_NOT_LOCKED +REVOKE EXECUTE ON PROCEDURE sp FROM u; +--error ER_TABLE_NOT_LOCKED +REVOKE PROCESS ON *.* FROM u; +DROP TABLE t1; diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index 0f21a6bc134..784f2338a55 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -786,8 +786,7 @@ static const int Table_procs_priv= 1 << PROCS_PRIV_TABLE; static const int Table_proxies_priv= 1 << PROXIES_PRIV_TABLE; static const int Table_roles_mapping= 1 << ROLES_MAPPING_TABLE; -static int open_grant_tables(THD *thd, TABLE_LIST *tables, - enum thr_lock_type lock_type, int tables_to_open); +static int open_grant_tables(THD *, TABLE_LIST *, enum thr_lock_type, int); const LEX_STRING acl_table_names[]= // matches enum_acl_tables { @@ -6030,7 +6029,8 @@ bool mysql_routine_grant(THD *thd, TABLE_LIST *table_list, bool is_proc, List_iterator str_list (user_list); LEX_USER *Str, *tmp_Str; TABLE_LIST tables[TABLES_MAX]; - bool create_new_users= 0, result; + bool create_new_users= 0; + int result; char *db_name, *table_name; DBUG_ENTER("mysql_routine_grant"); @@ -6465,7 +6465,8 @@ bool mysql_grant(THD *thd, const char *db, List &list, List_iterator str_list (list); LEX_USER *Str, *tmp_Str, *proxied_user= NULL; char tmp_db[SAFE_NAME_LEN+1]; - bool create_new_users=0, result; + bool create_new_users=0; + int result; TABLE_LIST tables[TABLES_MAX]; DBUG_ENTER("mysql_grant"); From ed36fc353f374892c225d191f5525f439031d939 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Thu, 20 Dec 2018 13:33:09 +0200 Subject: [PATCH 2/4] MDEV-18025: Detect corrupted innodb_page_compression=zlib pages In MDEV-13103, I made a mistake in the error handling of page_compressed=1 decryption when the default innodb_compression_algorithm=zlib is used. Due to this mistake, with certain versions of zlib, MariaDB would fail to detect a corrupted page. The problem was uncovered by the following tests: mariabackup.unencrypted_page_compressed mariabackup.encrypted_page_compressed --- storage/innobase/fil/fil0pagecompress.cc | 8 ++++---- storage/xtradb/fil/fil0pagecompress.cc | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/storage/innobase/fil/fil0pagecompress.cc b/storage/innobase/fil/fil0pagecompress.cc index 25cd8e28a91..101f8fb0f31 100644 --- a/storage/innobase/fil/fil0pagecompress.cc +++ b/storage/innobase/fil/fil0pagecompress.cc @@ -341,14 +341,14 @@ UNIV_INTERN ulint fil_page_decompress(byte* tmp_buf, byte* buf) case PAGE_ZLIB_ALGORITHM: { uLong len = srv_page_size; - if (Z_OK != uncompress(tmp_buf, &len, + if (Z_OK == uncompress(tmp_buf, &len, buf + header_len, uLong(actual_size)) - && len != srv_page_size) { - return 0; + && len == srv_page_size) { + break; } } - break; + return 0; #ifdef HAVE_LZ4 case PAGE_LZ4_ALGORITHM: if (LZ4_decompress_safe(reinterpret_cast(buf) diff --git a/storage/xtradb/fil/fil0pagecompress.cc b/storage/xtradb/fil/fil0pagecompress.cc index 25cd8e28a91..101f8fb0f31 100644 --- a/storage/xtradb/fil/fil0pagecompress.cc +++ b/storage/xtradb/fil/fil0pagecompress.cc @@ -341,14 +341,14 @@ UNIV_INTERN ulint fil_page_decompress(byte* tmp_buf, byte* buf) case PAGE_ZLIB_ALGORITHM: { uLong len = srv_page_size; - if (Z_OK != uncompress(tmp_buf, &len, + if (Z_OK == uncompress(tmp_buf, &len, buf + header_len, uLong(actual_size)) - && len != srv_page_size) { - return 0; + && len == srv_page_size) { + break; } } - break; + return 0; #ifdef HAVE_LZ4 case PAGE_LZ4_ALGORITHM: if (LZ4_decompress_safe(reinterpret_cast(buf) From 9f4a4cb401980e2bd321c0680595e81087ad60d5 Mon Sep 17 00:00:00 2001 From: Vladislav Vaintroub Date: Thu, 20 Dec 2018 14:31:18 +0100 Subject: [PATCH 3/4] Cleanup recent mariabackup validation patches. - Refactor code to isolate page validation in page_is_corrupted() function. - Introduce --extended-validation parameter(default OFF) for mariabackup --backup to enable decryption of encrypted uncompressed pages during backup. - mariabackup would still always check checksum on encrypted data, it is needed to detect partially written pages. --- extra/mariabackup/fil_cur.cc | 155 +++++++++--------- extra/mariabackup/xtrabackup.cc | 10 ++ extra/mariabackup/xtrabackup.h | 1 + .../encrypted_page_corruption.test | 8 +- 4 files changed, 100 insertions(+), 74 deletions(-) diff --git a/extra/mariabackup/fil_cur.cc b/extra/mariabackup/fil_cur.cc index 637acfd2e22..b4393f7ffdd 100644 --- a/extra/mariabackup/fil_cur.cc +++ b/extra/mariabackup/fil_cur.cc @@ -265,6 +265,87 @@ xb_fil_cur_open( return(XB_FIL_CUR_SUCCESS); } +static bool page_is_corrupted(byte *page, ulint page_no, xb_fil_cur_t *cursor, fil_space_t *space) +{ + byte tmp_frame[UNIV_PAGE_SIZE_MAX]; + byte tmp_page[UNIV_PAGE_SIZE_MAX]; + + ulint page_type = mach_read_from_2(page + FIL_PAGE_TYPE); + + /* We ignore the doublewrite buffer pages.*/ + if (cursor->space_id == TRX_SYS_SPACE + && page_no >= FSP_EXTENT_SIZE + && page_no < FSP_EXTENT_SIZE * 3) { + return false; + } + + /* Validate page number. */ + if (mach_read_from_4(page + FIL_PAGE_OFFSET) != page_no + && space->id != TRX_SYS_SPACE) { + /* On pages that are not all zero, the + page number must match. + + There may be a mismatch on tablespace ID, + because files may be renamed during backup. + We disable the page number check + on the system tablespace, because it may consist + of multiple files, and here we count the pages + from the start of each file.) + + The first 38 and last 8 bytes are never encrypted. */ + const ulint* p = reinterpret_cast(page); + const ulint* const end = reinterpret_cast( + page + cursor->page_size); + do { + if (*p++) { + return true; + } + } while (p != end); + + /* Whole zero page is valid. */ + return false; + } + + /* Validate encrypted pages. */ + if (mach_read_from_4(page + FIL_PAGE_FILE_FLUSH_LSN_OR_KEY_VERSION) && + (space->crypt_data && space->crypt_data->type!= CRYPT_SCHEME_UNENCRYPTED)) { + + if (!fil_space_verify_crypt_checksum(page, cursor->zip_size)) + return true; + + /* Compressed encrypted need to be unencryped and uncompressed for verification. */ + if (page_type != FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED && !opt_extended_validation) + return false; + + memcpy(tmp_page, page, cursor->page_size); + + bool decrypted = false; + if (!fil_space_decrypt(space, tmp_frame,tmp_page, &decrypted)) { + return true; + } + + if (page_type != FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED) { + return buf_page_is_corrupted(true, tmp_page, cursor->zip_size, space); + } + } + + if (page_type == FIL_PAGE_PAGE_COMPRESSED) { + memcpy(tmp_page, page, cursor->page_size); + } + + if (page_type == FIL_PAGE_PAGE_COMPRESSED || page_type == FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED) { + ulint decomp = fil_page_decompress(tmp_frame, tmp_page); + page_type = mach_read_from_2(tmp_page + FIL_PAGE_TYPE); + + return (!decomp + || (decomp != srv_page_size && cursor->zip_size) + || page_type == FIL_PAGE_PAGE_COMPRESSED + || page_type == FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED + || buf_page_is_corrupted(true, tmp_page, cursor->zip_size, space)); + } + + return buf_page_is_corrupted(true, page, cursor->zip_size, space); +} /************************************************************************ Reads and verifies the next block of pages from the source file. Positions the cursor after the last read non-corrupted page. @@ -284,8 +365,6 @@ xb_fil_cur_read( xb_fil_cur_result_t ret; ib_int64_t offset; ib_int64_t to_read; - byte tmp_frame[UNIV_PAGE_SIZE_MAX]; - byte tmp_page[UNIV_PAGE_SIZE_MAX]; cursor->read_filter->get_next_batch(&cursor->read_filter_ctxt, &offset, &to_read); @@ -347,78 +426,8 @@ read_retry: for (page = cursor->buf, i = 0; i < npages; page += cursor->page_size, i++) { ulint page_no = cursor->buf_page_no + i; - ulint page_type = mach_read_from_2(page + FIL_PAGE_TYPE); - if (cursor->space_id == TRX_SYS_SPACE - && page_no >= FSP_EXTENT_SIZE - && page_no < FSP_EXTENT_SIZE * 3) { - /* We ignore the doublewrite buffer pages */ - } else if (mach_read_from_4(page + FIL_PAGE_OFFSET) != page_no - && space->id != TRX_SYS_SPACE) { - /* On pages that are not all zero, the - page number must match. - - There may be a mismatch on tablespace ID, - because files may be renamed during backup. - We disable the page number check - on the system tablespace, because it may consist - of multiple files, and here we count the pages - from the start of each file.) - - The first 38 and last 8 bytes are never encrypted. */ - const ulint* p = reinterpret_cast(page); - const ulint* const end = reinterpret_cast( - page + cursor->page_size); - do { - if (*p++) { - goto corrupted; - } - } while (p != end); - } else if (mach_read_from_4( - page - + FIL_PAGE_FILE_FLUSH_LSN_OR_KEY_VERSION) - && space->crypt_data - && space->crypt_data->type - != CRYPT_SCHEME_UNENCRYPTED - && fil_space_verify_crypt_checksum( - page, cursor->zip_size)) { - bool decrypted = false; - - memcpy(tmp_page, page, cursor->page_size); - - if (!fil_space_decrypt(space, tmp_frame, - tmp_page, &decrypted)) { - goto corrupted; - } - - if (page_type == FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED) { - goto page_decomp; - } - - if (buf_page_is_corrupted( - true, tmp_page, cursor->zip_size, space)) { - goto corrupted; - } - } else if (page_type == FIL_PAGE_PAGE_COMPRESSED) { - memcpy(tmp_page, page, cursor->page_size); -page_decomp: - ulint decomp = fil_page_decompress(tmp_frame, tmp_page); - page_type = mach_read_from_2(tmp_page + FIL_PAGE_TYPE); - - if (!decomp - || (decomp != srv_page_size && cursor->zip_size) - || page_type == FIL_PAGE_PAGE_COMPRESSED - || page_type == FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED - || buf_page_is_corrupted(true, tmp_page, - cursor->zip_size, - space)) { - goto corrupted; - } - - } else if (page_type == FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED - || buf_page_is_corrupted(true, page, - cursor->zip_size, space)) { -corrupted: + if (page_is_corrupted(page, page_no, cursor, space)){ retry_count--; if (retry_count == 0) { diff --git a/extra/mariabackup/xtrabackup.cc b/extra/mariabackup/xtrabackup.cc index be022f7afa2..d2b241a28f5 100644 --- a/extra/mariabackup/xtrabackup.cc +++ b/extra/mariabackup/xtrabackup.cc @@ -206,6 +206,7 @@ char* log_ignored_opt = NULL; extern my_bool opt_use_ssl; my_bool opt_ssl_verify_server_cert; +my_bool opt_extended_validation; /* === metadata of backup === */ #define XTRABACKUP_METADATA_FILENAME "xtrabackup_checkpoints" @@ -510,6 +511,7 @@ enum options_xtrabackup OPT_XTRA_DATABASES_FILE, OPT_XTRA_CREATE_IB_LOGFILE, OPT_XTRA_PARALLEL, + OPT_XTRA_EXTENDED_VALIDATION, OPT_XTRA_STREAM, OPT_XTRA_COMPRESS, OPT_XTRA_COMPRESS_THREADS, @@ -976,6 +978,14 @@ struct my_option xb_server_options[] = (G_PTR*) &xtrabackup_parallel, (G_PTR*) &xtrabackup_parallel, 0, GET_INT, REQUIRED_ARG, 1, 1, INT_MAX, 0, 0, 0}, + {"extended_validation", OPT_XTRA_EXTENDED_VALIDATION, + "Enable extended validation for Innodb data pages during backup phase." + "Will slow down backup considerably, in case encryption is used.", + (G_PTR*)&opt_extended_validation, + (G_PTR*)&opt_extended_validation, + 0, GET_BOOL, NO_ARG, FALSE, 0, 0, 0, 0, 0}, + + {"log", OPT_LOG, "Ignored option for MySQL option compatibility", (G_PTR*) &log_ignored_opt, (G_PTR*) &log_ignored_opt, 0, GET_STR, OPT_ARG, 0, 0, 0, 0, 0, 0}, diff --git a/extra/mariabackup/xtrabackup.h b/extra/mariabackup/xtrabackup.h index 9af7c861dd7..2af5396fa06 100644 --- a/extra/mariabackup/xtrabackup.h +++ b/extra/mariabackup/xtrabackup.h @@ -128,6 +128,7 @@ extern my_bool opt_noversioncheck; extern my_bool opt_no_backup_locks; extern my_bool opt_decompress; extern my_bool opt_remove_original; +extern my_bool opt_extended_validation; extern char *opt_incremental_history_name; extern char *opt_incremental_history_uuid; diff --git a/mysql-test/suite/mariabackup/encrypted_page_corruption.test b/mysql-test/suite/mariabackup/encrypted_page_corruption.test index f87e12323da..7ee4e8c5d58 100644 --- a/mysql-test/suite/mariabackup/encrypted_page_corruption.test +++ b/mysql-test/suite/mariabackup/encrypted_page_corruption.test @@ -60,7 +60,7 @@ let $backuplog=$MYSQLTEST_VARDIR/tmp/backup.log; --disable_result_log --error 1 -exec $XTRABACKUP --defaults-file=$MYSQLTEST_VARDIR/my.cnf --backup --target-dir=$targetdir > $backuplog; +exec $XTRABACKUP --defaults-file=$MYSQLTEST_VARDIR/my.cnf --backup --extended-validation --target-dir=$targetdir > $backuplog; --enable_result_log @@ -68,6 +68,12 @@ exec $XTRABACKUP --defaults-file=$MYSQLTEST_VARDIR/my.cnf --backup --target-dir= --let SEARCH_FILE=$backuplog --source include/search_pattern_in_file.inc remove_file $backuplog; +rmdir $targetdir; + +# Due to very constructed nature of the "corruption" (faking checksums), the "corruption" won't be found without --extended-validation +--disable_result_log +exec $XTRABACKUP --defaults-file=$MYSQLTEST_VARDIR/my.cnf --backup --target-dir=$targetdir; +--enable_result_log drop table t1; rmdir $targetdir; From 40a094e4a8fdea2f07231faa0d5911ed023a320b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Fri, 21 Dec 2018 09:40:36 +0200 Subject: [PATCH 4/4] Relax a too tight suppression When using a wrong key, all encrypted pages will look corrupted, and occasionally the test may access other pages than the clustered index root page. --- mysql-test/suite/encryption/r/innodb-missing-key.result | 2 +- mysql-test/suite/encryption/t/innodb-missing-key.test | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/mysql-test/suite/encryption/r/innodb-missing-key.result b/mysql-test/suite/encryption/r/innodb-missing-key.result index 45a01f03c2b..da19f65dfb5 100644 --- a/mysql-test/suite/encryption/r/innodb-missing-key.result +++ b/mysql-test/suite/encryption/r/innodb-missing-key.result @@ -1,5 +1,5 @@ call mtr.add_suppression("InnoDB: The page \\[page id: space=[1-9][0-9]*, page number=[1-9][0-9]*\\] in file '.*test.t[123]\\.ibd' cannot be decrypted\\."); -call mtr.add_suppression("InnoDB: Encrypted page [1-9][0-9]*:3 in file .*test.t[12].ibd looks corrupted; key_version=1"); +call mtr.add_suppression("InnoDB: Encrypted page [1-9][0-9]*:[1-9][0-9]* in file .*test.t[12].ibd looks corrupted; key_version=1"); # Start server with keys2.txt CREATE TABLE t1(a int not null primary key auto_increment, b varchar(128)) engine=innodb ENCRYPTED=YES ENCRYPTION_KEY_ID=19; diff --git a/mysql-test/suite/encryption/t/innodb-missing-key.test b/mysql-test/suite/encryption/t/innodb-missing-key.test index c68604fdcf5..72c24802ce9 100644 --- a/mysql-test/suite/encryption/t/innodb-missing-key.test +++ b/mysql-test/suite/encryption/t/innodb-missing-key.test @@ -8,7 +8,7 @@ # call mtr.add_suppression("InnoDB: The page \\[page id: space=[1-9][0-9]*, page number=[1-9][0-9]*\\] in file '.*test.t[123]\\.ibd' cannot be decrypted\\."); -call mtr.add_suppression("InnoDB: Encrypted page [1-9][0-9]*:3 in file .*test.t[12].ibd looks corrupted; key_version=1"); +call mtr.add_suppression("InnoDB: Encrypted page [1-9][0-9]*:[1-9][0-9]* in file .*test.t[12].ibd looks corrupted; key_version=1"); --echo --echo # Start server with keys2.txt