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

Thanks! Your patch successfully passes my tests (the ones that found
the original freev() issue — the REALLOC bug was found by reading the
code).

–Arthur

On Fri, Jul 18, 2014 at 10:08 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "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