public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tom Lane <[email protected]>
To: David Rowley <[email protected]>
Cc: [email protected]
Cc: [email protected]
Subject: Re: BUG #19438: segfault with temp_file_limit inside cursor
Date: Fri, 27 Mar 2026 13:41:29 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
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
Attachments:
[text/x-diff] detect-double-free-with-requested_size.patch (671B, 2-detect-double-free-with-requested_size.patch)
download | inline diff:
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))
[text/x-diff] detect-double-free-with-methodid.patch (4.0K, 3-detect-double-free-with-methodid.patch)
download | inline diff:
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.
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: BUG #19438: segfault with temp_file_limit inside cursor
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox