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

Thanks! Your patch successfully passes my tests (the ones that found
the original freev() issue =E2=80=94 the REALLOC bug was found by reading t=
he
code).

=E2=80=93Arthur

On Fri, Jul 18, 2014 at 10:08 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Arthur O'Dwyer" <arthur.j.odwyer@gmail.com> writes:
>> On Thu, Jul 17, 2014 at 10:04 AM, Tom Lane <tgl@sss.pgh.pa.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=3Dpostgresql.git;a=3Dcommitdiff;h=3D1=
567e659a877d35ab4b85dafff41b2845d50990f
>
>                         regards, tom lane