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 <kuzmin.db4@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/19438-9d37b179c56d43aa@postgresql.org
Backpatch-through: 16

diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 161c2e2d3dfb0619b487a25b71d8134645337c03..6a9ea3671074a14abaf80fc7e40c17e4da721d3c 100644 (file)
--- 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 9077ed299b451c885cdd08f4230b4cd6959fc759..609c9bdc9a6b5c84f1b7829e881d8c19e3ed2cb3 100644 (file)
--- 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 bd00bab18fef22b87c0c237863bc64727fa2ddc1..36d0ad6d320ba1a6cf48327ee24740f553074ed9 100644 (file)
--- 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 */