{"thread":{"id":"37321","subject":"[PATCH 2/2] clone.c: don't leak memory in cmd_clone","startedAt":"2014-08-10T13:57:55Z","lastAt":"2014-08-10T23:03:32Z","messageCount":6,"participants":["Stefan Beller","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"247550","messageId":"1407679076-20300-1-git-send-email-stefanbeller@gmail.com","threadId":"37321","inReplyTo":null,"subject":"[PATCH 1/2] remote.c: don't leak the base branch name in format_tracking_info","fromName":"Stefan Beller","fromEmail":"stefanbeller@gmail.com","sentAt":"2014-08-10T13:57:55Z","receivedAt":"2014-08-10T13:57:55Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Found by scan.coverity.com (Id: 1127809)\n\nSigned-off-by: Stefan Beller <stefanbeller@gmail.com>\n---\n remote.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/remote.c b/remote.c\nindex 3d6c86a..2c1458f 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1983,6 +1983,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)\n \t\t\tstrbuf_addf(sb,\n \t\t\t\t_(\"  (use \\\"git pull\\\" to merge the remote branch into yours)\\n\"));\n \t}\n+\tfree(base);\n \treturn 1;\n }\n \n-- \n2.1.0.rc2\n"},{"id":"247549","messageId":"1407679076-20300-2-git-send-email-stefanbeller@gmail.com","threadId":"37321","inReplyTo":"1407679076-20300-1-git-send-email-stefanbeller@gmail.com","subject":"[PATCH 2/2] clone.c: don't leak memory in cmd_clone","fromName":"Stefan Beller","fromEmail":"stefanbeller@gmail.com","sentAt":"2014-08-10T13:57:56Z","receivedAt":"2014-08-10T13:57:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Free the refspec.\nFound by scan.coverity.com (Id: 1127806)\n\nSigned-off-by: Stefan Beller <stefanbeller@gmail.com>\n---\n builtin/clone.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex bbd169c..dd4092b 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1004,5 +1004,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&key);\n \tstrbuf_release(&value);\n \tjunk_mode = JUNK_LEAVE_ALL;\n+\n+\tfree(refspec);\n \treturn err;\n }\n-- \n2.1.0.rc2\n"},{"id":"247554","messageId":"53E78C59.2010704@gmail.com","threadId":"37321","inReplyTo":"1407679076-20300-1-git-send-email-stefanbeller@gmail.com","subject":"Re: [PATCH 1/2] remote.c: don't leak the base branch name in format_tracking_info","fromName":"Stefan Beller","fromEmail":"stefanbeller@gmail.com","sentAt":"2014-08-10T15:14:33Z","receivedAt":"2014-08-10T15:14:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 10.08.2014 15:57, Stefan Beller wrote:\n> Found by scan.coverity.com (Id: 1127809)\n> \n> Signed-off-by: Stefan Beller <stefanbeller@gmail.com>\n> ---\n>  remote.c | 1 +\n>  1 file changed, 1 insertion(+)\n> \n> diff --git a/remote.c b/remote.c\n> index 3d6c86a..2c1458f 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1983,6 +1983,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)\n>  \t\t\tstrbuf_addf(sb,\n>  \t\t\t\t_(\"  (use \\\"git pull\\\" to merge the remote branch into yours)\\n\"));\n>  \t}\n> +\tfree(base);\n>  \treturn 1;\n>  }\n>  \n> \n\nUpon testing this one again, I get a warning\nremote.c: In function ‘format_tracking_info’:\nremote.c:1986:2: warning: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type [enabled by default]\n  free(base);\n  ^\nIn file included from git-compat-util.h:103:0,\n                 from cache.h:4,\n                 from remote.c:1:\n/usr/include/stdlib.h:483:13: note: expected ‘void *’ but argument is of type ‘const char *’\n extern void free (void *__ptr) __THROW;\n             ^\n\nPlease ignore this patch.\n"},{"id":"247558","messageId":"20140810193242.GA9886@peff.net","threadId":"37321","inReplyTo":"53E78C59.2010704@gmail.com","subject":"Re: [PATCH 1/2] remote.c: don't leak the base branch name in format_tracking_info","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-10T19:32:42Z","receivedAt":"2014-08-10T19:32:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 10, 2014 at 05:14:33PM +0200, Stefan Beller wrote:\n\n> On 10.08.2014 15:57, Stefan Beller wrote:\n> > Found by scan.coverity.com (Id: 1127809)\n> > \n> > Signed-off-by: Stefan Beller <stefanbeller@gmail.com>\n> > ---\n> >  remote.c | 1 +\n> >  1 file changed, 1 insertion(+)\n> > \n> > diff --git a/remote.c b/remote.c\n> > index 3d6c86a..2c1458f 100644\n> > --- a/remote.c\n> > +++ b/remote.c\n> > @@ -1983,6 +1983,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)\n> >  \t\t\tstrbuf_addf(sb,\n> >  \t\t\t\t_(\"  (use \\\"git pull\\\" to merge the remote branch into yours)\\n\"));\n> >  \t}\n> > +\tfree(base);\n> >  \treturn 1;\n> >  }\n> >  \n> > \n> \n> Upon testing this one again, I get a warning\n> remote.c: In function ‘format_tracking_info’:\n> remote.c:1986:2: warning: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type [enabled by default]\n>   free(base);\n>   ^\n> In file included from git-compat-util.h:103:0,\n>                  from cache.h:4,\n>                  from remote.c:1:\n> /usr/include/stdlib.h:483:13: note: expected ‘void *’ but argument is of type ‘const char *’\n>  extern void free (void *__ptr) __THROW;\n>              ^\n> \n> Please ignore this patch.\n\nI think your patch is definitely fixing a leak; it's just that the\nexisting code is a little sloppy. It does:\n\n  const char *base;\n  ...\n  base = branch->merge[0]->dst;\n  base = shorten_unambiguous_ref(base, 0);\n\nIn the first assignment, \"base\" should be const, as we are pointing to\nsomebody else's memory. But in the second, we use the same pointer to\nstore newly allocated memory from shorten_unambiguous_ref.\n\nIn the general case, you need two pointers to do this right. However, we\ndon't actually look at \"base\" between the two assignments, so I think\nyou could just do it as:\n\n  char *base;\n  ...\n  base = shorten_unambiguous_ref(branch->merge[0]->dst, 0);\n\n-Peff\n"},{"id":"247560","messageId":"1407699813-1460-1-git-send-email-stefanbeller@gmail.com","threadId":"37321","inReplyTo":"20140810193242.GA9886@peff.net","subject":"[PATCH] remote.c: don't leak the base branch name in format_tracking_info","fromName":"Stefan Beller","fromEmail":"stefanbeller@gmail.com","sentAt":"2014-08-10T19:43:33Z","receivedAt":"2014-08-10T19:43:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Found by scan.coverity.com (Id: 1127809)\n\nSigned-off-by: Stefan Beller <stefanbeller@gmail.com>\n---\n remote.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 3d6c86a..894db09 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1920,7 +1920,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n int format_tracking_info(struct branch *branch, struct strbuf *sb)\n {\n \tint ours, theirs;\n-\tconst char *base;\n+\tchar *base;\n \tint upstream_is_gone = 0;\n \n \tswitch (stat_tracking_info(branch, &ours, &theirs)) {\n@@ -1936,8 +1936,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)\n \t\tbreak;\n \t}\n \n-\tbase = branch->merge[0]->dst;\n-\tbase = shorten_unambiguous_ref(base, 0);\n+\tbase = shorten_unambiguous_ref(branch->merge[0]->dst, 0);\n \tif (upstream_is_gone) {\n \t\tstrbuf_addf(sb,\n \t\t\t_(\"Your branch is based on '%s', but the upstream is gone.\\n\"),\n@@ -1983,6 +1982,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)\n \t\t\tstrbuf_addf(sb,\n \t\t\t\t_(\"  (use \\\"git pull\\\" to merge the remote branch into yours)\\n\"));\n \t}\n+\tfree(base);\n \treturn 1;\n }\n \n-- \n2.1.0.rc2\n"},{"id":"247570","messageId":"xmqq7g2g7xrv.fsf@gitster.dls.corp.google.com","threadId":"37321","inReplyTo":"53E78C59.2010704@gmail.com","subject":"Re: [PATCH 1/2] remote.c: don't leak the base branch name in format_tracking_info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-10T23:03:32Z","receivedAt":"2014-08-10T23:03:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <stefanbeller@gmail.com> writes:\n\n> On 10.08.2014 15:57, Stefan Beller wrote:\n>> Found by scan.coverity.com (Id: 1127809)\n>> \n>> Signed-off-by: Stefan Beller <stefanbeller@gmail.com>\n>> ---\n>>  remote.c | 1 +\n>>  1 file changed, 1 insertion(+)\n>> \n>> diff --git a/remote.c b/remote.c\n>> index 3d6c86a..2c1458f 100644\n>> --- a/remote.c\n>> +++ b/remote.c\n>> @@ -1983,6 +1983,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)\n>>  \t\t\tstrbuf_addf(sb,\n>>  \t\t\t\t_(\"  (use \\\"git pull\\\" to merge the remote branch into yours)\\n\"));\n>>  \t}\n>> +\tfree(base);\n>>  \treturn 1;\n>>  }\n>>  \n>> \n>\n> Upon testing this one again, I get a warning\n> remote.c: In function ‘format_tracking_info’:\n> remote.c:1986:2: warning: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type [enabled by default]\n>   free(base);\n>   ^\n> ...\n> Please ignore this patch.\n\nIt is perfectly fine to cast it to (char *) in this case, I think.\n\nThe real culprit is that the functionà reuses the same \"base\" (which\nis a pointer into a constant region of memory) to receive the new\ncopy allocated by shorten_unambiguous_ref(); the piece of memory\nreturned by the callee belongs to this function, and it is perfectly\nfine if this function modifies the contents of it (which it doesn't\nhappen to do).  Storing the returned value to a variable of type\n\"const char *\" does not absolve it from the responsibility to free\nit (hence your patch).\n"}]}