{"thread":{"id":"41893","subject":"[PATCHv4 0/4] Some cleanups","startedAt":"2016-04-01T00:35:42Z","lastAt":"2016-04-01T17:31:09Z","messageCount":14,"participants":["Stefan Beller","Eric Sunshine","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"282419","messageId":"1459470946-16522-1-git-send-email-sbeller@google.com","threadId":"41893","inReplyTo":null,"subject":"[PATCHv4 0/4] Some cleanups","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-01T00:35:42Z","receivedAt":"2016-04-01T00:35:42Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Thanks for the reviews!\nThanks Jeff, Eric, Junio!\n\nThis replaces sb/misc-cleanups.\n\n* Revert the builtin/notes fix to the first version as git_config_get_value\n is dangerous, and the memory allocation and free is just a small overhead here\n* the bundle code integrates all of the suggestions (i.e. rollback_lock_file\n conditioned on (!bundle_to_stdout).\n \nI hope I got everything by now. I will head out for today and tomorrow I'll\nbe traveling to NY, where there is the Git Merge conference on Mon, Tues and some\nvacation afterwards. I'll be online in a very limited fashion. (I plan on taking\nmy phone only, no laptop), so in case I missed a thing this series will halt\nfor a while or someone else picks it up.\n\nThanks,\nStefan\n\ndiff to remotes/origin/sb/misc-cleanups (which doesn't contain the bundle fix):\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 386402d..afcfa8f 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -741,13 +741,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_value(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 \ndiff --git a/bundle.c b/bundle.c\nindex 506ac49..08e32ca 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -435,12 +435,14 @@ 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\terror(_(\"unrecognized argument: %s\"), argv[1]);\n+\t\tgoto err;\n+\t}\n \n \tobject_array_remove_duplicates(&revs.pending);\n \n@@ -448,17 +450,23 @@ int create_bundle(struct bundle_header *header, const char *path,\n \tif (!ref_count)\n \t\tdie(_(\"Refusing to create empty bundle.\"));\n \telse if (ref_count < 0)\n-\t\treturn -1;\n+\t\tgoto err;\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+\t\trollback_lock_file(&lock);\n+\t}\n+\treturn -1;\n }\n \n int unbundle(struct bundle_header *header, int bundle_fd, int flags)\n\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 | 18 +++++++++++++-----\n credential-cache.c | 1 +\n wt-status.c | 4 +---\n 4 files changed, 18 insertions(+), 10 deletions(-)\n\n-- \n2.5.0.264.gc776916.dirty\n"},{"id":"282420","messageId":"1459470946-16522-2-git-send-email-sbeller@google.com","threadId":"41893","inReplyTo":"1459470946-16522-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 1/4] notes: don't leak memory in git_config_get_notes_strategy","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-01T00:35:43Z","receivedAt":"2016-04-01T00:35:43Z","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_const` and\n`parse_notes_merge_strategy` just compares the string against predefined\nvalues, so no need to keep it around longer.\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 52aa9af..afcfa8f 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -741,13 +741,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.5.0.264.gc776916.dirty\n"},{"id":"282422","messageId":"1459470946-16522-3-git-send-email-sbeller@google.com","threadId":"41893","inReplyTo":"1459470946-16522-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 2/4] abbrev_sha1_in_line: don't leak memory","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-01T00:35:44Z","receivedAt":"2016-04-01T00:35:44Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"`split` is of type `struct strbuf **`, and currently we are leaking split\nitself as well as each element in split[i]. We have a dedicated free\nfunction for `struct strbuf **`, which takes care of freeing all\nrelated 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 bba2596..9f4be33 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.5.0.264.gc776916.dirty\n"},{"id":"282421","messageId":"1459470946-16522-4-git-send-email-sbeller@google.com","threadId":"41893","inReplyTo":"1459470946-16522-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 3/4] bundle: don't leak an fd in case of early return","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-01T00:35:45Z","receivedAt":"2016-04-01T00:35:45Z","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 | 18 +++++++++++++-----\n 1 file changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 506ac49..08e32ca 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -435,12 +435,14 @@ 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\terror(_(\"unrecognized argument: %s\"), argv[1]);\n+\t\tgoto err;\n+\t}\n \n \tobject_array_remove_duplicates(&revs.pending);\n \n@@ -448,17 +450,23 @@ int create_bundle(struct bundle_header *header, const char *path,\n \tif (!ref_count)\n \t\tdie(_(\"Refusing to create empty bundle.\"));\n \telse if (ref_count < 0)\n-\t\treturn -1;\n+\t\tgoto err;\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+\t\trollback_lock_file(&lock);\n+\t}\n+\treturn -1;\n }\n \n int unbundle(struct bundle_header *header, int bundle_fd, int flags)\n-- \n2.5.0.264.gc776916.dirty\n"},{"id":"282423","messageId":"1459470946-16522-5-git-send-email-sbeller@google.com","threadId":"41893","inReplyTo":"1459470946-16522-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 4/4] credential-cache, send_request: close fd when done","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-01T00:35:46Z","receivedAt":"2016-04-01T00:35:46Z","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.5.0.264.gc776916.dirty\n"},{"id":"282424","messageId":"CAPig+cQ=83k-of=jP1r6bYqsYrUhiDKhR3_Bm-SfYndn+sL=Dg@mail.gmail.com","threadId":"41893","inReplyTo":"1459470946-16522-2-git-send-email-sbeller@google.com","subject":"Re: [PATCHv4 1/4] notes: don't leak memory in git_config_get_notes_strategy","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-01T00:46:15Z","receivedAt":"2016-04-01T00:46:15Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 31, 2016 at 8:35 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_const` and\n> `parse_notes_merge_strategy` just compares the string against predefined\n> values, so no need to keep it around longer.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index 52aa9af..afcfa8f 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -741,13 +741,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\nMeh. Rather than reverting the git_config_get_value(), it would have\nbeen just as easy and safer (less chance of a future change\nre-introducing a leak) if you had just inserted the necessary check\nhere:\n\n if (!value)\n return config_error_nonbool(key);\n\nBut, perhaps it's not worth the patch churn at this point...\n\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> --\n> 2.5.0.264.gc776916.dirty\n"},{"id":"282479","messageId":"xmqqh9fljojo.fsf@gitster.mtv.corp.google.com","threadId":"41893","inReplyTo":"CAPig+cQ=83k-of=jP1r6bYqsYrUhiDKhR3_Bm-SfYndn+sL=Dg@mail.gmail.com","subject":"Re: [PATCHv4 1/4] notes: don't leak memory in git_config_get_notes_strategy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-01T16:12:43Z","receivedAt":"2016-04-01T16:12:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Meh. Rather than reverting the git_config_get_value(), it would have\n> been just as easy and safer (less chance of a future change\n> re-introducing a leak) if you had just inserted the necessary check\n> here:\n>\n> if (!value)\n> return config_error_nonbool(key);\n\nYup, sounds much more sensible fix that is useful in the longer term\n(and avoids one unnecessary strdup()).\n\n>\n> But, perhaps it's not worth the patch churn at this point...\n>\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>> --\n>> 2.5.0.264.gc776916.dirty\n"},{"id":"282482","messageId":"xmqq4mbljm76.fsf@gitster.mtv.corp.google.com","threadId":"41893","inReplyTo":"xmqqh9fljojo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv4 1/4] notes: don't leak memory in git_config_get_notes_strategy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-01T17:03:25Z","receivedAt":"2016-04-01T17:03:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Eric Sunshine writes:\n>\n>> Meh. Rather than reverting the git_config_get_value(), it would have\n>> been just as easy and safer (less chance of a future change\n>> re-introducing a leak) if you had just inserted the necessary check\n>> here:\n>>\n>> if (!value)\n>> return config_error_nonbool(key);\n>\n> Yup, sounds much more sensible fix that is useful in the longer term\n> (and avoids one unnecessary strdup()).\n\nPerhaps like this?\n\n-- >8 --\nFrom: Stefan Beller \nDate: Thu, 31 Mar 2016 11:04:03 -0700\nSubject: [PATCH] notes: don't leak memory in git_config_get_notes_strategy\n\nThis function asks for the value of a configuration and\nafter using the value does not have to retain ownership\nof the value. git_config_get_string_const() however is\na function to get a copy of the value, but we forget to\nfree it before we return.\n\nBecause we only need to peek the value without retaining\na pointer to it, use git_config_get_value() to peek into\nthe value cached in the config API layer.\n\nAs git_config_get_value() does not insist the value to be\na string, we'd need to do the \"nonbool\" check ourselves.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n builtin/notes.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 52aa9af..c1265eb 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -743,8 +743,10 @@ static int git_config_get_notes_strategy(const char *key,\n {\n \tconst char *value;\n \n-\tif (git_config_get_string_const(key, &value))\n+\tif (git_config_get_value(key, &value))\n \t\treturn 1;\n+\tif (!value)\n+\t\treturn config_error_nonbool(key);\n \tif (parse_notes_merge_strategy(value, strategy))\n \t\tgit_die_config(key, \"unknown notes merge strategy %s\", value);\n \n-- \n2.8.0-225-g297c00e\n"},{"id":"282483","messageId":"xmqqzitdi7iq.fsf@gitster.mtv.corp.google.com","threadId":"41893","inReplyTo":"1459470946-16522-4-git-send-email-sbeller@google.com","subject":"Re: [PATCHv4 3/4] bundle: don't leak an fd in case of early return","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-01T17:05:49Z","receivedAt":"2016-04-01T17:05:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\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\nThanks. I think this turned out to be the trickiest one to get\nright among the four and my reading of this round tells me that\nit addresses all the trickiness pointed out in the reviews.\n\nWill replace.\n\n> bundle.c | 18 +++++++++++++-----\n> 1 file changed, 13 insertions(+), 5 deletions(-)\n>\n> diff --git a/bundle.c b/bundle.c\n> index 506ac49..08e32ca 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -435,12 +435,14 @@ 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\terror(_(\"unrecognized argument: %s\"), argv[1]);\n> +\t\tgoto err;\n> +\t}\n> \n> \tobject_array_remove_duplicates(&revs.pending);\n> \n> @@ -448,17 +450,23 @@ int create_bundle(struct bundle_header *header, const char *path,\n> \tif (!ref_count)\n> \t\tdie(_(\"Refusing to create empty bundle.\"));\n> \telse if (ref_count < 0)\n> -\t\treturn -1;\n> +\t\tgoto err;\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> +\t\trollback_lock_file(&lock);\n> +\t}\n> +\treturn -1;\n> }\n> \n> int unbundle(struct bundle_header *header, int bundle_fd, int flags)\n"},{"id":"282484","messageId":"20160401171424.GA20345@sigill.intra.peff.net","threadId":"41893","inReplyTo":"xmqq4mbljm76.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv4 1/4] notes: don't leak memory in git_config_get_notes_strategy","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-01T17:14:24Z","receivedAt":"2016-04-01T17:14:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 01, 2016 at 10:03:25AM -0700, Junio C Hamano wrote:\n\n> -- >8 --\n> From: Stefan Beller \n> Date: Thu, 31 Mar 2016 11:04:03 -0700\n> Subject: [PATCH] notes: don't leak memory in git_config_get_notes_strategy\n> \n> This function asks for the value of a configuration and\n> after using the value does not have to retain ownership\n> of the value. git_config_get_string_const() however is\n> a function to get a copy of the value, but we forget to\n> free it before we return.\n> \n> Because we only need to peek the value without retaining\n> a pointer to it, use git_config_get_value() to peek into\n> the value cached in the config API layer.\n> \n> As git_config_get_value() does not insist the value to be\n> a string, we'd need to do the \"nonbool\" check ourselves.\n\nUnfortunately, I don't think this is quite right. In the original, we\nrelied on git_config_get_string_const to notice a non-string value, at\nwhich point it would die:\n\n $ git -c notes.mergeStrategy notes merge whatever\n error: Missing value for 'notes.mergeStrategy'\n fatal: unable to parse 'notes.mergeStrategy' from command-line config\n\nBut in your patch:\n\n> @@ -743,8 +743,10 @@ static int git_config_get_notes_strategy(const char *key,\n> {\n> \tconst char *value;\n> \n> -\tif (git_config_get_string_const(key, &value))\n> +\tif (git_config_get_value(key, &value))\n> \t\treturn 1;\n> +\tif (!value)\n> +\t\treturn config_error_nonbool(key);\n> \tif (parse_notes_merge_strategy(value, strategy))\n> \t\tgit_die_config(key, \"unknown notes merge strategy %s\", value);\n\nWe just return an error from git_config_get_notes_strategy(). If this\nwere a callback to git_config(), that would be fine (as we would\nauto-die then in the caller), but it's not. It is called directly for a\nspecific key. One of the callers treats a non-zero return as \"we don't\nhave that variable\", and the other ignores the return value completely.\n\nSo I think you'd want something more like:\n\n if (!value) {\n\tconfig_error_nonbool(key);\n\tgit_die_config(key);\n }\n\nThat keeps the original message intact (though it is a bit verbose in\nthe first place).\n\nThis is why I wondered if the minor \"do not allocate\" tweak was worth\nthe trouble, when git_config_get_string() just handles this for us.\n\n-Peff\n"},{"id":"282485","messageId":"20160401171535.GB20345@sigill.intra.peff.net","threadId":"41893","inReplyTo":"xmqqzitdi7iq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv4 3/4] bundle: don't leak an fd in case of early return","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-01T17:15:35Z","receivedAt":"2016-04-01T17:15:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 01, 2016 at 10:05:49AM -0700, Junio C Hamano wrote:\n\n> Stefan Beller writes:\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> Thanks. I think this turned out to be the trickiest one to get\n> right among the four and my reading of this round tells me that\n> it addresses all the trickiness pointed out in the reviews.\n\nYou didn't see my response to the other patch yet. :)\n\n> > \t/* write pack */\n> > \tif (write_pack_data(bundle_fd, &revs))\n> > -\t\treturn -1;\n> > +\t\tgoto err;\n\nThis does still suffer from the double-close I mentioned earlier. I'm\nnot sure if we want to address that or not.\n\n-Peff\n"},{"id":"282489","messageId":"xmqqk2khi6eo.fsf@gitster.mtv.corp.google.com","threadId":"41893","inReplyTo":"20160401171535.GB20345@sigill.intra.peff.net","subject":"Re: [PATCHv4 3/4] bundle: don't leak an fd in case of early return","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-01T17:29:51Z","receivedAt":"2016-04-01T17:29:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> You didn't see my response to the other patch yet. :)\n>\n>> > \t/* write pack */\n>> > \tif (write_pack_data(bundle_fd, &revs))\n>> > -\t\treturn -1;\n>> > +\t\tgoto err;\n>\n> This does still suffer from the double-close I mentioned earlier. I'm\n> not sure if we want to address that or not.\n\nSomething like this on top?\n\n bundle.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 08e32ca..bbf4efa 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -453,8 +453,10 @@ int create_bundle(struct bundle_header *header, const char *path,\n \t\tgoto err;\n \n \t/* write pack */\n-\tif (write_pack_data(bundle_fd, &revs))\n+\tif (write_pack_data(bundle_fd, &revs)) {\n+\t\tbundle_fd = -1; /* already closed by the above call */\n \t\tgoto err;\n+\t}\n \n \tif (!bundle_to_stdout) {\n \t\tif (commit_lock_file(&lock))\n@@ -463,7 +465,8 @@ int create_bundle(struct bundle_header *header, const char *path,\n \treturn 0;\n err:\n \tif (!bundle_to_stdout) {\n-\t\tclose(bundle_fd);\n+\t\tif (0 <= bundle_fd)\n+\t\t\tclose(bundle_fd);\n \t\trollback_lock_file(&lock);\n \t}\n \treturn -1;\n"},{"id":"282490","messageId":"CAPig+cRNVoNAwLV5J0Epmdgo=t79ab-=mPLDH4kTxJjnuQ6miA@mail.gmail.com","threadId":"41893","inReplyTo":"20160401171424.GA20345@sigill.intra.peff.net","subject":"Re: [PATCHv4 1/4] notes: don't leak memory in git_config_get_notes_strategy","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-01T17:30:39Z","receivedAt":"2016-04-01T17:30:39Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Apr 1, 2016 at 1:14 PM, Jeff King wrote:\n> On Fri, Apr 01, 2016 at 10:03:25AM -0700, Junio C Hamano wrote:\n>> From: Stefan Beller \n>> Date: Thu, 31 Mar 2016 11:04:03 -0700\n>> Subject: [PATCH] notes: don't leak memory in git_config_get_notes_strategy\n>>\n>> This function asks for the value of a configuration and\n>> after using the value does not have to retain ownership\n>> of the value. git_config_get_string_const() however is\n>> a function to get a copy of the value, but we forget to\n>> free it before we return.\n>>\n>> Because we only need to peek the value without retaining\n>> a pointer to it, use git_config_get_value() to peek into\n>> the value cached in the config API layer.\n>>\n>> As git_config_get_value() does not insist the value to be\n>> a string, we'd need to do the \"nonbool\" check ourselves.\n\nNicer commit message.\n\n> Unfortunately, I don't think this is quite right. In the original, we\n> relied on git_config_get_string_const to notice a non-string value, at\n> which point it would die:\n>\n> $ git -c notes.mergeStrategy notes merge whatever\n> error: Missing value for 'notes.mergeStrategy'\n> fatal: unable to parse 'notes.mergeStrategy' from command-line config\n>\n> But in your patch:\n>\n>> + if (!value)\n>> + return config_error_nonbool(key);\n>\n> We just return an error from git_config_get_notes_strategy(). If this\n> were a callback to git_config(), that would be fine (as we would\n> auto-die then in the caller), but it's not. It is called directly for a\n> specific key. One of the callers treats a non-zero return as \"we don't\n> have that variable\", and the other ignores the return value completely.\n>\n> So I think you'd want something more like:\n>\n> if (!value) {\n> config_error_nonbool(key);\n> git_die_config(key);\n> }\n\nYep, I had noted the bit about having to die() manually when reviewing\nthe earlier patch, but it slipped from memory when composing the reply\nto the current version of the patch.\n\n> This is why I wondered if the minor \"do not allocate\" tweak was worth\n> the trouble, when git_config_get_string() just handles this for us.\n\nWhich again suggests a new function which does this work but doesn't\ncopy the string (despite the already quite large set of similar\nfunctions).\n"},{"id":"282491","messageId":"20160401173109.GA21061@sigill.intra.peff.net","threadId":"41893","inReplyTo":"xmqqk2khi6eo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv4 3/4] bundle: don't leak an fd in case of early return","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-01T17:31:09Z","receivedAt":"2016-04-01T17:31:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 01, 2016 at 10:29:51AM -0700, Junio C Hamano wrote:\n\n> > This does still suffer from the double-close I mentioned earlier. I'm\n> > not sure if we want to address that or not.\n> \n> Something like this on top?\n> \n> bundle.c | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n> \n> diff --git a/bundle.c b/bundle.c\n> index 08e32ca..bbf4efa 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -453,8 +453,10 @@ int create_bundle(struct bundle_header *header, const char *path,\n> \t\tgoto err;\n> \n> \t/* write pack */\n> -\tif (write_pack_data(bundle_fd, &revs))\n> +\tif (write_pack_data(bundle_fd, &revs)) {\n> +\t\tbundle_fd = -1; /* already closed by the above call */\n> \t\tgoto err;\n> +\t}\n> \n> \tif (!bundle_to_stdout) {\n> \t\tif (commit_lock_file(&lock))\n> @@ -463,7 +465,8 @@ int create_bundle(struct bundle_header *header, const char *path,\n> \treturn 0;\n> err:\n> \tif (!bundle_to_stdout) {\n> -\t\tclose(bundle_fd);\n> +\t\tif (0 <= bundle_fd)\n> +\t\t\tclose(bundle_fd);\n> \t\trollback_lock_file(&lock);\n\nYep, that looks fine.\n\n-Peff\n"}]}