Re: BUG #10976: Two memory leaks in regcomp cleanup

arthur(dot)j(dot)odwyer(at)gmail(dot)com writes:
> When MALLOC fails, pg_regcomp leaks memory in at least two places:

> (A) In freev(), the line
> freesubre(info, v, v->tree);
> should be
> freesubre(info, NULL, v->tree);
> as otherwise the "freed" subres will end up on v->treefree, which is leaked
> by the cleanst() two lines later.

Hmm ... what version of the code are you looking at exactly? There's no
"info" argument here in Postgres.

> That is, given the precondition that there are things in v->tree that aren't
> in v->treechain.

The problem with this proposal is that if there are subres in v->tree
that *are* in the treechain, we'll possibly try to free them twice
(if they're not marked INUSE), and definitely will be accessing
already-freed memory when cleanst looks at them.

It looks to me like there are two different operating regimes in this
code: one where all the subres are still in the treechain, and one where
we've marked as INUSE all the ones that are reachable from v->tree and
garbage-collected the rest. The markst/cleanst steps in pg_regcomp are
where the conversion is made. freev needs to work correctly either
before or after that.

In short, I think you're right that there's a bug here, but this is
not a good fix for it. I'm not sure freev has enough info to do the
right thing; we may need to rethink the data structure invariants,
so that there's not two different ways to clean up.

> (B) newlacon() leaks memory if REALLOC returns NULL on this line:

> v->lacons = (struct subre *) REALLOC(v->lacons,
> (v->nlacons + 1) * sizeof(struct subre));

> The fix is to use the same idiom already used everywhere else REALLOC is
> called in this module.

A temp variable, you mean. Yeah, that's a bug.

regards, tom lane