{"thread":{"id":"38766","subject":"[PATCH 2/3] bundle.c: fix memory leak","startedAt":"2015-03-09T16:58:22Z","lastAt":"2015-03-11T03:57:30Z","messageCount":13,"participants":["Stefan Beller","Torsten Bögershausen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"257413","messageId":"1425920304-22360-1-git-send-email-sbeller@google.com","threadId":"38766","inReplyTo":null,"subject":"[PATCH 1/3] connect.c: Fix memory leak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-09T16:58:22Z","receivedAt":"2015-03-09T16:58:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller <sbeller@google.com>\n---\n connect.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/connect.c b/connect.c\nindex ce0e121..6090211 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -739,6 +739,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \n \t\t\t\tfree(hostandport);\n \t\t\t\tfree(path);\n+\t\t\t\tfree(conn);\n \t\t\t\treturn NULL;\n \t\t\t} else {\n \t\t\t\tssh = getenv(\"GIT_SSH_COMMAND\");\n-- \n2.3.0.81.gc37f363\n"},{"id":"257412","messageId":"1425920304-22360-2-git-send-email-sbeller@google.com","threadId":"38766","inReplyTo":"1425920304-22360-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/3] bundle.c: fix memory leak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-09T16:58:23Z","receivedAt":"2015-03-09T16:58:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The continue statements nearby also have an accompanying free(ref);\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n bundle.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/bundle.c b/bundle.c\nindex 2e2dbd5..534783d 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -342,6 +342,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)\n \t\tif (e->item->type == OBJ_TAG &&\n \t\t\t\t!is_tag_in_date_range(e->item, revs)) {\n \t\t\te->item->flags |= UNINTERESTING;\n+\t\t\tfree(ref);\n \t\t\tcontinue;\n \t\t}\n \n-- \n2.3.0.81.gc37f363\n"},{"id":"257414","messageId":"1425920304-22360-3-git-send-email-sbeller@google.com","threadId":"38766","inReplyTo":"1425920304-22360-1-git-send-email-sbeller@google.com","subject":"[PATCH 3/3] builtin/help.c: fix memory leak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-09T16:58:24Z","receivedAt":"2015-03-09T16:58:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/help.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/help.c b/builtin/help.c\nindex 6133fe4..a1f5a0a 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -456,7 +456,7 @@ static void list_common_guides_help(void)\n int cmd_help(int argc, const char **argv, const char *prefix)\n {\n \tint nongit;\n-\tconst char *alias;\n+\tchar *alias;\n \tenum help_format parsed_help_format;\n \n \targc = parse_options(argc, argv, prefix, builtin_help_options,\n@@ -499,6 +499,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n \talias = alias_lookup(argv[0]);\n \tif (alias && !is_git_command(argv[0])) {\n \t\tprintf_ln(_(\"`git %s' is aliased to `%s'\"), argv[0], alias);\n+\t\tfree(alias);\n \t\treturn 0;\n \t}\n \n-- \n2.3.0.81.gc37f363\n"},{"id":"257423","messageId":"54FDF34F.60608@web.de","threadId":"38766","inReplyTo":"1425920304-22360-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/3] connect.c: Fix memory leak","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-03-09T19:23:59Z","receivedAt":"2015-03-09T19:23:59Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2015-03-09 17.58, Stefan Beller wrote:\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  connect.c | 1 +\n>  1 file changed, 1 insertion(+)\n> \n> diff --git a/connect.c b/connect.c\n> index ce0e121..6090211 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -739,6 +739,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n>  \n>  \t\t\t\tfree(hostandport);\n>  \t\t\t\tfree(path);\n> +\t\t\t\tfree(conn);\n>  \t\t\t\treturn NULL;\n>  \t\t\t} else {\n>  \t\t\t\tssh = getenv(\"GIT_SSH_COMMAND\");\n> \nNice catch, thanks.\n(Even if it is not sooo serious today, the current implementation of Git just dies\nwhen connection is NULL)\n"},{"id":"257507","messageId":"xmqq61a8zdix.fsf@gitster.dls.corp.google.com","threadId":"38766","inReplyTo":"1425920304-22360-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/3] connect.c: Fix memory leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-10T22:35:50Z","receivedAt":"2015-03-10T22:35:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  connect.c | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/connect.c b/connect.c\n> index ce0e121..6090211 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -739,6 +739,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n>  \n>  \t\t\t\tfree(hostandport);\n>  \t\t\t\tfree(path);\n> +\t\t\t\tfree(conn);\n>  \t\t\t\treturn NULL;\n\nThe lack of sufficient context in the patch made me wonder if merely\nfreeing conn is sufficient or do we need some process related\nclean-up before freeing it.\n\nIt appears that this codepath is used only when debugging with\nCONNECT_DIAG_URL and we didn't start any process --- we only\ncleared it with a few arrays initialized to empty at this point of\nthe code.\n\nSo it should be safe.  I wish I didn't have to do the digging\nmyself, though ;-)\n\n>  \t\t\t} else {\n>  \t\t\t\tssh = getenv(\"GIT_SSH_COMMAND\");\n"},{"id":"257508","messageId":"xmqq1tkwzdbj.fsf@gitster.dls.corp.google.com","threadId":"38766","inReplyTo":"1425920304-22360-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/3] bundle.c: fix memory leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-10T22:40:16Z","receivedAt":"2015-03-10T22:40:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> The continue statements nearby also have an accompanying free(ref);\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n\nI wonder what happens when dwim_ref() returned 2 or more, though.\n\n>  bundle.c | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/bundle.c b/bundle.c\n> index 2e2dbd5..534783d 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -342,6 +342,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)\n>  \t\tif (e->item->type == OBJ_TAG &&\n>  \t\t\t\t!is_tag_in_date_range(e->item, revs)) {\n>  \t\t\te->item->flags |= UNINTERESTING;\n> +\t\t\tfree(ref);\n>  \t\t\tcontinue;\n>  \t\t}\n"},{"id":"257509","messageId":"xmqqwq2oxyli.fsf@gitster.dls.corp.google.com","threadId":"38766","inReplyTo":"1425920304-22360-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 3/3] builtin/help.c: fix memory leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-10T22:43:37Z","receivedAt":"2015-03-10T22:43:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  builtin/help.c | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/help.c b/builtin/help.c\n> index 6133fe4..a1f5a0a 100644\n> --- a/builtin/help.c\n> +++ b/builtin/help.c\n> @@ -456,7 +456,7 @@ static void list_common_guides_help(void)\n>  int cmd_help(int argc, const char **argv, const char *prefix)\n>  {\n>  \tint nongit;\n> -\tconst char *alias;\n> +\tchar *alias;\n>  \tenum help_format parsed_help_format;\n>  \n>  \targc = parse_options(argc, argv, prefix, builtin_help_options,\n> @@ -499,6 +499,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n>  \talias = alias_lookup(argv[0]);\n>  \tif (alias && !is_git_command(argv[0])) {\n>  \t\tprintf_ln(_(\"`git %s' is aliased to `%s'\"), argv[0], alias);\n> +\t\tfree(alias);\n\nHmph, does this memory belong to us, or are we peeking into the\ncached data in the config cache layer?\n\n\n>  \t\treturn 0;\n>  \t}\n"},{"id":"257513","messageId":"CAGZ79kZ5=Q3WE6Ztr7PJ2Kv2xdKdCB2cD+_MH1GcqEDiZpU11w@mail.gmail.com","threadId":"38766","inReplyTo":"xmqq1tkwzdbj.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] bundle.c: fix memory leak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-10T23:08:32Z","receivedAt":"2015-03-10T23:08:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Mar 10, 2015 at 3:40 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> The continue statements nearby also have an accompanying free(ref);\n>>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>\n> I wonder what happens when dwim_ref() returned 2 or more, though.\n\nThat should also be fixed I guess. I'll look into it.\n\nThese one liner fixes are mostly done as a side project\nhaving fun, just doing what the code analysis tools says,\nsorry for not catching that one.\n\nMaybe instead of the reoccuring pattern\n\n    free(ref);\n    continue;\n\nwe could just have a\n\n    goto cleanup\n\nwhich goes to the end of the loop where we have\nthe free anyway.\n"},{"id":"257515","messageId":"xmqqk2yoxx1u.fsf@gitster.dls.corp.google.com","threadId":"38766","inReplyTo":"CAGZ79kZ5=Q3WE6Ztr7PJ2Kv2xdKdCB2cD+_MH1GcqEDiZpU11w@mail.gmail.com","subject":"Re: [PATCH 2/3] bundle.c: fix memory leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-10T23:17:01Z","receivedAt":"2015-03-10T23:17:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Tue, Mar 10, 2015 at 3:40 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>> The continue statements nearby also have an accompanying free(ref);\n>>>\n>>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>>> ---\n>>\n>> I wonder what happens when dwim_ref() returned 2 or more, though.\n>\n> That should also be fixed I guess. I'll look into it.\n>\n> These one liner fixes are mostly done as a side project\n> having fun, just doing what the code analysis tools says,\n> sorry for not catching that one.\n>\n> Maybe instead of the reoccuring pattern\n>\n>     free(ref);\n>     continue;\n>\n> we could just have a\n>\n>     goto cleanup\n>\n> which goes to the end of the loop where we have\n> the free anyway.\n\nYeah, I suspect that the end result may look more like that if you\ncover the dwim_ref() one as well.\n\nThanks.\n"},{"id":"257520","messageId":"CAGZ79kYSBsfbR_gH1Ur12vMpcK5S0K6wrOpMhNVvgffAb683Bw@mail.gmail.com","threadId":"38766","inReplyTo":"xmqqwq2oxyli.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/3] builtin/help.c: fix memory leak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-10T23:50:33Z","receivedAt":"2015-03-10T23:50:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Mar 10, 2015 at 3:43 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>>  builtin/help.c | 3 ++-\n>>  1 file changed, 2 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/help.c b/builtin/help.c\n>> index 6133fe4..a1f5a0a 100644\n>> --- a/builtin/help.c\n>> +++ b/builtin/help.c\n>> @@ -456,7 +456,7 @@ static void list_common_guides_help(void)\n>>  int cmd_help(int argc, const char **argv, const char *prefix)\n>>  {\n>>       int nongit;\n>> -     const char *alias;\n>> +     char *alias;\n>>       enum help_format parsed_help_format;\n>>\n>>       argc = parse_options(argc, argv, prefix, builtin_help_options,\n>> @@ -499,6 +499,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n>>       alias = alias_lookup(argv[0]);\n>>       if (alias && !is_git_command(argv[0])) {\n>>               printf_ln(_(\"`git %s' is aliased to `%s'\"), argv[0], alias);\n>> +             free(alias);\n>\n> Hmph, does this memory belong to us, or are we peeking into the\n> cached data in the config cache layer?\n\nSo  alias_lookup(..) is a wrapper around git_config_string essentially\n(some more git_config_functions are involved, but we eventually reach\ngit_config_string), where we have\n\n    *dest = xstrdup(value);\n\nso I think we need to free that memory, as the config cache layer doesn't\ncare any more.\n"},{"id":"257521","messageId":"1426031508-7575-1-git-send-email-sbeller@google.com","threadId":"38766","inReplyTo":"xmqqk2yoxx1u.fsf@gitster.dls.corp.google.com","subject":"[PATCH 2/3] bundle.c: fix memory leak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-10T23:51:48Z","receivedAt":"2015-03-10T23:51:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"There was one continue statement without an accompanying `free(ref)`.\nInstead of adding that, replace all the free&&continue with a goto\njust after writing the refs, where we'd do the free anyway and then\nreloop.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n bundle.c | 11 +++++------\n 1 file changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 2e2dbd5..f732c92 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -334,7 +334,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)\n \t\tif (e->item->flags & UNINTERESTING)\n \t\t\tcontinue;\n \t\tif (dwim_ref(e->name, strlen(e->name), sha1, &ref) != 1)\n-\t\t\tcontinue;\n+\t\t\tgoto skip_write_ref;\n \t\tif (read_ref_full(e->name, RESOLVE_REF_READING, sha1, &flag))\n \t\t\tflag = 0;\n \t\tdisplay_ref = (flag & REF_ISSYMREF) ? e->name : ref;\n@@ -342,7 +342,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)\n \t\tif (e->item->type == OBJ_TAG &&\n \t\t\t\t!is_tag_in_date_range(e->item, revs)) {\n \t\t\te->item->flags |= UNINTERESTING;\n-\t\t\tcontinue;\n+\t\t\tgoto skip_write_ref;\n \t\t}\n \n \t\t/*\n@@ -357,8 +357,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)\n \t\tif (!(e->item->flags & SHOWN) && e->item->type == OBJ_COMMIT) {\n \t\t\twarning(_(\"ref '%s' is excluded by the rev-list options\"),\n \t\t\t\te->name);\n-\t\t\tfree(ref);\n-\t\t\tcontinue;\n+\t\t\tgoto skip_write_ref;\n \t\t}\n \t\t/*\n \t\t * If you run \"git bundle create bndl v1.0..v2.0\", the\n@@ -388,8 +387,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)\n \t\t\t\tobj->flags |= SHOWN;\n \t\t\t\tadd_pending_object(revs, obj, e->name);\n \t\t\t}\n-\t\t\tfree(ref);\n-\t\t\tcontinue;\n+\t\t\tgoto skip_write_ref;\n \t\t}\n \n \t\tref_count++;\n@@ -397,6 +395,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)\n \t\twrite_or_die(bundle_fd, \" \", 1);\n \t\twrite_or_die(bundle_fd, display_ref, strlen(display_ref));\n \t\twrite_or_die(bundle_fd, \"\\n\", 1);\n+ skip_write_ref:\n \t\tfree(ref);\n \t}\n \n-- \n2.3.0.81.gc37f363\n"},{"id":"257529","messageId":"xmqqioe8ur50.fsf@gitster.dls.corp.google.com","threadId":"38766","inReplyTo":"CAGZ79kYSBsfbR_gH1Ur12vMpcK5S0K6wrOpMhNVvgffAb683Bw@mail.gmail.com","subject":"Re: [PATCH 3/3] builtin/help.c: fix memory leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-11T03:52:59Z","receivedAt":"2015-03-11T03:52:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>> Hmph, does this memory belong to us, or are we peeking into the\n>> cached data in the config cache layer?\n>\n> So  alias_lookup(..) is a wrapper around git_config_string essentially\n> (some more git_config_functions are involved, but we eventually reach\n> git_config_string), where we have\n>\n>     *dest = xstrdup(value);\n>\n> so I think we need to free that memory, as the config cache layer doesn't\n> care any more.\n\nAh, OK, thanks.\n"},{"id":"257528","messageId":"xmqqegowuqxh.fsf@gitster.dls.corp.google.com","threadId":"38766","inReplyTo":"1426031508-7575-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/3] bundle.c: fix memory leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-11T03:57:30Z","receivedAt":"2015-03-11T03:57:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> There was one continue statement without an accompanying `free(ref)`.\n> Instead of adding that, replace all the free&&continue with a goto\n> just after writing the refs, where we'd do the free anyway and then\n> reloop.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  bundle.c | 11 +++++------\n>  1 file changed, 5 insertions(+), 6 deletions(-)\n>\n> diff --git a/bundle.c b/bundle.c\n> index 2e2dbd5..f732c92 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -334,7 +334,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)\n>  \t\tif (e->item->flags & UNINTERESTING)\n>  \t\t\tcontinue;\n>  \t\tif (dwim_ref(e->name, strlen(e->name), sha1, &ref) != 1)\n> -\t\t\tcontinue;\n> +\t\t\tgoto skip_write_ref;\n\nThanks.\n\nI briefly wondered if we need to initialize ref=NULL for this to\nwork, because dwim_ref() does not need to clear ref when it says\n\"nothing matches\", but the function actually clears it upfront, so\nwe should be OK.\n\n> ...\n> @@ -397,6 +395,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)\n>  \t\twrite_or_die(bundle_fd, \" \", 1);\n>  \t\twrite_or_die(bundle_fd, display_ref, strlen(display_ref));\n>  \t\twrite_or_die(bundle_fd, \"\\n\", 1);\n> + skip_write_ref:\n>  \t\tfree(ref);\n>  \t}\n"}]}