public inbox for [email protected]
help / color / mirror / Atom feedRe: BUG #19438: segfault with temp_file_limit inside cursor
2+ messages / 2 participants
[nested] [flat]
* Re: BUG #19438: segfault with temp_file_limit inside cursor
@ 2026-03-29 23:41 David Rowley <[email protected]>
0 siblings, 1 reply; 2+ messages in thread
From: David Rowley @ 2026-03-29 23:41 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]; [email protected]
On Mon, 30 Mar 2026 at 04:32, Tom Lane <[email protected]> wrote:
> 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.
Thanks for writing that up.
I looked at the code and tested. The only thing that I noted was
GenerationFree(), where we do:
/* 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);
I expect you've likely thought of this, but if we do spit out the
warning there, then the Assert is definitely going to fail, as
InvalidAllocSize is defined as SIZE_MAX. I don't know if that means
it's worth deviating from the similar WARNINGs you've added and making
that one an ERROR. There's certainly no guarantee with the other
context that we'll not crash sometime very soon after issuing the
warning anyway, so maybe it's fine.
David
^ permalink raw reply [nested|flat] 2+ messages in thread
* Re: BUG #19438: segfault with temp_file_limit inside cursor
@ 2026-03-29 23:51 Tom Lane <[email protected]>
parent: David Rowley <[email protected]>
0 siblings, 0 replies; 2+ messages in thread
From: Tom Lane @ 2026-03-29 23:51 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: [email protected]; [email protected]
David Rowley <[email protected]> writes:
> I looked at the code and tested. The only thing that I noted was
> GenerationFree(), where we do:
> /* 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);
> I expect you've likely thought of this, but if we do spit out the
> warning there, then the Assert is definitely going to fail, as
> InvalidAllocSize is defined as SIZE_MAX.
Yeah, I saw that after sending the patch. Not only would that
Assert fail, but without it, code below would go nuts too.
> I don't know if that means
> it's worth deviating from the similar WARNINGs you've added and making
> that one an ERROR. There's certainly no guarantee with the other
> context that we'll not crash sometime very soon after issuing the
> warning anyway, so maybe it's fine.
Seems like a reasonable answer. What do you think of making the
double-free cases ERRORs across the board? If we don't error out,
there will likely be cascading problems in all the mcxt types not
just this one.
regards, tom lane
^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2026-03-29 23:51 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-03-29 23:41 Re: BUG #19438: segfault with temp_file_limit inside cursor David Rowley <[email protected]>
2026-03-29 23:51 ` Tom Lane <[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