| Message ID | 20240924220446.GT1143820@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4324c6c0d97ec48be18dfae8f1c1c3fb77d43f45 |
| Headers | show |
| Series | leak fixes for http fetch/push | expand |
On Tue, Sep 24, 2024 at 06:04:46PM -0400, Jeff King wrote: > Our repo->url string comes from str_end_url_with_slash(), which always > allocates its output buffer. We should free it before exiting to avoid > triggering the leak-checker. > > This can be seen by leak-checking t5540. > > Signed-off-by: Jeff King <peff@peff.net> > --- > http-push.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/http-push.c b/http-push.c > index f60b2ceba5..52c53928a9 100644 > --- a/http-push.c > +++ b/http-push.c > @@ -1972,6 +1972,7 @@ int cmd_main(int argc, const char **argv) > cleanup: > if (info_ref_lock) > unlock_remote(info_ref_lock); > + free(repo->url); > free(repo); > > http_cleanup(); I was wondering whether we also need to free `repo->path`, which is a `char *` as well. But that is only being assigned pointers into command line argument strings, and thus we do not have to free it. Patrick
On Thu, Sep 26, 2024 at 03:50:24PM +0200, Patrick Steinhardt wrote: > > diff --git a/http-push.c b/http-push.c > > index f60b2ceba5..52c53928a9 100644 > > --- a/http-push.c > > +++ b/http-push.c > > @@ -1972,6 +1972,7 @@ int cmd_main(int argc, const char **argv) > > cleanup: > > if (info_ref_lock) > > unlock_remote(info_ref_lock); > > + free(repo->url); > > free(repo); > > > > http_cleanup(); > > I was wondering whether we also need to free `repo->path`, which is a > `char *` as well. But that is only being assigned pointers into command > line argument strings, and thus we do not have to free it. Yeah, I think that "path" has the wrong type, and should be "const char *". I was a little surprised the compiler does not complain, but it is the old C gotcha: we assign to it via strchr(), which launders away the const. It would probably be OK to fix, though in general my dream is still that we'd delete all of this code in the not too distant future. Even if we keep dumb-http fetch for its resumability, dumb-http push just seems like a useless holdover from the early days. -Peff
diff --git a/http-push.c b/http-push.c index f60b2ceba5..52c53928a9 100644 --- a/http-push.c +++ b/http-push.c @@ -1972,6 +1972,7 @@ int cmd_main(int argc, const char **argv) cleanup: if (info_ref_lock) unlock_remote(info_ref_lock); + free(repo->url); free(repo); http_cleanup();
Our repo->url string comes from str_end_url_with_slash(), which always allocates its output buffer. We should free it before exiting to avoid triggering the leak-checker. This can be seen by leak-checking t5540. Signed-off-by: Jeff King <peff@peff.net> --- http-push.c | 1 + 1 file changed, 1 insertion(+)