{"thread":{"id":"41877","subject":"[PATCHv2 2/4] abbrev_sha1_in_line: don't leak memory","startedAt":"2016-03-30T17:05:14Z","lastAt":"2016-04-01T07:26:18Z","messageCount":20,"participants":["Stefan Beller","Jeff King","Eric Sunshine","Junio C Hamano","Philip Oakley"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"282239","messageId":"1459357518-14913-1-git-send-email-sbeller@google.com","threadId":"41877","inReplyTo":null,"subject":"[PATCHv2 0/4] Some cleanups","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-30T17:05:14Z","receivedAt":"2016-03-30T17:05:14Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v2:\nThanks Eric, Jeff, Junio for discussion, I picked up all hints and here\nis a v2.\n\n* drop the overallocation patches (1&2)\n* use git_config_get_string instead of its _const equivalent, such that\n we don't need a cast when freeing in git_config_get_notes_strategy\n* Use strbuf_list_free instead of cooking our own.\n* have a dedicated error exit path in bundle.c, create_bundle\n\nv1:\nOne of my first patches to Git were cleanup patches, and I fell back\nto my old pattern here, while thinking on how to write better commit\nmessages for the submodule bugfixes I currently have in flight.\n\nJust some one liners to not leak memory or file descriptors.\n\nThey are bundled as a series, but no patch relies on any predessor.\n\nThis applies on v2.8.0.\n\nThanks,\nStefan\n\nStefan Beller (4):\n notes: don't leak memory in git_config_get_notes_strategy\n abbrev_sha1_in_line: don't leak memory\n bundle: don't leak an fd in case of early return\n credential-cache, send_request: close fd when done\n\n builtin/notes.c | 5 +++--\n bundle.c | 23 +++++++++++++++++------\n credential-cache.c | 1 +\n wt-status.c | 4 +---\n 4 files changed, 22 insertions(+), 11 deletions(-)\n\n-- \n2.8.0.2.gb331331\n"},{"id":"282240","messageId":"1459357518-14913-2-git-send-email-sbeller@google.com","threadId":"41877","inReplyTo":"1459357518-14913-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 1/4] notes: don't leak memory in git_config_get_notes_strategy","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-30T17:05:15Z","receivedAt":"2016-03-30T17:05:15Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"`value` is just a temporary scratchpad, so we need to make sure it doesn't\nleak. It is xstrdup'd in `git_config_get_string` and\n`parse_notes_merge_strategy` just compares the string against predefined\nvalues, so no need to keep it around longer. Make `value` non-const to\navoid the cast in the free.\n\nSigned-off-by: Stefan Beller \n---\n builtin/notes.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex ed6f222..6fd058d 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -744,13 +744,14 @@ static int merge_commit(struct notes_merge_options *o)\n static int git_config_get_notes_strategy(const char *key,\n \t\t\t\t\t enum notes_merge_strategy *strategy)\n {\n-\tconst char *value;\n+\tchar *value;\n \n-\tif (git_config_get_string_const(key, &value))\n+\tif (git_config_get_string(key, &value))\n \t\treturn 1;\n \tif (parse_notes_merge_strategy(value, strategy))\n \t\tgit_die_config(key, \"unknown notes merge strategy %s\", value);\n \n+\tfree(value);\n \treturn 0;\n }\n \n-- \n2.8.0.2.gb331331\n"},{"id":"282236","messageId":"1459357518-14913-3-git-send-email-sbeller@google.com","threadId":"41877","inReplyTo":"1459357518-14913-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 2/4] abbrev_sha1_in_line: don't leak memory","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-30T17:05:16Z","receivedAt":"2016-03-30T17:05:16Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"`split` is of type `struct strbuf **`, which we have a dedicated free\nfunction for, which takes care of freeing all related memory.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Stefan Beller \n---\n wt-status.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex ef74864..1ea2ebe 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1063,9 +1063,7 @@ static void abbrev_sha1_in_line(struct strbuf *line)\n \t\t\t\tstrbuf_addf(line, \"%s\", split[i]->buf);\n \t\t}\n \t}\n-\tfor (i = 0; split[i]; i++)\n-\t\tstrbuf_release(split[i]);\n-\n+\tstrbuf_list_free(split);\n }\n \n static void read_rebase_todolist(const char *fname, struct string_list *lines)\n-- \n2.8.0.2.gb331331\n"},{"id":"282237","messageId":"1459357518-14913-4-git-send-email-sbeller@google.com","threadId":"41877","inReplyTo":"1459357518-14913-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 3/4] bundle: don't leak an fd in case of early return","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-30T17:05:17Z","receivedAt":"2016-03-30T17:05:17Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In successful operation `write_pack_data` will close the `bundle_fd`,\nbut when we exit early, we need to take care of the file descriptor\nas well as the lock file ourselves. The lock file may be deleted at the\nend of running the program, but we are in library code, so we should\nnot rely on that.\n\nHelped-by: Jeff King \nSigned-off-by: Stefan Beller \n---\n bundle.c | 23 +++++++++++++++++------\n 1 file changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 506ac49..fbc8593 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -407,6 +407,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n \tint bundle_to_stdout;\n \tint ref_count = 0;\n \tstruct rev_info revs;\n+\tint ret = -1;\n \n \tbundle_to_stdout = !strcmp(path, \"-\");\n \tif (bundle_to_stdout)\n@@ -435,30 +436,40 @@ int create_bundle(struct bundle_header *header, const char *path,\n \n \t/* write prerequisites */\n \tif (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv))\n-\t\treturn -1;\n+\t\tgoto err;\n \n \targc = setup_revisions(argc, argv, &revs, NULL);\n \n-\tif (argc > 1)\n-\t\treturn error(_(\"unrecognized argument: %s\"), argv[1]);\n+\tif (argc > 1) {\n+\t\tret = error(_(\"unrecognized argument: %s\"), argv[1]);\n+\t\tgoto err;\n+\t}\n \n \tobject_array_remove_duplicates(&revs.pending);\n \n \tref_count = write_bundle_refs(bundle_fd, &revs);\n \tif (!ref_count)\n \t\tdie(_(\"Refusing to create empty bundle.\"));\n-\telse if (ref_count < 0)\n-\t\treturn -1;\n+\telse if (ref_count < 0) {\n+\t\tif (!bundle_to_stdout)\n+\t\t\tclose(bundle_fd);\n+\t\tgoto err;\n+\t}\n \n \t/* write pack */\n \tif (write_pack_data(bundle_fd, &revs))\n-\t\treturn -1;\n+\t\tgoto err;\n \n \tif (!bundle_to_stdout) {\n \t\tif (commit_lock_file(&lock))\n \t\t\tdie_errno(_(\"cannot create '%s'\"), path);\n \t}\n \treturn 0;\n+err:\n+\tif (!bundle_to_stdout)\n+\t\tclose(bundle_fd);\n+\trollback_lock_file(&lock);\n+\treturn ret;\n }\n \n int unbundle(struct bundle_header *header, int bundle_fd, int flags)\n-- \n2.8.0.2.gb331331\n"},{"id":"282238","messageId":"1459357518-14913-5-git-send-email-sbeller@google.com","threadId":"41877","inReplyTo":"1459357518-14913-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 4/4] credential-cache, send_request: close fd when done","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-30T17:05:18Z","receivedAt":"2016-03-30T17:05:18Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"No need to keep it open any further.\n\nSigned-off-by: Stefan Beller \n---\n credential-cache.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/credential-cache.c b/credential-cache.c\nindex f4afdc6..86e21de 100644\n--- a/credential-cache.c\n+++ b/credential-cache.c\n@@ -32,6 +32,7 @@ static int send_request(const char *socket, const struct strbuf *out)\n \t\twrite_or_die(1, in, r);\n \t\tgot_data = 1;\n \t}\n+\tclose(fd);\n \treturn got_data;\n }\n \n-- \n2.8.0.2.gb331331\n"},{"id":"282247","messageId":"20160330172354.GB19675@sigill.intra.peff.net","threadId":"41877","inReplyTo":"1459357518-14913-4-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 3/4] bundle: don't leak an fd in case of early return","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-30T17:23:54Z","receivedAt":"2016-03-30T17:23:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 30, 2016 at 10:05:17AM -0700, Stefan Beller wrote:\n\n> diff --git a/bundle.c b/bundle.c\n> index 506ac49..fbc8593 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -407,6 +407,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n> \tint bundle_to_stdout;\n> \tint ref_count = 0;\n> \tstruct rev_info revs;\n> +\tint ret = -1;\n\nA minor nit, but I don't think we ever put anything but \"-1\" in this\nvariable. It could go away and we can just \"return -1\" in the \"err\"\npath.\n\n-Peff\n"},{"id":"282249","messageId":"20160330172538.GC19675@sigill.intra.peff.net","threadId":"41877","inReplyTo":"1459357518-14913-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 0/4] Some cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-30T17:25:38Z","receivedAt":"2016-03-30T17:25:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 30, 2016 at 10:05:14AM -0700, Stefan Beller wrote:\n\n> v2:\n> Thanks Eric, Jeff, Junio for discussion, I picked up all hints and here\n> is a v2.\n> \n> * drop the overallocation patches (1&2)\n> * use git_config_get_string instead of its _const equivalent, such that\n> we don't need a cast when freeing in git_config_get_notes_strategy\n> * Use strbuf_list_free instead of cooking our own.\n> * have a dedicated error exit path in bundle.c, create_bundle\n\nI'm OK with all of these as-is, though I did mention a nit in the third\none. I also like Junio's rewrite instead of using strbuf_list_free.\n\n-Peff\n"},{"id":"282250","messageId":"CAGZ79kYwpj+NSn4yZK0tqASQ7CghAMgE6L9thVhTwuBNmFY6PA@mail.gmail.com","threadId":"41877","inReplyTo":"20160330172538.GC19675@sigill.intra.peff.net","subject":"Re: [PATCHv2 0/4] Some cleanups","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-30T17:32:40Z","receivedAt":"2016-03-30T17:32:40Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Mar 30, 2016 at 10:25 AM, Jeff King wrote:\n> On Wed, Mar 30, 2016 at 10:05:14AM -0700, Stefan Beller wrote:\n>\n>> v2:\n>> Thanks Eric, Jeff, Junio for discussion, I picked up all hints and here\n>> is a v2.\n>>\n>> * drop the overallocation patches (1&2)\n>> * use git_config_get_string instead of its _const equivalent, such that\n>> we don't need a cast when freeing in git_config_get_notes_strategy\n>> * Use strbuf_list_free instead of cooking our own.\n>> * have a dedicated error exit path in bundle.c, create_bundle\n>\n> I'm OK with all of these as-is, though I did mention a nit in the third\n> one. I also like Junio's rewrite instead of using strbuf_list_free.\n\nI'm fine using the rewritten version instead of using strbuf_list_free. :)\nOn the third one, there is one case, where we have\n\n if (..)\n return error(_(text));\n\nand that is an exit(128); eventually.\n\nI thought it is worth preserving the difference (as it is a faithful\nbug fix not a\nchange to make it better or more uniform).\n\nThanks,\nStefan\n\n\n\n>\n> -Peff\n"},{"id":"282251","messageId":"CAPig+cTeVgyNHa5uHUMDZ6ifxyjk2wvm=jGd9tP1+BuBKKVR+g@mail.gmail.com","threadId":"41877","inReplyTo":"1459357518-14913-2-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 1/4] notes: don't leak memory in git_config_get_notes_strategy","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-30T17:32:46Z","receivedAt":"2016-03-30T17:32:46Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 30, 2016 at 1:05 PM, Stefan Beller wrote:\n> `value` is just a temporary scratchpad, so we need to make sure it doesn't\n> leak. It is xstrdup'd in `git_config_get_string` and\n> `parse_notes_merge_strategy` just compares the string against predefined\n> values, so no need to keep it around longer. Make `value` non-const to\n> avoid the cast in the free.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> @@ -744,13 +744,14 @@ static int merge_commit(struct notes_merge_options *o)\n> static int git_config_get_notes_strategy(const char *key,\n> enum notes_merge_strategy *strategy)\n> {\n> - const char *value;\n> + char *value;\n>\n> - if (git_config_get_string_const(key, &value))\n> + if (git_config_get_string(key, &value))\n> return 1;\n> if (parse_notes_merge_strategy(value, strategy))\n> git_die_config(key, \"unknown notes merge strategy %s\", value);\n>\n> + free(value);\n> return 0;\n> }\n\nHmm, I thought Peff's suggestion of using git_config_get_value() was\naccepted as superior since it avoids the allocation altogether, thus\nno need for free() and no leak.\n"},{"id":"282252","messageId":"CAPig+cQ92gY=A_mtToe=gb2jPBHTJKZ7DcjYL+E6t1QFbdYuWA@mail.gmail.com","threadId":"41877","inReplyTo":"1459357518-14913-3-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 2/4] abbrev_sha1_in_line: don't leak memory","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-30T17:35:50Z","receivedAt":"2016-03-30T17:35:50Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 30, 2016 at 1:05 PM, Stefan Beller wrote:\n> `split` is of type `struct strbuf **`, which we have a dedicated free\n> function for, which takes care of freeing all related memory.\n\nI think it's important to explain that 'split' and each split[]\nelement were being leaked (despite the existing strbuf_release()) as\njustification for why this change is beneficial.\n\n> Helped-by: Eric Sunshine \n> Signed-off-by: Stefan Beller \n> ---\n> wt-status.c | 4 +---\n> 1 file changed, 1 insertion(+), 3 deletions(-)\n>\n> diff --git a/wt-status.c b/wt-status.c\n> index ef74864..1ea2ebe 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -1063,9 +1063,7 @@ static void abbrev_sha1_in_line(struct strbuf *line)\n> strbuf_addf(line, \"%s\", split[i]->buf);\n> }\n> }\n> - for (i = 0; split[i]; i++)\n> - strbuf_release(split[i]);\n> -\n> + strbuf_list_free(split);\n> }\n"},{"id":"282253","messageId":"20160330173837.GA20706@sigill.intra.peff.net","threadId":"41877","inReplyTo":"CAGZ79kYwpj+NSn4yZK0tqASQ7CghAMgE6L9thVhTwuBNmFY6PA@mail.gmail.com","subject":"Re: [PATCHv2 0/4] Some cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-30T17:38:37Z","receivedAt":"2016-03-30T17:38:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 30, 2016 at 10:32:40AM -0700, Stefan Beller wrote:\n\n> > I'm OK with all of these as-is, though I did mention a nit in the third\n> > one. I also like Junio's rewrite instead of using strbuf_list_free.\n> \n> I'm fine using the rewritten version instead of using strbuf_list_free. :)\n> On the third one, there is one case, where we have\n> \n> if (..)\n> return error(_(text));\n> \n> and that is an exit(128); eventually.\n\nIn the caller perhaps, but isn't that equivalent to:\n\n error(_(text));\n return -1;\n\n?\n\nI think it is OK to make assumptions about error()'s return value; that\nis what it is there for.\n\n-Peff\n"},{"id":"282254","messageId":"CAPig+cSE5wVNNwiwkYoOXnfDKUO3tBKqUUbqBHPCTA5ibj-kNg@mail.gmail.com","threadId":"41877","inReplyTo":"1459357518-14913-4-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 3/4] bundle: don't leak an fd in case of early return","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-30T17:41:16Z","receivedAt":"2016-03-30T17:41:16Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 30, 2016 at 1:05 PM, Stefan Beller wrote:\n> In successful operation `write_pack_data` will close the `bundle_fd`,\n> but when we exit early, we need to take care of the file descriptor\n> as well as the lock file ourselves. The lock file may be deleted at the\n> end of running the program, but we are in library code, so we should\n> not rely on that.\n>\n> Helped-by: Jeff King \n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/bundle.c b/bundle.c\n> @@ -435,30 +436,40 @@ int create_bundle(struct bundle_header *header, const char *path,\n>\n> /* write prerequisites */\n> if (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv))\n> - return -1;\n> + goto err;\n>\n> argc = setup_revisions(argc, argv, &revs, NULL);\n>\n> - if (argc > 1)\n> - return error(_(\"unrecognized argument: %s\"), argv[1]);\n> + if (argc > 1) {\n> + ret = error(_(\"unrecognized argument: %s\"), argv[1]);\n> + goto err;\n> + }\n>\n> object_array_remove_duplicates(&revs.pending);\n>\n> ref_count = write_bundle_refs(bundle_fd, &revs);\n> if (!ref_count)\n> die(_(\"Refusing to create empty bundle.\"));\n> - else if (ref_count < 0)\n> - return -1;\n> + else if (ref_count < 0) {\n> + if (!bundle_to_stdout)\n> + close(bundle_fd);\n\nWhy is this close() here considering that it gets closed by the 'err' path?\n\n> + goto err;\n> + }\n>\n> /* write pack */\n> if (write_pack_data(bundle_fd, &revs))\n> - return -1;\n> + goto err;\n>\n> if (!bundle_to_stdout) {\n> if (commit_lock_file(&lock))\n> die_errno(_(\"cannot create '%s'\"), path);\n> }\n> return 0;\n> +err:\n> + if (!bundle_to_stdout)\n> + close(bundle_fd);\n> + rollback_lock_file(&lock);\n> + return ret;\n> }\n"},{"id":"282271","messageId":"xmqqegarsmil.fsf@gitster.mtv.corp.google.com","threadId":"41877","inReplyTo":"CAPig+cTeVgyNHa5uHUMDZ6ifxyjk2wvm=jGd9tP1+BuBKKVR+g@mail.gmail.com","subject":"Re: [PATCHv2 1/4] notes: don't leak memory in git_config_get_notes_strategy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-30T21:07:14Z","receivedAt":"2016-03-30T21:07:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> diff --git a/builtin/notes.c b/builtin/notes.c\n>> @@ -744,13 +744,14 @@ static int merge_commit(struct notes_merge_options *o)\n>> static int git_config_get_notes_strategy(const char *key,\n>> enum notes_merge_strategy *strategy)\n>> {\n>> - const char *value;\n>> + char *value;\n>>\n>> - if (git_config_get_string_const(key, &value))\n>> + if (git_config_get_string(key, &value))\n>> return 1;\n>> if (parse_notes_merge_strategy(value, strategy))\n>> git_die_config(key, \"unknown notes merge strategy %s\", value);\n>>\n>> + free(value);\n>> return 0;\n>> }\n>\n> Hmm, I thought Peff's suggestion of using git_config_get_value() was\n> accepted as superior since it avoids the allocation altogether, thus\n> no need for free() and no leak.\n\nI agree that this caller can avoid taking ownership of value by\nusing git_config_get_value() and that would be a cleaner solution\nhere.\n\nThis is a tangent, but am I the only one who finds that the naming\nof functions in config-get API is confusing? Just wondering if we\nshould rename the ones that keeps the memory ownership to the config\nsubsystem with s/get/peek/ or something.\n"},{"id":"282272","messageId":"CAGZ79kZSLH_eMt0v6unHHZi0SQBHXicH_EPxy++9XdAw6fUUqg@mail.gmail.com","threadId":"41877","inReplyTo":"xmqqegarsmil.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv2 1/4] notes: don't leak memory in git_config_get_notes_strategy","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-30T21:10:08Z","receivedAt":"2016-03-30T21:10:08Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Mar 30, 2016 at 2:07 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>\n>>> diff --git a/builtin/notes.c b/builtin/notes.c\n>>> @@ -744,13 +744,14 @@ static int merge_commit(struct notes_merge_options *o)\n>>> static int git_config_get_notes_strategy(const char *key,\n>>> enum notes_merge_strategy *strategy)\n>>> {\n>>> - const char *value;\n>>> + char *value;\n>>>\n>>> - if (git_config_get_string_const(key, &value))\n>>> + if (git_config_get_string(key, &value))\n>>> return 1;\n>>> if (parse_notes_merge_strategy(value, strategy))\n>>> git_die_config(key, \"unknown notes merge strategy %s\", value);\n>>>\n>>> + free(value);\n>>> return 0;\n>>> }\n>>\n>> Hmm, I thought Peff's suggestion of using git_config_get_value() was\n>> accepted as superior since it avoids the allocation altogether, thus\n>> no need for free() and no leak.\n>\n> I agree that this caller can avoid taking ownership of value by\n> using git_config_get_value() and that would be a cleaner solution\n> here.\n>\n> This is a tangent, but am I the only one who finds that the naming\n> of functions in config-get API is confusing? Just wondering if we\n> should rename the ones that keeps the memory ownership to the config\n> subsystem with s/get/peek/ or something.\n>\n\nI demonstrated the confusion and disability to read and distinguish\nbetween those names clearly already.\n\nSo I'd strongly favor a better naming for functions in config.c\n"},{"id":"282287","messageId":"20160331010604.GA23999@sigill.intra.peff.net","threadId":"41877","inReplyTo":"xmqqegarsmil.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv2 1/4] notes: don't leak memory in git_config_get_notes_strategy","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-31T01:06:04Z","receivedAt":"2016-03-31T01:06:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 30, 2016 at 02:07:14PM -0700, Junio C Hamano wrote:\n\n> This is a tangent, but am I the only one who finds that the naming\n> of functions in config-get API is confusing? Just wondering if we\n> should rename the ones that keeps the memory ownership to the config\n> subsystem with s/get/peek/ or something.\n\nYou are definitely not the only one.\n\nI think the get/peek thing would help, but I also get confused between\ngit_config_string() and git_config_get_string().\n\n-Peff\n"},{"id":"282293","messageId":"xmqqegarqrnu.fsf@gitster.mtv.corp.google.com","threadId":"41877","inReplyTo":"20160331010604.GA23999@sigill.intra.peff.net","subject":"Re: [PATCHv2 1/4] notes: don't leak memory in git_config_get_notes_strategy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-31T02:59:01Z","receivedAt":"2016-03-31T02:59:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Mar 30, 2016 at 02:07:14PM -0700, Junio C Hamano wrote:\n>\n>> This is a tangent, but am I the only one who finds that the naming\n>> of functions in config-get API is confusing? Just wondering if we\n>> should rename the ones that keeps the memory ownership to the config\n>> subsystem with s/get/peek/ or something.\n>\n> You are definitely not the only one.\n>\n> I think the get/peek thing would help, but I also get confused between\n> git_config_string() and git_config_get_string().\n\nI actually do not have problems with the ones with names that do not\nhave \"get\" in.\n\ngit_config_$type() is always a helper function that are designed to\nbe called by config callback to parse (and complain) a \npair that is expected to be of $type, and git_$family_config() is\nalways a helper function that are designed to be used as the\ncallback of git_config() to handle the configuration related to the\nnamed command family.\n"},{"id":"282355","messageId":"CAGZ79kYFr8bjEhV9sDzR1KbqAVrO89-DRBN2u+zQBwVYViDP9w@mail.gmail.com","threadId":"41877","inReplyTo":"CAPig+cSE5wVNNwiwkYoOXnfDKUO3tBKqUUbqBHPCTA5ibj-kNg@mail.gmail.com","subject":"Re: [PATCHv2 3/4] bundle: don't leak an fd in case of early return","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T17:47:43Z","receivedAt":"2016-03-31T17:47:43Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Mar 30, 2016 at 10:41 AM, Eric Sunshine wrote:\n>> - else if (ref_count < 0)\n>> - return -1;\n>> + else if (ref_count < 0) {\n>> + if (!bundle_to_stdout)\n>> + close(bundle_fd);\n>\n> Why is this close() here considering that it gets closed by the 'err' path?\n\nThanks for pointing out; fixed in a reroll.\n"},{"id":"282371","messageId":"9C112693D94545DC917C90299E52A719@PhilipOakley","threadId":"41877","inReplyTo":"1459357518-14913-4-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 3/4] bundle: don't leak an fd in case of early return","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2016-03-31T19:00:42Z","receivedAt":"2016-03-31T19:00:42Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Stefan Beller\" \n> In successful operation `write_pack_data` will close the `bundle_fd`,\n> but when we exit early, we need to take care of the file descriptor\n> as well as the lock file ourselves. The lock file may be deleted at the\n> end of running the program, but we are in library code, so we should\n> not rely on that.\n>\n> Helped-by: Jeff King \n> Signed-off-by: Stefan Beller \n\nHas this been tested on Windows? I had a similar problem very recently\nhttps://groups.google.com/forum/#!msg/git-for-windows/6LPxf9xZKhI/-s7XD18yCwAJ\nwhere a bad rev-list-arg would cause the `bundle create` to die, and, on \nwindows, leave the incomplete bundle file locked.\n\ndscho suggested one possible solution for that, but I haven't had any time \nto try any patches.\n\n> ---\n> bundle.c | 23 +++++++++++++++++------\n> 1 file changed, 17 insertions(+), 6 deletions(-)\n>\n> diff --git a/bundle.c b/bundle.c\n> index 506ac49..fbc8593 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -407,6 +407,7 @@ int create_bundle(struct bundle_header *header, const \n> char *path,\n> int bundle_to_stdout;\n> int ref_count = 0;\n> struct rev_info revs;\n> + int ret = -1;\n>\n> bundle_to_stdout = !strcmp(path, \"-\");\n> if (bundle_to_stdout)\n> @@ -435,30 +436,40 @@ int create_bundle(struct bundle_header *header, \n> const char *path,\n>\n> /* write prerequisites */\n> if (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv))\n> - return -1;\n> + goto err;\n>\n> argc = setup_revisions(argc, argv, &revs, NULL);\n>\n> - if (argc > 1)\n> - return error(_(\"unrecognized argument: %s\"), argv[1]);\n> + if (argc > 1) {\n> + ret = error(_(\"unrecognized argument: %s\"), argv[1]);\n> + goto err;\n> + }\n>\n> object_array_remove_duplicates(&revs.pending);\n>\n> ref_count = write_bundle_refs(bundle_fd, &revs);\n> if (!ref_count)\n> die(_(\"Refusing to create empty bundle.\"));\n> - else if (ref_count < 0)\n> - return -1;\n> + else if (ref_count < 0) {\n> + if (!bundle_to_stdout)\n> + close(bundle_fd);\n> + goto err;\n> + }\n>\n> /* write pack */\n> if (write_pack_data(bundle_fd, &revs))\n> - return -1;\n> + goto err;\n>\n> if (!bundle_to_stdout) {\n> if (commit_lock_file(&lock))\n> die_errno(_(\"cannot create '%s'\"), path);\n> }\n> return 0;\n> +err:\n> + if (!bundle_to_stdout)\n> + close(bundle_fd);\n> + rollback_lock_file(&lock);\n> + return ret;\n> }\n>\n> int unbundle(struct bundle_header *header, int bundle_fd, int flags)\n> -- \n> 2.8.0.2.gb331331\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"282417","messageId":"CAGZ79kYMxJdh_8n8zaaaEQP_BqWfS12JEry1MHkYqwQPJTWTNw@mail.gmail.com","threadId":"41877","inReplyTo":"9C112693D94545DC917C90299E52A719@PhilipOakley","subject":"Re: [PATCHv2 3/4] bundle: don't leak an fd in case of early return","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-01T00:25:45Z","receivedAt":"2016-04-01T00:25:45Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Mar 31, 2016 at 12:00 PM, Philip Oakley wrote:\n> From: \"Stefan Beller\" \n>>\n>> In successful operation `write_pack_data` will close the `bundle_fd`,\n>> but when we exit early, we need to take care of the file descriptor\n>> as well as the lock file ourselves. The lock file may be deleted at the\n>> end of running the program, but we are in library code, so we should\n>> not rely on that.\n>>\n>> Helped-by: Jeff King \n>> Signed-off-by: Stefan Beller \n>\n>\n> Has this been tested on Windows? I had a similar problem very recently\n> https://groups.google.com/forum/#!msg/git-for-windows/6LPxf9xZKhI/-s7XD18yCwAJ\n> where a bad rev-list-arg would cause the `bundle create` to die, and, on\n> windows, leave the incomplete bundle file locked.\n>\n> dscho suggested one possible solution for that, but I haven't had any time\n> to try any patches.\n\nI think with Jeffs suggestion to only rollback the lock in case of\n(!bundle_to_stdout)\nwe're not making it worse. I do not have a Windows machine at hand to test it :(\n"},{"id":"282434","messageId":"55AD693FD6AC44119B8368ABC36FB1A9@PhilipOakley","threadId":"41877","inReplyTo":"CAGZ79kYMxJdh_8n8zaaaEQP_BqWfS12JEry1MHkYqwQPJTWTNw@mail.gmail.com","subject":"Re: [PATCHv2 3/4] bundle: don't leak an fd in case of early return","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2016-04-01T07:26:18Z","receivedAt":"2016-04-01T07:26:18Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Stefan Beller\" \n> On Thu, Mar 31, 2016 at 12:00 PM, Philip Oakley \n> wrote:\n>> From: \"Stefan Beller\" \n>>>\n>>> In successful operation `write_pack_data` will close the `bundle_fd`,\n>>> but when we exit early, we need to take care of the file descriptor\n>>> as well as the lock file ourselves. The lock file may be deleted at the\n>>> end of running the program, but we are in library code, so we should\n>>> not rely on that.\n>>>\n>>> Helped-by: Jeff King \n>>> Signed-off-by: Stefan Beller \n>>\n>>\n>> Has this been tested on Windows? I had a similar problem very recently\n>> https://groups.google.com/forum/#!msg/git-for-windows/6LPxf9xZKhI/-s7XD18yCwAJ\n>> where a bad rev-list-arg would cause the `bundle create` to die, and, on\n>> windows, leave the incomplete bundle file locked.\n>>\n>> dscho suggested one possible solution for that, but I haven't had any \n>> time\n>> to try any patches.\n>\n> I think with Jeffs suggestion to only rollback the lock in case of\n> (!bundle_to_stdout)\n> we're not making it worse. I do not have a Windows machine at hand to test \n> it :(\n\nThant's fine. I just wanted to make folks aware of the problem so it doesn't \nget worse.\n\nIt looks like the cause is that the \nbundle.c:compute_and_write_prerequisites(), which includes parsing the \narg-list (and die on bad arg), is called after the creation and locking of \nthe bundle file descriptor and header, thus leaving it hanging on Windows.\n\n--\n\nPhilip \n"}]}