Fix more portability issues in new amcheck code.

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
index f8ee5384f328c11495e63bb0471c22266c630ac9..1581e51f3ca7ff9e4e2446662c48230cb4707679 100644 (file)
--- 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,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
index 37b40a0404d5534e04ade54c0da6884a11c01042..8bb890438aa9577cc3b73c3de550da75dd3931b7 100644 (file)
--- 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);