[MDEV-17957] Make Innodb_checksum_algorithm stricter for strict_* values Created: 2018-12-10  Updated: 2020-08-25  Resolved: 2018-12-13

Status: Closed
Project: MariaDB Server
Component/s: Storage Engine - InnoDB, Storage Engine - XtraDB
Affects Version/s: 10.1.37, 10.2, 10.3
Fix Version/s: 10.4.1, 10.1.38, 10.0.38, 10.2.20, 10.3.12

Type: Bug Priority: Major
Reporter: Thirunarayanan Balathandayuthapani Assignee: Marko Mäkelä
Resolution: Fixed Votes: 0
Labels: None

Issue Links:
Problem/Incident
causes MDEV-16896 encryption.innodb-checksum-algorithm ... Closed
Relates
relates to MDEV-13542 Crashing on a corrupted page is unhel... Closed
relates to MDEV-17638 Improve error message about corruptio... Closed
relates to MDEV-18529 InnoDB wrongly skips decryption of en... Closed

 Description   

innodb_checksum_algorithm checks for the another algorithm even though the value is
specified as strict_. It makes the innodb_checksum_algorithm weaker for strict_ values.



 Comments   
Comment by Thirunarayanan Balathandayuthapani [ 2018-12-10 ]

Patch is present in bb-10.1-thiru https://github.com/MariaDB/server/commit/6c8538ac219cd9248ddaaaa59251de937d354b79

Comment by Marko Mäkelä [ 2018-12-11 ]

Thanks! I think that we can simplify this a little more by assuming that when the crc32 checksum is being used, it has been written by MySQL 5.6.3 or later.

Please submit a 10.0 version as well, and observe my review comments in GitHub.

For the 10.1 version, please implement strict checksum variants also for fil_space_verify_crypt_checksum().

Comment by Thirunarayanan Balathandayuthapani [ 2018-12-11 ]

Put the patch for 10.0 and addressed the review comments for 10.1 patch as well.
Both are present in bb-10.0-thiru and bb-10.1-thiru

Comment by Thirunarayanan Balathandayuthapani [ 2018-12-13 ]

In 10.2, there are few dead code:

bool
buf_page_is_checksum_valid_crc32(
        const byte*                     read_buf,
        ulint                           checksum_field1,
        ulint                           checksum_field2,
        bool                            use_legacy_big_endian)
{
        const uint32_t  crc32 = buf_calc_page_crc32(read_buf,
                                                    use_legacy_big_endian);
 
 
        if (checksum_field1 != checksum_field2) {
                goto invalid;
        }
 
        if (checksum_field1 == crc32) {
                return(true);
        } else {
                const uint32_t  crc32_legacy = buf_calc_page_crc32(read_buf, true);
 
                if (checksum_field1 == crc32_legacy) {
                        return(true);
                }
        }
 
It checks for both little endian and big endian variant of crc32 algorithm.
 
But in buf_page_is_corrupted():
 
                if (buf_page_is_checksum_valid_crc32(read_buf,
                        checksum_field1, checksum_field2, false)) {
                        return(false);
                }
 
                ...............
                .................
 
               /* We need to check whether the stored checksum matches legacy
                big endian checksum or Innodb checksum. We optimize the order
                based on earlier results. if earlier we have found pages
                matching legacy big endian checksum, we try to match it first.
                Otherwise we check innodb checksum first. */
                if (legacy_big_endian_checksum) {
                        if (buf_page_is_checksum_valid_crc32(read_buf,
                                checksum_field1, checksum_field2, true)) {
 
                                return(false);
                        }
                        legacy_checksum_checked = true;
                }
 
                ..............
                ..............
 
                /* If legacy checksum is not checked, do it now. */
                if (!legacy_checksum_checked && buf_page_is_checksum_valid_crc32(
                        read_buf, checksum_field1, checksum_field2, true)) {
 
                        legacy_big_endian_checksum = true;
                        return(false);
                }
 
                Above code can never be executed because these condition already checked as a part of
                buf_page_is_checksum_valid_crc32().

Another issue with page_zip_verify_checksum():

             /* We need to check whether the stored checksum matches legacy
                big endian checksum or Innodb checksum. We optimize the order
                based on earlier results. if earlier we have found pages
                matching legacy big endian checksum, we try to match it first.
                Otherwise we check innodb checksum first. */
                if (legacy_big_endian_checksum) {
                        const uint32_t calculated =
                                page_zip_calc_checksum(data, size, curr_algo, true);
                        if (stored == calculated) {
 
                                return(TRUE);
                        }
                        legacy_checksum_checked = true;
                }
 
            ............
            .............
 
                /* If legacy checksum is not checked, do it now. */
                if ((legacy_checksum_checked
                     && stored == calculated)) {
                        legacy_big_endian_checksum = true;
                        return(TRUE);
                }
 
            It is also kind of dead code because it should check if (!legacy_checksum_checked).
            may be merging error from mysql-5.7

Comment by Thirunarayanan Balathandayuthapani [ 2018-12-13 ]

Fixed the above issues in bb-10.2-23578 branch.

Comment by Marko Mäkelä [ 2018-12-13 ]

I merged this up to 10.2 until now.

Generated at Thu Feb 08 08:40:25 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.