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

David Rowley <dgrowleyml@gmail.com> writes:
> For the switching MemoryContextMethodID patch, I applied the memory
> context benchmarking patch I used when writing that code to test out
> the overhead in a tight palloc/pfree loop (attached). I can see an
> overhead of a little over 6.5%.

Hm.  I got an overhead of about 2% on an Apple M4, which might be
argued to be acceptable, but 12% on an aging x86_64 platform.
Realistically, given that we failed to notice this omission at
all for more than three years, it's hard to argue that testing
for it in non-debug builds is worth any overhead.

Here's a fleshed-out version of the requested_size method.
I noted that AllocSetRealloc needs a defense too, and then
extended the patch to generation.c and slab.c.  bump.c
doesn't have an issue, and I don't think alignedalloc.c
needs its own defense either: it can rely on the underlying
context type.

            regards, tom lane

diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 161c2e2d3df..bea9e47ee4f 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))
@@ -1373,6 +1377,10 @@ AllocSetRealloc(void *pointer, Size size, int flags)
     oldchksize = GetChunkSizeFromFreeListIdx(fidx);

 #ifdef MEMORY_CONTEXT_CHECKING
+    /* Test for previously-freed chunk */
+    if (unlikely(chunk->requested_size == InvalidAllocSize))
+        elog(WARNING, "detected realloc of freed chunk in %s %p",
+             set->header.name, chunk);
     /* Test for someone scribbling on unused space in chunk */
     if (chunk->requested_size < oldchksize)
         if (!sentinel_ok(pointer, chunk->requested_size))
diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c
index 9077ed299b4..fe9d087c85e 100644
--- a/src/backend/utils/mmgr/generation.c
+++ b/src/backend/utils/mmgr/generation.c
@@ -762,6 +762,10 @@ GenerationFree(void *pointer)
     }

 #ifdef MEMORY_CONTEXT_CHECKING
+    /* Test for previously-freed chunk */
+    if (unlikely(chunk->requested_size == InvalidAllocSize))
+        elog(WARNING, "detected double pfree in %s %p",
+             ((MemoryContext) block->context)->name, chunk);
     /* Test for someone scribbling on unused space in chunk */
     Assert(chunk->requested_size < chunksize);
     if (!sentinel_ok(pointer, chunk->requested_size))
@@ -867,6 +871,10 @@ GenerationRealloc(void *pointer, Size size, int flags)
     set = block->context;

 #ifdef MEMORY_CONTEXT_CHECKING
+    /* Test for previously-freed chunk */
+    if (unlikely(chunk->requested_size == InvalidAllocSize))
+        elog(WARNING, "detected realloc of freed chunk in %s %p",
+             ((MemoryContext) set)->name, chunk);
     /* Test for someone scribbling on unused space in chunk */
     Assert(chunk->requested_size < oldsize);
     if (!sentinel_ok(pointer, chunk->requested_size))
diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c
index bd00bab18fe..0d1e6285c29 100644
--- a/src/backend/utils/mmgr/slab.c
+++ b/src/backend/utils/mmgr/slab.c
@@ -539,6 +539,7 @@ SlabAllocSetupNewChunk(MemoryContext context, SlabBlock *block,
     MemoryChunkSetHdrMask(chunk, block, MAXALIGN(slab->chunkSize), MCTX_SLAB_ID);

 #ifdef MEMORY_CONTEXT_CHECKING
+    chunk->requested_size = size;
     /* slab mark to catch clobber of "unused" space */
     Assert(slab->chunkSize < (slab->fullChunkSize - Slab_CHUNKHDRSZ));
     set_sentinel(MemoryChunkGetPointer(chunk), size);
@@ -748,11 +749,17 @@ SlabFree(void *pointer)
     slab = block->slab;

 #ifdef MEMORY_CONTEXT_CHECKING
+    /* Test for previously-freed chunk */
+    if (unlikely(chunk->requested_size == InvalidAllocSize))
+        elog(WARNING, "detected double pfree in %s %p",
+             slab->header.name, chunk);
     /* Test for someone scribbling on unused space in chunk */
     Assert(slab->chunkSize < (slab->fullChunkSize - Slab_CHUNKHDRSZ));
     if (!sentinel_ok(pointer, slab->chunkSize))
         elog(WARNING, "detected write past chunk end in %s %p",
              slab->header.name, chunk);
+    /* Reset requested_size to InvalidAllocSize in free chunks */
+    chunk->requested_size = InvalidAllocSize;
 #endif

     /* push this chunk onto the head of the block's free list */