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

"Arthur O'Dwyer" <arthur(dot)j(dot)odwyer(at)gmail(dot)com> writes:
> On Thu, Jul 17, 2014 at 10:04 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> 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?

No, the INUSE marking doesn't happen till pg_regcomp runs markst(), so
that would break cleanup of failures occurring before that. There's
somewhat of a narrow window for this case, since v->tree doesn't get
set until parse() returns, but failures there certainly are possible.

After some reflection I decided that what we need is to teach freesubre
to stick things back into the treefree list if and only if treechain is
non-NULL. This guarantees we can't corrupt the treechain list with a
premature free of a subre, and it preserves the existing not-broken
logic for cleaning up after an error occuring before we reach markst().
So it ends up being one line of code change, though I added a bunch of
commentary as well:

http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=1567e659a877d35ab4b85dafff41b2845d50990f

regards, tom lane