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

On Tue, 30 Jun 2026 at 06:56, Thom Brown <thom(at)linux(dot)com> wrote:
>
> On Tue, 30 Jun 2026 at 03:17, Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> >
> > On Tue, Jun 30, 2026 at 1:21 PM Thom Brown <thom(at)linux(dot)com> wrote:
> > > On Tue, 30 Jun 2026 at 00:24, Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> > > > EXEC SQL AT con1 SELECT datname INTO :anything FROM pg_database;
> > > > EXEC SQL AT con2 ... something that reaches ecpg_raise() ...
> > > >
> > > > /* Why should "anything" not be accessible, and mine to free(), here? */
> > >
> > > I see, yeah, that does seem wrong. 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() so that it cleans up at the end of the statement
> > > instead of the start of the next one?
> >
> > Oh, that sounds plausible and simple! Have I understood the
> > lifetime/ownership rule correctly here?: after a successful statement,
> > ownership of auto-allocated objects is transferred to the caller, and
> > the fact that we currently don't "clear" (forget, disown) the list
> > until the next statement is just an implementation detail and we could
> > indeed make it "eager". In contrast, ECPGfree_auto_mem() is only used
> > to clean up partially allocated variables on failure, since then the
> > caller doesn't own anything. If that all makes sense, then perhaps
> > the auto-mem list could always be NULL between ECPG statements,
> > meaning that ownership of "anything" would already have been
> > transferred to the caller (cleared) in this example. And then perhaps
> > we wouldn't even need a thread-exit destructor here? On the other
> > hand, that'd be a subtle semantic change that could upset an
> > application. So perhaps this would have to be a well-signalled
> > master-only change, if it makes sense to do it? I haven't actually
> > tried any of that or written a test (feel free if you're interested in
> > looking at this), I'm just trying to confirm my understanding of the
> > design.
> >
> > This is independent of the destructor issue I started with, which I
> > assume we'd want to back-patch. It would be nice if we could do
> > something like the above in master and that leads to deleting the
> > destructor though (we still need thread-exit cleanup for other ECPG
> > things though, and need a new system for that that also works on
> > Windows, more patches soon..., so it's not a big deal either way).
>
> AFAICT, ownership transfers to the caller on success, and the lazy
> clearing at next statement is just timing. But I think
> ECPGfree_auto_mem() isn't only internal failure-cleanup. It's exported
> and called by application code as the bulk-free for GET DESCRIPTOR ...
> INTO :array results, which the caller does own.
>
> I'm looking at dynalloc.pgc, line 83, and those arrays look like
> they're auto-allocated in descriptor.c:
>
> Line 437 onward, for the data array:
>
> /* allocate storage if needed */
> if (arrsize == 0 && *(void **) var == NULL)
> {
> void *mem = ecpg_auto_alloc(offset * ntuples, lineno);
>
> if (!mem)
> {
> va_end(args);
> return false;
> }
> *(void **) var = mem;
> var = mem;
> }
>
> And there's another one at line 557 onward for the indicator array

Correction: the allocation I pointed at in descriptor.c (line 437)
isn't for the data array. That branch handles
RETURNED_LENGTH/RETURNED_OCTET_LENGTH items. The data and indicator
arrays in dynalloc.pgc actually come from ecpg_store_result(),
execute.c lines 398 and 409, which ECPGget_desc() calls at
descriptor.c line 519. The one at line 557 is for an indicator given
without a data variable.

Doesn't change the conclusion: everything GET DESCRIPTOR materialises
into a zero-sized pointer variable ends up on the auto-mem list, so
the bulk-free usage and the need for thread-exit node cleanup still
stand.

Thom