Thread: BUG #10976: Two memory leaks in regcomp cleanup
The following bug has been logged on the website: Bug reference: 10976 Logged by: Arthur O'Dwyer Email address: arthur.j.odwyer@gmail.com PostgreSQL version: 9.3.0 Operating system: Ubuntu Linux Description: 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. That is, given the precondition that there are things in v->tree that aren't in v->treechain. This precondition is invariably true if we are being called because nfatree() has run out of memory here: markst(v->tree); cleanst(info, v); /* clears v->treechain without clearing v->tree */ [...some comments...] re->re_info |= nfatree(info, v, v->tree, debug); CNOERR(); /* calls freev() */ (B) newlacon() leaks memory if REALLOC returns NULL on this line: v->lacons = (struct subre *) REALLOC(v->lacons, (v->nlacons + 1) * sizeof(struct subre)); The fix is to use the same idiom already used everywhere else REALLOC is called in this module.
arthur.j.odwyer@gmail.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. > 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. 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. > (B) newlacon() leaks memory if REALLOC returns NULL on this line: > v->lacons = (struct subre *) REALLOC(v->lacons, > (v->nlacons + 1) * sizeof(struct subre)); > The fix is to use the same idiom already used everywhere else REALLOC is > called in this module. A temp variable, you mean. Yeah, that's a bug. regards, tom lane
On Thu, Jul 17, 2014 at 10:04 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > arthur.j.odwyer@gmail.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
"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=postgresql.git;a=commitdiff;h=1567e659a877d35ab4b85dafff41b2845d50990f regards, tom lane
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