Re: BUG #19438: segfault with temp_file_limit inside cursor

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