From 303448bc912486f4766129cc407a5077a3ca4359 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= <marko.makela@mariadb.com>
Date: Mon, 28 Mar 2022 13:36:36 +0300
Subject: MDEV-27931: buf_page_is_corrupted() wrongly claims corruption

In commit 437da7bc54daa131b46900128ebe3ad2ca25c11a (MDEV-19534),
the default value of the global variable srv_checksum_algorithm
in innochecksum was changed from SRV_CHECKSUM_ALGORITHM_INNODB
to implied 0 (innodb_checksum_algorithm=crc32). As a result,
the function buf_page_is_corrupted() would by default invoke
buf_calc_page_crc32() in innochecksum, and crc32_inited would hold.

This would cause "innochecksum" to fail on a particular page.

The actual problem is older, introduced in 2011 in
mysql/mysql-server@17e497bdb793bc6b8360aa1c626dcd8bb5cfad1b
(MySQL 5.6.3). It should affect the validation of pages of old
data files that were written with innodb_checksum_algorithm=innodb.
When using innodb_checksum_algorithm=crc32 (the default setting
since MariaDB Server 10.2), some valid pages would be rejected
only because exactly one of the two checksum fields accidentally
matches the innodb_checksum_algorithm=crc32 value.

buf_page_is_corrupted(): Simplify the logic of non-strict
checksum validation, by always invoking buf_calc_page_crc32().
Remove a bogus condition that if only one of the checksum fields
contains the value returned by buf_calc_page_crc32(), the page
is corrupted.
---
 storage/innobase/buf/buf0buf.cc | 96 +++++++++++------------------------------
 1 file changed, 24 insertions(+), 72 deletions(-)

diff --git a/storage/innobase/buf/buf0buf.cc b/storage/innobase/buf/buf0buf.cc
index 505539f0217..679f12ca6d1 100644
--- a/storage/innobase/buf/buf0buf.cc
+++ b/storage/innobase/buf/buf0buf.cc
@@ -977,8 +977,6 @@ buf_page_is_corrupted(
 #endif
 	size_t		checksum_field1 = 0;
 	size_t		checksum_field2 = 0;
-	uint32_t	crc32 = 0;
-	bool		crc32_inited = false;
 
 	ulint page_type = mach_read_from_2(read_buf + FIL_PAGE_TYPE);
 
@@ -1104,8 +1102,13 @@ buf_page_is_corrupted(
 	case SRV_CHECKSUM_ALGORITHM_STRICT_NONE:
 		return !buf_page_is_checksum_valid_none(
 			read_buf, checksum_field1, checksum_field2);
+	case SRV_CHECKSUM_ALGORITHM_NONE:
+		/* should have returned false earlier */
+		break;
 	case SRV_CHECKSUM_ALGORITHM_CRC32:
 	case SRV_CHECKSUM_ALGORITHM_INNODB:
+		const uint32_t crc32 = buf_calc_page_crc32(read_buf);
+
 		if (buf_page_is_checksum_valid_none(read_buf,
 			checksum_field1, checksum_field2)) {
 #ifdef UNIV_INNOCHECKSUM
@@ -1121,7 +1124,7 @@ buf_page_is_corrupted(
 					" crc32 = " UINT32PF "; recorded = " ULINTPF ";\n",
 					cur_page_num,
 					buf_calc_page_new_checksum(read_buf),
-					buf_calc_page_crc32(read_buf),
+					crc32,
 					checksum_field1);
 			}
 #endif /* UNIV_INNOCHECKSUM */
@@ -1138,84 +1141,33 @@ buf_page_is_corrupted(
 		    != mach_read_from_4(read_buf + FIL_PAGE_LSN)
 		    && checksum_field2 != BUF_NO_CHECKSUM_MAGIC) {
 
-			if (curr_algo == SRV_CHECKSUM_ALGORITHM_CRC32) {
-				DBUG_EXECUTE_IF(
-					"page_intermittent_checksum_mismatch", {
-					static int page_counter;
-					if (page_counter++ == 2) {
-						checksum_field2++;
-					}
-				});
-
-				crc32 = buf_page_check_crc32(read_buf,
-							     checksum_field2);
-				crc32_inited = true;
-
-				if (checksum_field2 != crc32
-				    && checksum_field2
-				       != buf_calc_page_old_checksum(read_buf)) {
-					return true;
-				}
-			} else {
-				ut_ad(curr_algo
-				      == SRV_CHECKSUM_ALGORITHM_INNODB);
-
-				if (checksum_field2
-				    != buf_calc_page_old_checksum(read_buf)) {
-					crc32 = buf_page_check_crc32(
-						read_buf, checksum_field2);
-					crc32_inited = true;
-
-					if (checksum_field2 != crc32) {
-						return true;
-					}
-				}
+			DBUG_EXECUTE_IF(
+				"page_intermittent_checksum_mismatch", {
+				static int page_counter;
+				if (page_counter++ == 2) return true;
+			});
+
+			if ((checksum_field1 != crc32
+			     || checksum_field2 != crc32)
+			    && checksum_field2
+			    != buf_calc_page_old_checksum(read_buf)) {
+				return true;
 			}
 		}
 
-		if (checksum_field1 == 0
-		    || checksum_field1 == BUF_NO_CHECKSUM_MAGIC) {
-		} else if (curr_algo == SRV_CHECKSUM_ALGORITHM_CRC32) {
-			if (!crc32_inited) {
-				crc32 = buf_page_check_crc32(
-					read_buf, checksum_field2);
-				crc32_inited = true;
-			}
-
-			if (checksum_field1 != crc32
+		switch (checksum_field1) {
+		case 0:
+		case BUF_NO_CHECKSUM_MAGIC:
+			break;
+		default:
+			if ((checksum_field1 != crc32
+			     || checksum_field2 != crc32)
 			    && checksum_field1
 			    != buf_calc_page_new_checksum(read_buf)) {
 				return true;
 			}
-		} else {
-			ut_ad(curr_algo == SRV_CHECKSUM_ALGORITHM_INNODB);
-
-			if (checksum_field1
-			    != buf_calc_page_new_checksum(read_buf)) {
-
-				if (!crc32_inited) {
-					crc32 = buf_page_check_crc32(
-						read_buf, checksum_field2);
-					crc32_inited = true;
-				}
-
-				if (checksum_field1 != crc32) {
-					return true;
-				}
-			}
-		}
-
-		if (crc32_inited
-		    && ((checksum_field1 == crc32
-			 && checksum_field2 != crc32)
-			|| (checksum_field1 != crc32
-			    && checksum_field2 == crc32))) {
-			return true;
 		}
 
-		break;
-	case SRV_CHECKSUM_ALGORITHM_NONE:
-		/* should have returned false earlier */
 		break;
 	}
 
-- 
cgit v1.2.1