public inbox for [email protected]
help / color / mirror / Atom feedRe: BUG #19438: segfault with temp_file_limit inside cursor
3+ messages / 2 participants
[nested] [flat]
* Re: BUG #19438: segfault with temp_file_limit inside cursor
@ 2026-03-29 15:32 Tom Lane <[email protected]>
2026-03-29 16:25 ` Re: BUG #19438: segfault with temp_file_limit inside cursor Tom Lane <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: Tom Lane @ 2026-03-29 15:32 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: [email protected]; [email protected]
David Rowley <[email protected]> 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
Attachments:
[text/x-diff] v2-detect-double-free-with-requested_size.patch (3.6K, 2-v2-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..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 */
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: BUG #19438: segfault with temp_file_limit inside cursor
2026-03-29 15:32 Re: BUG #19438: segfault with temp_file_limit inside cursor Tom Lane <[email protected]>
@ 2026-03-29 16:25 ` Tom Lane <[email protected]>
2026-03-29 23:33 ` Re: BUG #19438: segfault with temp_file_limit inside cursor David Rowley <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: Tom Lane @ 2026-03-29 16:25 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: [email protected]; [email protected]
I wrote:
> ... I don't think alignedalloc.c
> needs its own defense either: it can rely on the underlying
> context type.
I started to wonder if an explicit test in AlignedAllocFree
could be useful anyway to make such problems a bit less obscure.
However, when I tried
p = palloc_aligned(...);
pfree(p);
pfree(p);
I got
ERROR: pfree called with invalid pointer 0x1f286b0 (header 0x7f7f7f7f7f7f7f7f)
That is, we'll never get to AlignedAllocFree because the underlying
context would have wipe_mem'd the aligned chunk's header during the
first pfree. The only case in which such a test could be helpful is
in a build with MEMORY_CONTEXT_CHECKING but not CLOBBER_FREED_MEMORY.
While I suppose some people might build that way, it's got to be such
a tiny minority as to not be worth worrying about.
regards, tom lane
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: BUG #19438: segfault with temp_file_limit inside cursor
2026-03-29 15:32 Re: BUG #19438: segfault with temp_file_limit inside cursor Tom Lane <[email protected]>
2026-03-29 16:25 ` Re: BUG #19438: segfault with temp_file_limit inside cursor Tom Lane <[email protected]>
@ 2026-03-29 23:33 ` David Rowley <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: David Rowley @ 2026-03-29 23:33 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]; [email protected]
On Mon, 30 Mar 2026 at 05:25, Tom Lane <[email protected]> wrote:
> I started to wonder if an explicit test in AlignedAllocFree
> could be useful anyway to make such problems a bit less obscure.
> However, when I tried
>
> p = palloc_aligned(...);
> pfree(p);
> pfree(p);
>
> I got
>
> ERROR: pfree called with invalid pointer 0x1f286b0 (header 0x7f7f7f7f7f7f7f7f)
>
> That is, we'll never get to AlignedAllocFree because the underlying
> context would have wipe_mem'd the aligned chunk's header during the
> first pfree. The only case in which such a test could be helpful is
> in a build with MEMORY_CONTEXT_CHECKING but not CLOBBER_FREED_MEMORY.
> While I suppose some people might build that way, it's got to be such
> a tiny minority as to not be worth worrying about.
I think you might have trouble trying to get the MemoryContext.name
for the elog warning anyway. That's only accessible from the unaligned
allocation and whatever method that context type uses to backlink the
owning context from the chunk pointer. Given that, it very much seems
not worthwhile as I imagine that means adding some callback function
to MemoryContextMethods!
David
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2026-03-29 23:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-03-29 15:32 Re: BUG #19438: segfault with temp_file_limit inside cursor Tom Lane <[email protected]>
2026-03-29 16:25 ` Tom Lane <[email protected]>
2026-03-29 23:33 ` David Rowley <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox