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

On Thu, Jul 17, 2014 at 10:04 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> 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.

Ah, yes, we piped an "info" argument through all the places that call
MALLOC/FREE, so that compiling a regex wouldn't have to touch global
state. You can safely pretend I didn't write "info," there.

>> 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.

Hmm. I think you're right --- I *think* the subres in v->tree are
INUSE by definition, so double-free isn't an issue, but cleanst will
definitely be looking at them after they've been freed, which is still
a bug. What if you just swap the order that freev() does cleanst() and
freesubre() so that the cleanst() happens first?

> 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.

Please keep me cc'ed and/or send me an email when there's an
"official" patch for this leak.

Thanks,
Arthur