Re: Per-thread leak in ECPG's memory.c

On Mon, 6 Jul 2026 22:34:59 +0300, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> On 29/06/2026 16:19, Thomas Munro wrote:
> > Hi,
> >
> > ECPG's auto_mem_destructor() doesn't seem quite right:
> >
> > 1. POSIX thread-specific keys are reset to NULL before their at-exit
> > destructors are called, so its call to ECPGfree_auto_mem() gets NULL
> > from get_auto_allocs(), so nothing much happens. It should use the
> > value passed to the destructor.
> >
> > 2. ECPGfree_auto_mem() doesn't seem to be the right thing to do
> > anyway, because the user is expected to free heap objects allocated by
> > the library, for example where
> > src/interfaces/ecpg/test/thread/alloc.pgc does this:
> >
> > char **r = NULL;
> > ...
> > for (i = 1; i <= REPEATS; ++i)
> > {
> > EXEC SQL SELECT relname INTO :r FROM pg_class WHERE relname =
> > 'pg_class';
> > free(r);
> > r = NULL;
> > }
> >
> > With a fix for only problem #1 in place, the thread-exit destructor
> > double-frees "r" from the final loop. I *think* what is wanted here
> > is ecpg_clear_auto_mem(), to free just the list structure and not the
> > values themselves. Draft patch like that attached.
> >
> > It still leaks on Windows, but that's a known issue and I have a fix
> > for that as part of a larger refactoring of thread-related stuff, more
> > on that shortly. This looked like a bug to report separately first.
>
> Good catch!

The v2 patch looks right to me. I applied it on top of current master,
built it clean, and ran the ecpg suite: all 66 tests pass. To confirm
the hazard is real I also tried the #1-only version (use the passed
value but keep freeing the payload), and thread/alloc aborts on a
double-free, as you'd expect; v2 is clean.

> I wonder if we should refrain from doing the "set_auto_allocs(NULL);"
> call from the destructor. I guess it's harmless, but it feels a bit
> wrong to me. Like in the attached.

Agreed. The slot is already NULL by the time the destructor runs,
so re-clearing it buys nothing, and storing into a TLS key from inside
its own destructor is a wart worth avoiding. Keeping
set_auto_allocs(NULL) in ecpg_clear_auto_mem(), where the thread is
still live, and leaving it out of the destructor seems like the right
split.

The one thing worth spelling out in the commit message is why freeing
the node but not the pointer is correct: the auto_mem node is library
bookkeeping, but ownership of the pointed-to value was handed to the
caller when the statement succeeded. That captures the bug, and it
is what separates this path from ECPGfree_auto_mem().

> > Hmm, I wonder why ecpg_raise() frees auto-allocated values for all
> > connections just because one connection raised an error.
>
> I didn't look at that
>
> - Heikki

Earlier in the thread Thom Brown wrote:
> So would it just be a matter of moving ecpg_clear_auto_mem() out of
> ecpg_do_prologue() and put it into ecpg_do_epilogue() ...

I think that is the better model, but it can't fully retire the
destructor. SELECT ... INTO transfers ownership and could be cleared
eagerly, but GET DESCRIPTOR ... INTO :array leaves caller-owned arrays
on the list between statements by design, so a thread that exits before
the bulk-free still needs node cleanup. I'd keep the back-patch limited
to the destructor fix and treat the epilogue move as a separate
master-only change.

On your off-list ordering question, whether this can go in as-is or the
ecpg_raise() issue has to come first: as-is, I think. The patch is
self-contained. It changes only the thread-exit destructor and the
node-versus-value split, and touches neither ecpg_raise() nor where
ecpg_clear_auto_mem() is called. It is a back-patchable fix for a
double-free in valid .pgc code, with no dependency on the ecpg_raise()
rework in either direction. It unblocks the rest of the series. The
ecpg_raise() over-free stays a separate master-only change, and as noted
above it would not let us remove the destructor anyway, so there is no
reason to gate the leak fix on it.

On the Windows replacement mentioned upthread: the reason the
destructor never runs there is that TlsAlloc has no thread-exit
callback. The drop-in that does is FlsAlloc, whose fiber-local storage
callback fires on thread and fiber exit and is handed the stored value
as its argument, which is exactly the contract this patch now relies on.

For a concrete reference, I maintain a small portable thread-exit
cleanup primitive built on this, __os_thread_atexit(fn, arg):

https://github.com/gburd/libxtc/blob/main/src/os/os_tls.c#L69-L129

It exists because a runtime has to free per-thread state on any thread
that touches it, including host/carrier threads it did not create, which
is the same spot ECPG is in with application threads. It is one
lazily-created key whose destructor walks a per-thread LIFO list and
frees each node after calling its fn. Two details line up with this
thread. The destructor uses the value the OS passes in rather than
re-reading the slot, which is problem #1 here. And it frees only its
own list nodes, leaving each fn to decide the payload's fate, which is
the node-versus-value split v2 introduces. The POSIX build uses
pthread_key_create and the Windows build maps the same key onto
FlsAlloc, so one mechanism covers both with no Windows leak. It is not
something Postgres would depend on, just prior art showing the shape in
this patch ports cleanly, Windows included.

-greg