Fix more portability issues in new amcheck code.

tglsfdc · tglsfdc · commit 321633e17b07 · 2020-10-23T19:08:01.000-04:00
verify_heapam() wasn't being careful to sanity-check tuple line pointers before using them, resulting in SIGBUS on alignment-picky architectures. Fix that, add some more test coverage. Mark Dilger, some tweaking by me Discussion: https://postgr.es/m/30B8E99A-2D9C-48D4-A55C-741C9D5F1563@enterprisedb.com
diff --git a/contrib/amcheck/t/001_verify_heapam.pl b/contrib/amcheck/t/001_verify_heapam.pl
@@ -4,7 +4,7 @@
 use PostgresNode;
 use TestLib;
 
-use Test::More tests => 55;
+use Test::More tests => 79;
 
 my ($node, $result);
 
@@ -109,13 +109,17 @@ 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 some line pointers.  The values are chosen to hit the
+	# various line-pointer-corruption checks in verify_heapam.c
+	# on both little-endian and big-endian architectures.
 	seek($fh, 32, 0)
 	  or BAIL_OUT("seek failed: $!");
-	syswrite($fh, pack("L*", 0x55555555, 0xAAAAAAAA))
-	  or BAIL_OUT("syswrite failed: $!");
+	syswrite(
+		$fh,
+		pack("L*",
+			0xAAA15550, 0xAAA0D550, 0x00010000,
+			0x00008000, 0x0000800F, 0x001e8000)
+	) or BAIL_OUT("syswrite failed: $!");
 	close($fh)
 	  or BAIL_OUT("close failed: $!");
 
@@ -126,17 +130,23 @@ sub detects_heap_corruption
 {
 	my ($function, $testname) = @_;
 
-	detects_corruption($function, $testname,
-		qr/line pointer redirection to item at offset \d+ exceeds maximum offset \d+/
+	detects_corruption(
+		$function,
+		$testname,
+		qr/line pointer redirection to item at offset \d+ precedes minimum offset \d+/,
+		qr/line pointer redirection to item at offset \d+ exceeds maximum offset \d+/,
+		qr/line pointer to page offset \d+ is not maximally aligned/,
+		qr/line pointer length \d+ is less than the minimum tuple header size \d+/,
+		qr/line pointer to page offset \d+ with length \d+ ends beyond maximum page offset \d+/,
 	);
 }
 
 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
@@ -105,6 +105,7 @@ typedef struct HeapCheckContext
 	OffsetNumber offnum;
 	ItemId		itemid;
 	uint16		lp_len;
+	uint16		lp_off;
 	HeapTupleHeader tuphdr;
 	int			natts;
 
@@ -247,6 +248,13 @@ verify_heapam(PG_FUNCTION_ARGS)
 	memset(&ctx, 0, sizeof(HeapCheckContext));
 	ctx.cached_xid = InvalidTransactionId;
 
+	/*
+	 * If we report corruption when not examining some individual attribute,
+	 * we need attnum to be reported as NULL.  Set that up before any
+	 * corruption reporting might happen.
+	 */
+	ctx.attnum = -1;
+
 	/* The tupdesc and tuplestore must be created in ecxt_per_query_memory */
 	old_context = MemoryContextSwitchTo(rsinfo->econtext->ecxt_per_query_memory);
 	random_access = (rsinfo->allowedModes & SFRM_Materialize_Random) != 0;
@@ -378,14 +386,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 +417,36 @@ verify_heapam(PG_FUNCTION_ARGS)
 				continue;
 			}
 
-			/* Set up context information about this next tuple */
+			/* Sanity-check the line pointer's offset and length values */
 			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",
+										   ctx.lp_off));
+				continue;
+			}
+			if (ctx.lp_len < MAXALIGN(SizeofHeapTupleHeader))
+			{
+				report_corruption(&ctx,
+								  psprintf("line pointer length %u is less than the minimum tuple header size %u",
+										   ctx.lp_len,
+										   (unsigned) MAXALIGN(SizeofHeapTupleHeader)));
+				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",
+										   ctx.lp_off,
+										   ctx.lp_len,
+										   (unsigned) BLCKSZ));
+				continue;
+			}
+
+			/* It should be safe to examine the tuple's header, at least */
 			ctx.tuphdr = (HeapTupleHeader) PageGetItem(ctx.page, ctx.itemid);
 			ctx.natts = HeapTupleHeaderGetNatts(ctx.tuphdr);
 
@@ -1088,25 +1132,6 @@ check_tuple(HeapCheckContext *ctx)
 	bool		fatal = false;
 	uint16		infomask = ctx->tuphdr->t_infomask;
 
-	/*
-	 * If we report corruption before iterating over individual attributes, we
-	 * need attnum to be reported as NULL.  Set that up before any corruption
-	 * reporting might happen.
-	 */
-	ctx->attnum = -1;
-
-	/*
-	 * If the line pointer for this tuple does not reserve enough space for a
-	 * complete tuple header, we dare not read the tuple header.
-	 */
-	if (ctx->lp_len < MAXALIGN(SizeofHeapTupleHeader))
-	{
-		report_corruption(ctx,
-						  psprintf("line pointer length %u is less than the minimum tuple header size %u",
-								   ctx->lp_len, (uint32) MAXALIGN(SizeofHeapTupleHeader)));
-		return;
-	}
-
 	/* If xmin is normal, it should be within valid range */
 	xmin = HeapTupleHeaderGetXmin(ctx->tuphdr);
 	switch (get_xid_status(xmin, ctx, NULL))
@@ -1256,6 +1281,9 @@ check_tuple(HeapCheckContext *ctx)
 	for (ctx->attnum = 0; ctx->attnum < ctx->natts; ctx->attnum++)
 		if (!check_tuple_attribute(ctx))
 			break;				/* cannot continue */
+
+	/* revert attnum to -1 until we again examine individual attributes */
+	ctx->attnum = -1;
 }
 
 /*
@@ -1393,9 +1421,9 @@ get_xid_status(TransactionId xid, HeapCheckContext *ctx,
 	if (!fxid_in_cached_range(fxid, ctx))
 	{
 		/*
-		 * We may have been checking against stale values.  Update the
-		 * cached range to be sure, and since we relied on the cached
-		 * range when we performed the full xid conversion, reconvert.
+		 * We may have been checking against stale values.  Update the cached
+		 * range to be sure, and since we relied on the cached range when we
+		 * performed the full xid conversion, reconvert.
 		 */
 		update_cached_xid_range(ctx);
 		fxid = FullTransactionIdFromXidAndCtx(xid, ctx);
