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);