{"thread":{"id":"45844","subject":"[PATCH] small memory leak fix","startedAt":"2017-05-01T20:40:37Z","lastAt":"2017-05-01T21:38:35Z","messageCount":3,"participants":["David CARLIER","Stefan Beller","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"318403","messageId":"C3A43A45-9120-4755-AE42-FD33F68F3FCB@gmail.com","threadId":"45844","inReplyTo":null,"subject":"[PATCH] small memory leak fix","fromName":"David CARLIER","fromEmail":"devnexen@gmail.com","sentAt":"2017-05-01T20:40:24Z","receivedAt":"2017-05-01T20:40:37Z","isPatch":true,"sender":{"key":"devnexen@gmail.com","avatar":null},"body":"Hi this is my first submission, a memory leak found in the maint branch (might be in master as well).\n\nKind regards.\n\nFrom d98f3944780730447f111a4178c9d99f5110c260 Mon Sep 17 00:00:00 2001\nFrom: David Carlier \nDate: Mon, 1 May 2017 21:14:09 +0100\nSubject: [PATCH] memory leaks fixes for remote lists.\n\nboth push and fetch lists were not freed thus\nusing free_refspecs usage.\n\nSigned-off-by: David Carlier \n---\n remote.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/remote.c b/remote.c\nindex 9f83fe2c4..2f8cb35a3 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -742,6 +742,8 @@ int for_each_remote(each_remote_fn fn, void *priv)\n \t\t\tr->push = parse_push_refspec(r->push_refspec_nr,\n \t\t\t\t\t\t r->push_refspec);\n \t\tresult = fn(r, priv);\n+\t\tfree_refspecs(r->push, r->push_refspec_nr);\n+\t\tfree_refspecs(r->fetch, r->fetch_refspec_nr);\n \t}\n \treturn result;\n }\n-- \n2.12.2\n\n"},{"id":"318404","messageId":"CAGZ79kYfoBOc+hVhm7XEg1TvYQ8myF4f=0wSwDs-agYxaArsWQ@mail.gmail.com","threadId":"45844","inReplyTo":"C3A43A45-9120-4755-AE42-FD33F68F3FCB@gmail.com","subject":"Re: [PATCH] small memory leak fix","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-01T20:50:57Z","receivedAt":"2017-05-01T20:51:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 1, 2017 at 1:40 PM, David CARLIER wrote:\n> Hi this is my first submission, a memory leak found in the maint branch (might be in master as well).\n>\n> Kind regards.\n\nHi and welcome to Git!\n\n> From d98f3944780730447f111a4178c9d99f5110c260 Mon Sep 17 00:00:00 2001\n> From: David Carlier \n> Date: Mon, 1 May 2017 21:14:09 +0100\n> Subject: [PATCH] memory leaks fixes for remote lists.\n\nWe usually word the subject line as \"area: what we do\"\n(C.f. $ git log --oneline --no-merges -- remote.c)\nMaybe:\n\n remote.c: plug memleak in for_each_remote\n\n>\n> both push and fetch lists were not freed thus\n> using free_refspecs usage.\n>\n> Signed-off-by: David Carlier \n> ---\n> remote.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/remote.c b/remote.c\n> index 9f83fe2c4..2f8cb35a3 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -742,6 +742,8 @@ int for_each_remote(each_remote_fn fn, void *priv)\n> r->push = parse_push_refspec(r->push_refspec_nr,\n> r->push_refspec);\n> result = fn(r, priv);\n> + free_refspecs(r->push, r->push_refspec_nr);\n> + free_refspecs(r->fetch, r->fetch_refspec_nr);\n\nAfter freeing the refspec, r->push/fetch still points to\nthe (now free'd) memory. We'd want to reset it to NULL as well,\nsuch that e.g. in this function\n\n if (!r->fetch)\n ...\n\nstill works.\n\nAfter reading this, I think we'd rather want to keep the fetch/push refspec\naround for the next access of the struct remote, and only free the memory\nwhen the remote itself is freed?\n\nThat however is a problem as we never free them, they are globals :(\n\nThanks,\nStefan\n"},{"id":"318408","messageId":"20170501213820.4miu5qizzifmavrb@sigill.intra.peff.net","threadId":"45844","inReplyTo":"CAGZ79kYfoBOc+hVhm7XEg1TvYQ8myF4f=0wSwDs-agYxaArsWQ@mail.gmail.com","subject":"Re: [PATCH] small memory leak fix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-01T21:38:20Z","receivedAt":"2017-05-01T21:38:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 01, 2017 at 01:50:57PM -0700, Stefan Beller wrote:\n\n> > diff --git a/remote.c b/remote.c\n> > index 9f83fe2c4..2f8cb35a3 100644\n> > --- a/remote.c\n> > +++ b/remote.c\n> > @@ -742,6 +742,8 @@ int for_each_remote(each_remote_fn fn, void *priv)\n> > r->push = parse_push_refspec(r->push_refspec_nr,\n> > r->push_refspec);\n> > result = fn(r, priv);\n> > + free_refspecs(r->push, r->push_refspec_nr);\n> > + free_refspecs(r->fetch, r->fetch_refspec_nr);\n> \n> After freeing the refspec, r->push/fetch still points to\n> the (now free'd) memory. We'd want to reset it to NULL as well,\n> such that e.g. in this function\n> \n> if (!r->fetch)\n> ...\n> \n> still works.\n> \n> After reading this, I think we'd rather want to keep the fetch/push refspec\n> around for the next access of the struct remote, and only free the memory\n> when the remote itself is freed?\n> \n> That however is a problem as we never free them, they are globals :(\n\nYeah, I think the point is that the whole \"remotes\" array is a\nprogram-length global that never goes away (and must not, because after\nread_config() sets the \"loaded\" flag, we would never reload it).\n\nThe \"fetch\" and \"push\" bits are lazily parsed from the refspec strings,\nbut are intended to have the same lifetime. Unlike the rest of it, we\n_could_ drop them after use and then lazy-parse them again.\n\nBut note that we call an arbitrary callback in this function. What\nexpectations does it have for the lifetimes? Do any of the callbacks\nrecord pointers to the refspecs? Or for that mater, the patch as shown\nfrees the refspecs even if we didn't just lazily allocate them in this\nfunction (e.g., if we did so in remote_get_1()).\n\nSo I don't think freeing them is safe unless we do a complete audit of\naccess to those refspecs. And it's probably not worth the trouble; these\nshould just follow the same global-until-exit allocation scheme as the\nrest of \"struct remote\".\n\n-Peff\n"}]}