| Message ID | patch-12.20-9be5b0c7836-20221228T175512Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leak fixes: various simple leak fixes | expand |
Am 28.12.22 um 19:00 schrieb Ævar Arnfjörð Bjarmason: > Fix a memory leak that's been with us ever since > 2f4038ab337 (Git-aware CGI to provide dumb HTTP transport, > 2009-10-30). In this case we're not calling regerror() after a failed > regexec(), and don't otherwise use "re" afterwards. > > We can therefore simplify this code by calling regfree() right after > the regexec(). An alternative fix would be to add a regfree() to both > the "return" and "break" path in this for-loop. Yes, or to add one regfree() call early in the conditional... > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > http-backend.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/http-backend.c b/http-backend.c > index 6eb3b2fe51c..9bb63c458b1 100644 > --- a/http-backend.c > +++ b/http-backend.c > @@ -759,10 +759,14 @@ int cmd_main(int argc, const char **argv) > struct service_cmd *c = &services[i]; > regex_t re; > regmatch_t out[1]; > + int ret; > > if (regcomp(&re, c->pattern, REG_EXTENDED)) > die("Bogus regex in service table: %s", c->pattern); > - if (!regexec(&re, dir, 1, out, 0)) { > + ret = regexec(&re, dir, 1, out, 0); > + regfree(&re); > + > + if (!ret) { > size_t n; > ... i.e. right here. But only after copying the offsets out of "out"! Anyway, the ret approach taken here is fine. "dir" is still leaking, by the way. Probably worth a separate patch. > if (strcmp(method, c->method)) > @@ -774,7 +778,6 @@ int cmd_main(int argc, const char **argv) > dir[out[0].rm_so] = 0; > break; > } > - regfree(&re); > } > > if (!cmd)
René Scharfe <l.s.r@web.de> writes: >> diff --git a/http-backend.c b/http-backend.c >> index 6eb3b2fe51c..9bb63c458b1 100644 >> --- a/http-backend.c >> +++ b/http-backend.c >> @@ -759,10 +759,14 @@ int cmd_main(int argc, const char **argv) >> struct service_cmd *c = &services[i]; >> regex_t re; >> regmatch_t out[1]; >> + int ret; >> >> if (regcomp(&re, c->pattern, REG_EXTENDED)) >> die("Bogus regex in service table: %s", c->pattern); >> - if (!regexec(&re, dir, 1, out, 0)) { >> + ret = regexec(&re, dir, 1, out, 0); >> + regfree(&re); >> + >> + if (!ret) { >> size_t n; >> > > ... i.e. right here. But only after copying the offsets out of "out"! "only after..."? Do out[i].rm_eo and out[i].rm_so become invalid after calling regfree() on the regex out[] was taken against? I do not think so, and am confused by the comment. After all, if we can free only after copying the offsets out of "out", the posted patch in question is already wrong ;-) > Anyway, the ret approach taken here is fine. Yup. > "dir" is still leaking, by the way. Probably worth a separate patch. Good eyes. But I'd actually say UNLEAK() is fine for that one. Thanks.
Am 29.12.22 um 08:32 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >>> diff --git a/http-backend.c b/http-backend.c >>> index 6eb3b2fe51c..9bb63c458b1 100644 >>> --- a/http-backend.c >>> +++ b/http-backend.c >>> @@ -759,10 +759,14 @@ int cmd_main(int argc, const char **argv) >>> struct service_cmd *c = &services[i]; >>> regex_t re; >>> regmatch_t out[1]; >>> + int ret; >>> >>> if (regcomp(&re, c->pattern, REG_EXTENDED)) >>> die("Bogus regex in service table: %s", c->pattern); >>> - if (!regexec(&re, dir, 1, out, 0)) { >>> + ret = regexec(&re, dir, 1, out, 0); >>> + regfree(&re); >>> + >>> + if (!ret) { >>> size_t n; >>> >> >> ... i.e. right here. But only after copying the offsets out of "out"! > > "only after..."? Do out[i].rm_eo and out[i].rm_so become invalid > after calling regfree() on the regex out[] was taken against? I do > not think so, and am confused by the comment. Nah, sorry, I was confused. o_O "out" is not affected by regfree(), of course; it's supplied by the regexec() caller -- cmd_main() owns it. René
diff --git a/http-backend.c b/http-backend.c index 6eb3b2fe51c..9bb63c458b1 100644 --- a/http-backend.c +++ b/http-backend.c @@ -759,10 +759,14 @@ int cmd_main(int argc, const char **argv) struct service_cmd *c = &services[i]; regex_t re; regmatch_t out[1]; + int ret; if (regcomp(&re, c->pattern, REG_EXTENDED)) die("Bogus regex in service table: %s", c->pattern); - if (!regexec(&re, dir, 1, out, 0)) { + ret = regexec(&re, dir, 1, out, 0); + regfree(&re); + + if (!ret) { size_t n; if (strcmp(method, c->method)) @@ -774,7 +778,6 @@ int cmd_main(int argc, const char **argv) dir[out[0].rm_so] = 0; break; } - regfree(&re); } if (!cmd)
Fix a memory leak that's been with us ever since 2f4038ab337 (Git-aware CGI to provide dumb HTTP transport, 2009-10-30). In this case we're not calling regerror() after a failed regexec(), and don't otherwise use "re" afterwards. We can therefore simplify this code by calling regfree() right after the regexec(). An alternative fix would be to add a regfree() to both the "return" and "break" path in this for-loop. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- http-backend.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)