maria-developers team mailing list archive

[Marko Mäkelä <marko.makela@xxxxxxxxxxx>]

On Tue, Feb 7, 2017 at 8:12 PM, jan <jan.lindstrom@xxxxxxxxxxx> wrote:
>  # Write file to make mysql-test-run.pl expect the "crash", but don't
>  # start it until it's told to
> -# We give 30 seconds to do a clean shutdown because we do not want
> -# to redo apply the pages of t1.ibd at the time of recovery.
> -# We want SQL to initiate the first access to t1.ibd.
>  # Wait until disconnected.

Remove all these messages.

> diff --git a/storage/innobase/fil/fil0crypt.cc b/storage/innobase/fil/fil0crypt.cc
> index fed1cf94d84..4299600709a 100644
> --- a/storage/innobase/fil/fil0crypt.cc
> +++ b/storage/innobase/fil/fil0crypt.cc
> @@ -982,7 +982,16 @@ fil_space_verify_crypt_checksum(
>         /* Declare empty pages non-corrupted */
>         if (checksum == 0
>             && *reinterpret_cast<const ib_uint64_t*>(page + FIL_PAGE_LSN) == 0) {
> -               return (true);
> +               /* make sure that the page is really empty */
> +               for (ulint i = 0; i < UNIV_PAGE_SIZE; i++) {
> +                       if (page[i] != 0) {
> +                               ib_logf(IB_LOG_LEVEL_INFO,
> +                                       "Checksum fields zero but page is not empty.");
> +
> +                               return(false);
> +                       }
> +               }
> +               return(true);
>         }

Please use buf_page_is_zeroes() instead of duplicating the code.
Do not spam the error log if the checksum happens to be 0.
I guess we could also have FIL_PAGE_LSN at 0 when importing a file.

> @@ -1016,16 +1025,28 @@ fil_space_verify_crypt_checksum(
>         bool encrypted = (checksum == cchecksum1 || checksum == cchecksum2
>                 || checksum == BUF_NO_CHECKSUM_MAGIC);
>
> -       /* Old InnoDB versions did not initialize
> -       FIL_PAGE_FILE_FLUSH_LSN field so there could be garbage
> -       and above checksum check could produce false positive.
> -       Thus we also check does the traditional stored
> -       checksum fields match the calculated one. Both of these
> -       could naturally produce false positive but then
> -       we just decrypt the page and after that corrupted
> -       pages very probable stay corrupted and valid
> -       pages very probable stay valid.
> +       /* MySQL 5.6 and MariaDB 10.0 and 10.1 will write an LSN to the
> +       first page of each system tablespace file at
> +       FIL_PAGE_FILE_FLUSH_LSN offset. On other pages and in other files,
> +       the field might have been uninitialized until MySQL 5.5. In MySQL 5.7
> +       (and MariaDB Server 10.2.2) WL#7990 stopped writing the field for other
> +       than page 0 of the system tablespace.

I think that we should conduct some research to make the comments more
useful and reliable. Looking at the MySQL 3.23.49 source code, I see
that we indeed did not initialize FIL_PAGE_FILE_FLUSH_LSN (or
FIL_PAGE_ARCH_LOG_NO for that matter) in the system tablespace. The
latter field was repurposed as SPACE_ID later. This means that the
system tablespace (which was the only option before MySQL 4.1) can
contain garbage in the FIL_PAGE_FILE_FLUSH_LSN field, except if we can
infer from some other attributes of the system tablespace that it was
created by MySQL 5.5 or later.

The next interesting MySQL version to check is 4.1.2 where
innodb_file_per_table was introduced. The oldest version in the 4.1
series that I was able to find is 4.1.21. It indeed does not make any
effort to initialize FIL_PAGE_FILE_FLUSH_LSN on other pages than the
first one. Because the buffer pool can be used for general-purpose
memory allocations (BUF_BLOCK_MEMORY), it is possible that the
FLUSH_LSN field will be written as nonzero garbage, even if we were
able to assume that the initially allocated memory was
zero-initialized by the operating system.

So, the above comment is correct.

> +       Starting from MariaDB 10.1 the fiedl has been repurposed for
> +       encryption key_version.

Correct the typo "fiedl".
> +
> +       Starting with MySQL 5.7 (and MariaDB Server 10.2), the
> +       field has been repurposed for SPATIAL INDEX pages.

Refer to the field name FIL_RTREE_SPLIT_SEQ_NUM in the comment, so
that it can be found easily by grep.

> +
> +       Note that FIL_PAGE_FILE_FLUSH_LSN is not included in the InnoDB page
> +       checksum.
> +
> +       Thus,FIL_PAGE_FILE_FLUSH_LSN could contain uninitialized value, 0,
> +       correct key_version or spatial index information, thus we also
> +       verify page using traditional checksum check to detect if page
> +       would be valid but not encrypted.
>         */

Well, 10.1 cannot handle spatial index information. I would simply say:

"Thus, FIL_PAGE_FILE_FLUSH_LSN could contain any value. While the
field would usually be 0 for pages that are not encrypted, we cannot
assume that a nonzero value means that the page is encrypted.
Therefore we must validate the page both as encrypted and unencrypted
when FIL_PAGE_FILE_FLUSH_LSN does not contain 0."

OK to push with these changes.

Note: I should still go through the whole logic, to be able to say if
we really fixed the issues. I currently believe that more work is
needed, but that should be tracked as a separate issue.

Marko
-- 
DON’T MISS
M|17
April 11 - 12, 2017
The Conrad Hotel
New York City
https://m17.mariadb.com/

Marko Mäkelä, Lead Developer InnoDB
MariaDB Corporation