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

From: "Arthur O'Dwyer" <arthur(dot)j(dot)odwyer(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: pgsql-bugs(at)postgresql(dot)org
Subject: Re: BUG #10976: Two memory leaks in regcomp cleanup
Date: 2014-07-18 18:58:49
Message-ID: CADvuK0Ljqgu7QzoPGocf1O3Mr9=5u3429P7Sx82JtzPWA-L6sA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-bugs

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

In response to

Browse pgsql-bugs by date

  From Date Subject
Next Message Matheus de Oliveira 2014-07-18 20:17:42 Re: BUG #10991: psql -c ignores my pager settings in ~/.psqlrc
Previous Message David G Johnston 2014-07-18 17:54:18 Re: BUG #10991: psql -c ignores my pager settings in ~/.psqlrc