Horton

Thread

Commits

  1. Applied patch by Peter Harris to free auto_mem structure on connect

  1. Per-thread leak in ECPG's memory.c

    Thomas Munro <thomas.munro@gmail.com> — 2026-06-29T13:19:29Z

    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.

Hmm, I wonder why ecpg_raise() frees auto-allocated values for all
connections just because one connection raised an error.

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

    Thom Brown <thom@linux.com> — 2026-06-29T14:30:51Z

    On Mon, 29 Jun 2026 at 14:09, Thomas Munro <thomas.munro@gmail.com> 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.
>
> Hmm, I wonder why ecpg_raise() frees auto-allocated values for all
> connections just because one connection raised an error.

I got curious, and found this in connect.c:

    /*
     * clear auto_mem structure because some error handling functions might
     * access it
     */
    ecpg_clear_auto_mem();

If I skip the pgindent commit, the blame gives us this commit:

commit 0c96e42797dbe2918c909209abbaee4d2c985e38
Author: Michael Meskes <meskes@postgresql.org>
Date:   Wed Nov 8 10:46:47 2006 +0000

    Applied patch by Peter Harris to free auto_mem structure on connect


Digging into bug reports from that time, we get:
https://www.postgresql.org/message-id/200611071423.kA7ENpJ1080586%40wwwmaster.postgresql.org

"When using more than one database connection with ECPG, you might have
obtained and freed blocks of data on one connection before trying to open
the other.
If the second connection fails, ECPGraise will be called and call
ECPGfree_auto_mem.  This can cause an invalid free() of a pointer you've
already freed."

I couldn't find the actual patch though.

Thom

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

    Thomas Munro <thomas.munro@gmail.com> — 2026-06-29T23:34:31Z

    On Tue, Jun 30, 2026 at 2:31 AM Thom Brown <thom@linux.com> wrote:
> On Mon, 29 Jun 2026 at 14:09, Thomas Munro <thomas.munro@gmail.com> wrote:
> > Hmm, I wonder why ecpg_raise() frees auto-allocated values for all
> > connections just because one connection raised an error.

> Digging into bug reports from that time, we get:
> https://www.postgresql.org/message-id/200611071423.kA7ENpJ1080586%40wwwmaster.postgresql.org
>
> "When using more than one database connection with ECPG, you might have
> obtained and freed blocks of data on one connection before trying to open
> the other.
> If the second connection fails, ECPGraise will be called and call
> ECPGfree_auto_mem.  This can cause an invalid free() of a pointer you've
> already freed."

Thanks for finding that.  Hmm, OK, but I was wondering about the
opposite scenario, where you *haven't* freed blocks of data before
doing something on another connection that frees everything for the
thread:

  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? */

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

    Thom Brown <thom@linux.com> — 2026-06-30T01:21:20Z

    On Tue, 30 Jun 2026 at 00:24, Thomas Munro <thomas.munro@gmail.com> wrote:
>
> On Tue, Jun 30, 2026 at 2:31 AM Thom Brown <thom@linux.com> wrote:
> > On Mon, 29 Jun 2026 at 14:09, Thomas Munro <thomas.munro@gmail.com> wrote:
> > > Hmm, I wonder why ecpg_raise() frees auto-allocated values for all
> > > connections just because one connection raised an error.
>
> > Digging into bug reports from that time, we get:
> > https://www.postgresql.org/message-id/200611071423.kA7ENpJ1080586%40wwwmaster.postgresql.org
> >
> > "When using more than one database connection with ECPG, you might have
> > obtained and freed blocks of data on one connection before trying to open
> > the other.
> > If the second connection fails, ECPGraise will be called and call
> > ECPGfree_auto_mem.  This can cause an invalid free() of a pointer you've
> > already freed."
>
> Thanks for finding that.  Hmm, OK, but I was wondering about the
> opposite scenario, where you *haven't* freed blocks of data before
> doing something on another connection that frees everything for the
> thread:
>
>   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?

Thom

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

    Thomas Munro <thomas.munro@gmail.com> — 2026-06-30T02:16:47Z

    On Tue, Jun 30, 2026 at 1:21 PM Thom Brown <thom@linux.com> wrote:
> On Tue, 30 Jun 2026 at 00:24, Thomas Munro <thomas.munro@gmail.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).

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

    Thom Brown <thom@linux.com> — 2026-06-30T05:56:04Z

    On Tue, 30 Jun 2026 at 03:17, Thomas Munro <thomas.munro@gmail.com> wrote:
>
> On Tue, Jun 30, 2026 at 1:21 PM Thom Brown <thom@linux.com> wrote:
> > On Tue, 30 Jun 2026 at 00:24, Thomas Munro <thomas.munro@gmail.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

So maybe it really does both cleanup on failure, and free descriptor
arrays that the caller owns. The change I semi-proposed would mean the
list would be NULL between statements, which holds for SELECT-INTO,
although not for descriptors. I don't think the destructor fully goes
away. A thread that does GET DESCRIPTOR and exits before bulk-freeing
still leaves nodes on the list. So thread-exit node cleanup is still
needed for the descriptor case, which matches what you said about
needing a new Windows-capable mechanism anyway.


Thom

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

    Thom Brown <thom@linux.com> — 2026-07-02T02:46:14Z

    On Tue, 30 Jun 2026 at 06:56, Thom Brown <thom@linux.com> wrote:
>
> On Tue, 30 Jun 2026 at 03:17, Thomas Munro <thomas.munro@gmail.com> wrote:
> >
> > On Tue, Jun 30, 2026 at 1:21 PM Thom Brown <thom@linux.com> wrote:
> > > On Tue, 30 Jun 2026 at 00:24, Thomas Munro <thomas.munro@gmail.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

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

    Heikki Linnakangas <hlinnaka@iki.fi> — 2026-07-06T19:34:59Z

    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!

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.

> 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