From 095555daf1246ac28595fcf5b26bbb4686b0cdb6 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 30 Mar 2026 12:02:08 -0400 Subject: [PATCH] Detect pfree or repalloc of a previously-freed memory chunk. Before the major rewrite in commit c6e0fe1f2, AllocSetFree() would typically crash when asked to free an already-free chunk. That was an ugly but serviceable way of detecting coding errors that led to double pfrees. But since that rewrite, double pfrees went through just fine, because the "hdrmask" of a freed chunk isn't changed at all when putting it on the freelist. We'd end with a corrupt freelist that circularly links back to the doubly-freed chunk, which would usually result in trouble later, far removed from the actual bug. This situation is no good at all for debugging purposes. Fortunately, we can fix it at low cost in MEMORY_CONTEXT_CHECKING builds by making AllocSetFree() check for chunk->requested_size == InvalidAllocSize, relying on the pre-existing code that sets it that way just below. I investigated the alternative of changing a freed chunk's methodid field, which would allow detection in non-MEMORY_CONTEXT_CHECKING builds too. But that adds measurable overhead. Seeing that we didn't notice this oversight for more than three years, it's hard to argue that detecting this type of bug is worth any extra overhead in production builds. Likewise fix AllocSetRealloc() to detect repalloc() on a freed chunk, and apply similar changes in generation.c and slab.c. (generation.c would hit an Assert failure anyway, but it seems best to make it act like aset.c.) bump.c doesn't need changes since it doesn't support pfree in the first place. Ideally alignedalloc.c would receive similar changes, but in debugging builds it's impossible to reach AlignedAllocFree() or AlignedAllocRealloc() on a pfreed chunk, because the underlying context's pfree would have wiped the chunk header of the aligned chunk. But that means we should get an error of some sort, so let's be content with that. Per investigation of why the test case for bug #19438 didn't appear to fail in v16 and up, even though the underlying bug was still present. (This doesn't fix the underlying double-free bug, just cause it to get detected.) Bug: #19438 Reported-by: Dmitriy Kuzmin Author: Tom Lane Reviewed-by: David Rowley Discussion: https://postgr.es/m/19438-9d37b179c56d43aa@postgresql.org Backpatch-through: 16 --- src/backend/utils/mmgr/aset.c | 24 ++++++++++++++++++++++++ src/backend/utils/mmgr/generation.c | 10 ++++++++++ src/backend/utils/mmgr/slab.c | 8 ++++++++ 3 files changed, 42 insertions(+) diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c index 161c2e2d3df..6a9ea367107 100644 --- a/src/backend/utils/mmgr/aset.c +++ b/src/backend/utils/mmgr/aset.c @@ -1174,7 +1174,26 @@ AllocSetFree(void *pointer) Assert(FreeListIdxIsValid(fidx)); link = GetFreeListLink(chunk); + /* + * It might seem odd that we use elevel ERROR for double-pfree but + * only WARNING for write-past-chunk-end. But the two conditions are + * not very comparable. In the double-pfree case we can prevent + * corruption before it happens; while if we let it go through, the + * result would be a corrupted freelist that allows this chunk to get + * re-allocated twice. Thus the original bug could cascade into + * hard-to-understand misbehavior that might manifest far away from + * the actual source of the problem. On the other hand, a write past + * chunk end can be relatively benign if just a few bytes too many + * were written: often, only padding or unused space gets affected. + * Moreover, whatever damage was done is already done, and we're just + * reporting after the fact with no ability to clean it up. So just + * warn, like AllocSetCheck would do if the chunk didn't get freed. + */ #ifdef MEMORY_CONTEXT_CHECKING + /* Test for previously-freed chunk */ + if (unlikely(chunk->requested_size == InvalidAllocSize)) + elog(ERROR, "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 +1392,11 @@ AllocSetRealloc(void *pointer, Size size, int flags) oldchksize = GetChunkSizeFromFreeListIdx(fidx); #ifdef MEMORY_CONTEXT_CHECKING + /* See comments in AllocSetFree about uses of ERROR and WARNING here */ + /* Test for previously-freed chunk */ + if (unlikely(chunk->requested_size == InvalidAllocSize)) + elog(ERROR, "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..609c9bdc9a6 100644 --- a/src/backend/utils/mmgr/generation.c +++ b/src/backend/utils/mmgr/generation.c @@ -762,6 +762,11 @@ GenerationFree(void *pointer) } #ifdef MEMORY_CONTEXT_CHECKING + /* See comments in AllocSetFree about uses of ERROR and WARNING here */ + /* Test for previously-freed chunk */ + if (unlikely(chunk->requested_size == InvalidAllocSize)) + elog(ERROR, "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 +872,11 @@ GenerationRealloc(void *pointer, Size size, int flags) set = block->context; #ifdef MEMORY_CONTEXT_CHECKING + /* See comments in AllocSetFree about uses of ERROR and WARNING here */ + /* Test for previously-freed chunk */ + if (unlikely(chunk->requested_size == InvalidAllocSize)) + elog(ERROR, "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..36d0ad6d320 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,18 @@ SlabFree(void *pointer) slab = block->slab; #ifdef MEMORY_CONTEXT_CHECKING + /* See comments in AllocSetFree about uses of ERROR and WARNING here */ + /* Test for previously-freed chunk */ + if (unlikely(chunk->requested_size == InvalidAllocSize)) + elog(ERROR, "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 */