Re: BUG #19438: segfault with temp_file_limit inside cursor - Mailing list pgsql-bugs

I wrote:
> Somehow, we are not crashing on a
> double free with the new memory chunk header infrastructure.

In fact, we are not.  AllocSetFree does not change the "hdrmask" field
of a freed chunk.  So if we try to free it again, we end up right back
at AllocSetFree, and the outcome is there's no detected problem but
the corresponding freelist is now corrupt because the chunk got linked
into it twice.  In this example that doesn't cause any visible
misbehavior, because we'll free the holdStore's context before doing
very much more with it (and AllocSetCheck won't notice this type of
corruption).  Other cases could lead to very hard-to-diagnose problems
that manifest somewhere far removed from the actual bug.

In MEMORY_CONTEXT_CHECKING builds, we can cheaply detect double frees
by using the existing behavior that requested_size is set to
InvalidAllocSize during AllocSetFree.  Another plausible idea is to
change a freed chunk's MemoryContextMethodID to something invalid,
which'd permit detection of double frees even in
non-MEMORY_CONTEXT_CHECKING builds.

I made draft patches showing how to do it both ways.  (Both patches
pass check-world and are able to detect the bug in v17.)  The
methodid-change way seems like the better alternative to me,
but it is more invasive and does add a cycle or two when freeing or
reusing a chunk.

The other mcxt modules need to be looked at too, but I thought
I'd try to get agreement on the solution approach before going
further.

            regards, tom lane

diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 161c2e2d3df..ebf237bf575 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -1175,6 +1175,10 @@ AllocSetFree(void *pointer)
         link = GetFreeListLink(chunk);
 
 #ifdef MEMORY_CONTEXT_CHECKING
+        /* Test for previously-freed chunk */
+        if (unlikely(chunk->requested_size == InvalidAllocSize))
+            elog(WARNING, "detected double pfree in %s %p",
+                 set->header.name, chunk);
         /* Test for someone scribbling on unused space in chunk */
         if (chunk->requested_size < GetChunkSizeFromFreeListIdx(fidx))
             if (!sentinel_ok(pointer, chunk->requested_size))
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 161c2e2d3df..82ce25133ff 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -765,7 +765,7 @@ AllocSetAllocLarge(MemoryContext context, Size size, int flags)

     chunk = (MemoryChunk *) (((char *) block) + ALLOC_BLOCKHDRSZ);

-    /* mark the MemoryChunk as externally managed */
+    /* mark the MemoryChunk as externally managed and valid */
     MemoryChunkSetHdrMaskExternal(chunk, MCTX_ASET_ID);

 #ifdef MEMORY_CONTEXT_CHECKING
@@ -826,7 +826,7 @@ AllocSetAllocChunkFromBlock(MemoryContext context, AllocBlock block,
     block->freeptr += (chunk_size + ALLOC_CHUNKHDRSZ);
     Assert(block->freeptr <= block->endptr);

-    /* store the free list index in the value field */
+    /* store the free list index in the value field, and mark chunk valid */
     MemoryChunkSetHdrMask(chunk, block, fidx, MCTX_ASET_ID);

 #ifdef MEMORY_CONTEXT_CHECKING
@@ -910,8 +910,8 @@ AllocSetAllocFromNewBlock(MemoryContext context, Size size, int flags,
         block->freeptr += (availchunk + ALLOC_CHUNKHDRSZ);
         availspace -= (availchunk + ALLOC_CHUNKHDRSZ);

-        /* store the freelist index in the value field */
-        MemoryChunkSetHdrMask(chunk, block, a_fidx, MCTX_ASET_ID);
+        /* store the freelist index in the value field, but mark chunk free */
+        MemoryChunkSetHdrMask(chunk, block, a_fidx, MCTX_UNUSED_CHUNK_ID);
 #ifdef MEMORY_CONTEXT_CHECKING
         chunk->requested_size = InvalidAllocSize;    /* mark it free */
 #endif
@@ -1058,6 +1058,9 @@ AllocSetAlloc(MemoryContext context, Size size, int flags)
         set->freelist[fidx] = link->next;
         VALGRIND_MAKE_MEM_NOACCESS(link, sizeof(AllocFreeListLink));

+        /* mark chunk valid */
+        MemoryChunkSetMethodID(chunk, MCTX_ASET_ID);
+
 #ifdef MEMORY_CONTEXT_CHECKING
         chunk->requested_size = size;
         /* set mark to catch clobber of "unused" space */
@@ -1185,6 +1188,10 @@ AllocSetFree(void *pointer)
 #ifdef CLOBBER_FREED_MEMORY
         wipe_mem(pointer, GetChunkSizeFromFreeListIdx(fidx));
 #endif
+
+        /* mark chunk free */
+        MemoryChunkSetMethodID(chunk, MCTX_UNUSED_CHUNK_ID);
+
         /* push this chunk onto the top of the free list */
         VALGRIND_MAKE_MEM_DEFINED(link, sizeof(AllocFreeListLink));
         link->next = set->freelist[fidx];
diff --git a/src/include/utils/memutils_internal.h b/src/include/utils/memutils_internal.h
index 475e91b336b..9fa877332ee 100644
--- a/src/include/utils/memutils_internal.h
+++ b/src/include/utils/memutils_internal.h
@@ -138,6 +138,9 @@ typedef enum MemoryContextMethodID
     MCTX_15_RESERVED_WIPEDMEM_ID    /* 1111 occurs in wipe_mem'd memory */
 } MemoryContextMethodID;

+/* This MemoryContextMethodID is recommended to put in pfree'd chunks */
+#define MCTX_UNUSED_CHUNK_ID    MCTX_0_RESERVED_UNUSEDMEM_ID
+
 /*
  * The number of bits that 8-byte memory chunk headers can use to encode the
  * MemoryContextMethodID.
diff --git a/src/include/utils/memutils_memorychunk.h b/src/include/utils/memutils_memorychunk.h
index bda9912182d..b876940d5e0 100644
--- a/src/include/utils/memutils_memorychunk.h
+++ b/src/include/utils/memutils_memorychunk.h
@@ -58,6 +58,9 @@
  * MemoryChunkSetHdrMaskExternal:
  *        Used to set up an externally managed MemoryChunk.
  *
+ * MemoryChunkSetMethodID:
+ *        Used to change a MemoryChunk's MethodID while preserving other fields.
+ *
  * MemoryChunkIsExternal:
  *        Determine if the given MemoryChunk is externally managed, i.e.
  *        MemoryChunkSetHdrMaskExternal() was called on the chunk.
@@ -196,6 +199,21 @@ MemoryChunkSetHdrMaskExternal(MemoryChunk *chunk,
         methodid;
 }

+/*
+ * MemoryChunkSetMethodID:
+ *        Set the methodid without changing any other chunk header fields.
+ *        This is typically used while marking a chunk as free or not-free.
+ */
+static inline void
+MemoryChunkSetMethodID(MemoryChunk *chunk,
+                       MemoryContextMethodID methodid)
+{
+    Assert((int) methodid <= MEMORY_CONTEXT_METHODID_MASK);
+
+    chunk->hdrmask = (chunk->hdrmask & ~MEMORY_CONTEXT_METHODID_MASK) |
+        methodid;
+}
+
 /*
  * MemoryChunkIsExternal
  *        Return true if 'chunk' is marked as external.