From da61aac5f94adcc9d1db81f04d36c902328fac74 Mon Sep 17 00:00:00 2001
From: Mark Dilger <mark.dilger@enterprisedb.com>
Date: Fri, 23 Oct 2020 13:44:49 -0700
Subject: [PATCH v23] Sanity checking line pointers

Sanity checking the offset and length of line pointers before
fetching data based on them, per report from Tom Lane about the
unsafety of what the code was doing.
---
 contrib/amcheck/t/001_verify_heapam.pl | 22 +++++++++-----
 contrib/amcheck/verify_heapam.c        | 42 ++++++++++++++++++++++++--
 2 files changed, 53 insertions(+), 11 deletions(-)

diff --git a/contrib/amcheck/t/001_verify_heapam.pl b/contrib/amcheck/t/001_verify_heapam.pl
index f8ee5384f3..7af91068e3 100644
--- a/contrib/amcheck/t/001_verify_heapam.pl
+++ b/contrib/amcheck/t/001_verify_heapam.pl
@@ -4,7 +4,7 @@ use warnings;
 use PostgresNode;
 use TestLib;
 
-use Test::More tests => 55;
+use Test::More tests => 79;
 
 my ($node, $result);
 
@@ -109,12 +109,14 @@ sub corrupt_first_page
 	  or BAIL_OUT("open failed: $!");
 	binmode $fh;
 
-	# Corrupt two line pointers.  To be stable across platforms, we use
-	# 0x55555555 and 0xAAAAAAAA for the two, which are bitwise reverses of each
-	# other.
+	# Corrupt pairs of line pointers.  To be stable across platforms, we
+	# use pairs of values that are bitwise reverses of each other.
 	seek($fh, 32, 0)
 	  or BAIL_OUT("seek failed: $!");
-	syswrite($fh, pack("L*", 0x55555555, 0xAAAAAAAA))
+	syswrite($fh, pack("L*",
+					0x55555555, 0xAAAAAAAA,
+					0x00055000, 0x000AA000,
+					0x001F0000, 0x0000F800))
 	  or BAIL_OUT("syswrite failed: $!");
 	close($fh)
 	  or BAIL_OUT("close failed: $!");
@@ -127,16 +129,20 @@ sub detects_heap_corruption
 	my ($function, $testname) = @_;
 
 	detects_corruption($function, $testname,
-		qr/line pointer redirection to item at offset \d+ exceeds maximum offset \d+/
+		qr/line pointer redirection to item at offset \d+ exceeds maximum offset \d+/,
+		qr/line pointer to page offset \d+ exceeds maximum page offset \d+/,
+		qr/line pointer to page offset \d+ with length \d+ ends beyond maximum page offset \d+/,
+		qr/line pointer redirection to item at offset \d+ precedes minimum offset \d+/,
+		qr/line pointer to page offset \d+ is not maximally aligned/,
 	);
 }
 
 sub detects_corruption
 {
-	my ($function, $testname, $re) = @_;
+	my ($function, $testname, @re) = @_;
 
 	my $result = $node->safe_psql('postgres', qq(SELECT * FROM $function));
-	like($result, $re, $testname);
+	like($result, $_, $testname) for (@re);
 }
 
 sub detects_no_corruption
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index 37b40a0404..5c01854f50 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -105,6 +105,7 @@ typedef struct HeapCheckContext
 	OffsetNumber offnum;
 	ItemId		itemid;
 	uint16		lp_len;
+	uint16		lp_off;
 	HeapTupleHeader tuphdr;
 	int			natts;
 
@@ -378,14 +379,22 @@ verify_heapam(PG_FUNCTION_ARGS)
 
 			/*
 			 * If this line pointer has been redirected, check that it
-			 * redirects to a valid offset within the line pointer array.
+			 * redirects to a valid offset within the line pointer array
 			 */
 			if (ItemIdIsRedirected(ctx.itemid))
 			{
 				OffsetNumber rdoffnum = ItemIdGetRedirect(ctx.itemid);
 				ItemId		rditem;
 
-				if (rdoffnum < FirstOffsetNumber || rdoffnum > maxoff)
+				if (rdoffnum < FirstOffsetNumber)
+				{
+					report_corruption(&ctx,
+									  psprintf("line pointer redirection to item at offset %u precedes minimum offset %u",
+											   (unsigned) rdoffnum,
+											   (unsigned) FirstOffsetNumber));
+					continue;
+				}
+				if (rdoffnum > maxoff)
 				{
 					report_corruption(&ctx,
 									  psprintf("line pointer redirection to item at offset %u exceeds maximum offset %u",
@@ -401,8 +410,35 @@ verify_heapam(PG_FUNCTION_ARGS)
 				continue;
 			}
 
-			/* Set up context information about this next tuple */
+			/* Check the line pointer points within page bounds */
 			ctx.lp_len = ItemIdGetLength(ctx.itemid);
+			ctx.lp_off = ItemIdGetOffset(ctx.itemid);
+			if (ctx.lp_off != MAXALIGN(ctx.lp_off))
+			{
+				report_corruption(&ctx,
+								  psprintf("line pointer to page offset %u is not maximally aligned",
+										   (unsigned) ctx.lp_off));
+				continue;
+			}
+			if (ctx.lp_off > BLCKSZ)
+			{
+				report_corruption(&ctx,
+								  psprintf("line pointer to page offset %u exceeds maximum page offset %u",
+										   (unsigned) ctx.lp_off,
+										   (unsigned) BLCKSZ));
+				continue;
+			}
+			if (ctx.lp_off + ctx.lp_len > BLCKSZ)
+			{
+				report_corruption(&ctx,
+								  psprintf("line pointer to page offset %u with length %u ends beyond maximum page offset %u",
+										   (unsigned) ctx.lp_off,
+										   (unsigned) ctx.lp_len,
+										   (unsigned) BLCKSZ));
+				continue;
+			}
+
+			/* It should be safe to fetch the page item */
 			ctx.tuphdr = (HeapTupleHeader) PageGetItem(ctx.page, ctx.itemid);
 			ctx.natts = HeapTupleHeaderGetNatts(ctx.tuphdr);
 
-- 
2.21.1 (Apple Git-122.3)