Re: Reducing the chunk header sizes on all memory context types - Mailing list pgsql-hackers

So I pushed that, but I don't feel that we're out of the woods yet.
As I mentioned at [1], while testing this stuff I hit a case where
aset.c will try to wipe_mem practically the entire address space after
being asked to pfree() an invalid pointer.  The specific reproducer
isn't too interesting because it depends on the pre-80ef92675 state of
the code, but what I take away from this is that aset.c is still far
too fragile as it stands.

One problem is that aset.c generally isn't too careful about whether
a putative chunk is actually one of its chunks.  That was okay before
c6e0fe1f2 because we would never get to AllocSetFree etc unless the
word before the chunk data pointed at a moderately-sane AllocSet.
Now, we can arrive at that code on the strength of three random bits,
so it's got to be more careful.  In the attached patch, I make
AllocSetIsValid do an IsA() test, and make sure to apply it to the
aset we think we have found from the chunk header.  This is not in
any way a new check: what it is doing is replacing the IsA check done
by the "AssertArg(MemoryContextIsValid(context))" that was performed
by GetMemoryChunkContext in the old code, and that we've completely
lost in the code as it stands.

The other problem, which is what is leading to wipe_mem-the-universe,
is that aset.c figures the size of a non-external chunk essentially
as "1 << MemoryChunkGetValue(chunk)", where the "value" field is 30
bits wide and has undergone exactly zero validation before
AllocSetFree uses the size in memset.  That's far, far too trusting.
In the attached I put in some asserts to verify that the value field
is in the valid range for a freelist index, which should usually
trigger if we have a garbage value, or at the very least constrain
the damage.

What I am mainly wondering about at this point is whether Asserts
are good enough or we should use actual test-and-elog checks for
these things.  The precedent of the old GetMemoryChunkContext
implementation suggests that assertions are good enough for the
AllocSetIsValid tests.  On the other hand, there are existing
cross-checks like

        if (block->freeptr != block->endptr)
            elog(ERROR, "could not find block containing chunk %p", chunk);

so at least in some code paths we've thought it is worth expending
such tests in production builds.  Any opinions?

I imagine generation.c and slab.c need similar bulletproofing
measures, but I didn't look at them yet.

            regards, tom lane

[1] https://www.postgresql.org/message-id/3436789.1665187055%40sss.pgh.pa.us

diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index ec423375ae..76a9f02bd8 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -132,6 +132,10 @@ typedef struct AllocFreeListLink
 #define GetFreeListLink(chkptr) \
     (AllocFreeListLink *) ((char *) (chkptr) + ALLOC_CHUNKHDRSZ)

+/* Validate a freelist index retrieved from a chunk header */
+#define FreeListIdxIsValid(fidx) \
+    ((fidx) >= 0 && (fidx) < ALLOCSET_NUM_FREELISTS)
+
 /* Determine the size of the chunk based on the freelist index */
 #define GetChunkSizeFromFreeListIdx(fidx) \
     ((((Size) 1) << ALLOC_MINBITS) << (fidx))
@@ -202,7 +206,15 @@ typedef struct AllocBlockData
  * AllocSetIsValid
  *        True iff set is valid allocation set.
  */
-#define AllocSetIsValid(set) PointerIsValid(set)
+#define AllocSetIsValid(set) \
+    (PointerIsValid(set) && IsA(set, AllocSetContext))
+
+/*
+ * AllocBlockIsValid
+ *        True iff block is valid block of allocation set.
+ */
+#define AllocBlockIsValid(block) \
+    (PointerIsValid(block) && AllocSetIsValid((block)->aset))

 /*
  * We always store external chunks on a dedicated block.  This makes fetching
@@ -530,8 +542,7 @@ AllocSetReset(MemoryContext context)
 {
     AllocSet    set = (AllocSet) context;
     AllocBlock    block;
-    Size        keepersize PG_USED_FOR_ASSERTS_ONLY
-    = set->keeper->endptr - ((char *) set);
+    Size        keepersize PG_USED_FOR_ASSERTS_ONLY;

     AssertArg(AllocSetIsValid(set));

@@ -540,6 +551,9 @@ AllocSetReset(MemoryContext context)
     AllocSetCheck(context);
 #endif

+    /* Remember keeper block size for Assert below */
+    keepersize = set->keeper->endptr - ((char *) set);
+
     /* Clear chunk freelists */
     MemSetAligned(set->freelist, 0, sizeof(set->freelist));

@@ -598,8 +612,7 @@ AllocSetDelete(MemoryContext context)
 {
     AllocSet    set = (AllocSet) context;
     AllocBlock    block = set->blocks;
-    Size        keepersize PG_USED_FOR_ASSERTS_ONLY
-    = set->keeper->endptr - ((char *) set);
+    Size        keepersize PG_USED_FOR_ASSERTS_ONLY;

     AssertArg(AllocSetIsValid(set));

@@ -608,6 +621,9 @@ AllocSetDelete(MemoryContext context)
     AllocSetCheck(context);
 #endif

+    /* Remember keeper block size for Assert below */
+    keepersize = set->keeper->endptr - ((char *) set);
+
     /*
      * If the context is a candidate for a freelist, put it into that freelist
      * instead of destroying it.
@@ -994,9 +1010,10 @@ AllocSetFree(void *pointer)

     if (MemoryChunkIsExternal(chunk))
     {
-
+        /* Release single-chunk block. */
         AllocBlock    block = ExternalChunkGetBlock(chunk);

+        AssertArg(AllocBlockIsValid(block));
         set = block->aset;

 #ifdef MEMORY_CONTEXT_CHECKING
@@ -1011,7 +1028,6 @@ AllocSetFree(void *pointer)
         }
 #endif

-
         /*
          * Try to verify that we have a sane block pointer, the freeptr should
          * match the endptr.
@@ -1036,12 +1052,17 @@ AllocSetFree(void *pointer)
     }
     else
     {
-        int            fidx = MemoryChunkGetValue(chunk);
         AllocBlock    block = MemoryChunkGetBlock(chunk);
-        AllocFreeListLink *link = GetFreeListLink(chunk);
+        int            fidx;
+        AllocFreeListLink *link;

+        AssertArg(AllocBlockIsValid(block));
         set = block->aset;

+        fidx = MemoryChunkGetValue(chunk);
+        Assert(FreeListIdxIsValid(fidx));
+        link = GetFreeListLink(chunk);
+
 #ifdef MEMORY_CONTEXT_CHECKING
         /* Test for someone scribbling on unused space in chunk */
         if (chunk->requested_size < GetChunkSizeFromFreeListIdx(fidx))
@@ -1089,6 +1110,7 @@ AllocSetRealloc(void *pointer, Size size)
     AllocSet    set;
     MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
     Size        oldsize;
+    int            fidx;

     /* Allow access to private part of chunk header. */
     VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOCCHUNK_PRIVATE_LEN);
@@ -1105,9 +1127,11 @@ AllocSetRealloc(void *pointer, Size size)
         Size        oldblksize;

         block = ExternalChunkGetBlock(chunk);
-        oldsize = block->endptr - (char *) pointer;
+        AssertArg(AllocBlockIsValid(block));
         set = block->aset;

+        oldsize = block->endptr - (char *) pointer;
+
 #ifdef MEMORY_CONTEXT_CHECKING
         /* Test for someone scribbling on unused space in chunk */
         Assert(chunk->requested_size < oldsize);
@@ -1201,9 +1225,13 @@ AllocSetRealloc(void *pointer, Size size)
     }

     block = MemoryChunkGetBlock(chunk);
-    oldsize = GetChunkSizeFromFreeListIdx(MemoryChunkGetValue(chunk));
+    AssertArg(AllocBlockIsValid(block));
     set = block->aset;

+    fidx = MemoryChunkGetValue(chunk);
+    Assert(FreeListIdxIsValid(fidx));
+    oldsize = GetChunkSizeFromFreeListIdx(fidx);
+
 #ifdef MEMORY_CONTEXT_CHECKING
     /* Test for someone scribbling on unused space in chunk */
     if (chunk->requested_size < oldsize)
@@ -1328,6 +1356,7 @@ AllocSetGetChunkContext(void *pointer)
     else
         block = (AllocBlock) MemoryChunkGetBlock(chunk);

+    AssertArg(AllocBlockIsValid(block));
     set = block->aset;

     return &set->header;
@@ -1342,16 +1371,19 @@ Size
 AllocSetGetChunkSpace(void *pointer)
 {
     MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
+    int            fidx;

     if (MemoryChunkIsExternal(chunk))
     {
         AllocBlock    block = ExternalChunkGetBlock(chunk);

+        AssertArg(AllocBlockIsValid(block));
         return block->endptr - (char *) chunk;
     }

-    return GetChunkSizeFromFreeListIdx(MemoryChunkGetValue(chunk)) +
-        ALLOC_CHUNKHDRSZ;
+    fidx = MemoryChunkGetValue(chunk);
+    Assert(FreeListIdxIsValid(fidx));
+    return GetChunkSizeFromFreeListIdx(fidx) + ALLOC_CHUNKHDRSZ;
 }

 /*
@@ -1361,6 +1393,8 @@ AllocSetGetChunkSpace(void *pointer)
 bool
 AllocSetIsEmpty(MemoryContext context)
 {
+    AssertArg(AllocSetIsValid(context));
+
     /*
      * For now, we say "empty" only if the context is new or just reset. We
      * could examine the freelists to determine if all space has been freed,
@@ -1394,6 +1428,8 @@ AllocSetStats(MemoryContext context,
     AllocBlock    block;
     int            fidx;

+    AssertArg(AllocSetIsValid(set));
+
     /* Include context header in totalspace */
     totalspace = MAXALIGN(sizeof(AllocSetContext));

@@ -1405,14 +1441,14 @@ AllocSetStats(MemoryContext context,
     }
     for (fidx = 0; fidx < ALLOCSET_NUM_FREELISTS; fidx++)
     {
+        Size        chksz = GetChunkSizeFromFreeListIdx(fidx);
         MemoryChunk *chunk = set->freelist[fidx];

         while (chunk != NULL)
         {
-            Size        chksz = GetChunkSizeFromFreeListIdx(MemoryChunkGetValue(chunk));
             AllocFreeListLink *link = GetFreeListLink(chunk);

-            Assert(GetChunkSizeFromFreeListIdx(fidx) == chksz);
+            Assert(MemoryChunkGetValue(chunk) == fidx);

             freechunks++;
             freespace += chksz + ALLOC_CHUNKHDRSZ;
@@ -1522,7 +1558,13 @@ AllocSetCheck(MemoryContext context)
             }
             else
             {
-                chsize = GetChunkSizeFromFreeListIdx(MemoryChunkGetValue(chunk));    /* aligned chunk size */
+                int            fidx = MemoryChunkGetValue(chunk);
+
+                if (!FreeListIdxIsValid(fidx))
+                    elog(WARNING, "problem in alloc set %s: bad chunk size for chunk %p in block %p",
+                         name, chunk, block);
+
+                chsize = GetChunkSizeFromFreeListIdx(fidx); /* aligned chunk size */

                 /*
                  * Check the stored block offset correctly references this