{"thread":{"id":"41867","subject":"[PATCH 0/6] Some cleanups","startedAt":"2016-03-30T00:38:47Z","lastAt":"2016-03-30T17:21:58Z","messageCount":24,"participants":["Stefan Beller","Junio C Hamano","Eric Sunshine","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"282155","messageId":"1459298333-21899-1-git-send-email-sbeller@google.com","threadId":"41867","inReplyTo":null,"subject":"[PATCH 0/6] Some cleanups","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-30T00:38:47Z","receivedAt":"2016-03-30T00:38:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"One 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 (6):\n path.c: allocate enough memory for string\n imap-send.c, cram: allocate enough memory for null terminated string\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 | 1 +\n bundle.c | 10 ++++++++--\n credential-cache.c | 1 +\n imap-send.c | 2 +-\n path.c | 2 +-\n wt-status.c | 2 +-\n 6 files changed, 13 insertions(+), 5 deletions(-)\n\n-- \n2.8.0.8.g27a27a6.dirty\n"},{"id":"282156","messageId":"1459298333-21899-2-git-send-email-sbeller@google.com","threadId":"41867","inReplyTo":"1459298333-21899-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/6] path.c: allocate enough memory for string","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-30T00:38:48Z","receivedAt":"2016-03-30T00:38:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"`strlen` returns the length of a string without the terminating null byte.\nTo make sure enough memory is allocated we need to pass `strlen(..) + 1`\nto the allocation function.\n\nSigned-off-by: Stefan Beller \n---\n path.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/path.c b/path.c\nindex 969b494..0ae8af5 100644\n--- a/path.c\n+++ b/path.c\n@@ -155,7 +155,7 @@ static struct trie *make_trie_node(const char *key, void *value)\n \tstruct trie *new_node = xcalloc(1, sizeof(*new_node));\n \tnew_node->len = strlen(key);\n \tif (new_node->len) {\n-\t\tnew_node->contents = xmalloc(new_node->len);\n+\t\tnew_node->contents = xmalloc(new_node->len + 1);\n \t\tmemcpy(new_node->contents, key, new_node->len);\n \t}\n \tnew_node->value = value;\n-- \n2.8.0.8.g27a27a6.dirty\n"},{"id":"282161","messageId":"1459298333-21899-3-git-send-email-sbeller@google.com","threadId":"41867","inReplyTo":"1459298333-21899-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/6] imap-send.c, cram: allocate enough memory for null terminated string","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-30T00:38:49Z","receivedAt":"2016-03-30T00:38:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"`strlen` returns the length of a string without the terminating null byte.\nTo make sure enough memory is allocated we need to pass `strlen(..) + 1`\nto the allocation function.\n\nSigned-off-by: Stefan Beller \n---\n imap-send.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 2c52027..f7e9909 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -872,7 +872,7 @@ static char *cram(const char *challenge_64, const char *user, const char *pass)\n \t * enough upper bound for challenge (decoded result).\n \t */\n \tencoded_len = strlen(challenge_64);\n-\tchallenge = xmalloc(encoded_len);\n+\tchallenge = xmalloc(encoded_len + 1);\n \tdecoded_len = EVP_DecodeBlock((unsigned char *)challenge,\n \t\t\t\t (unsigned char *)challenge_64, encoded_len);\n \tif (decoded_len < 0)\n-- \n2.8.0.8.g27a27a6.dirty\n"},{"id":"282158","messageId":"1459298333-21899-4-git-send-email-sbeller@google.com","threadId":"41867","inReplyTo":"1459298333-21899-1-git-send-email-sbeller@google.com","subject":"[PATCH 3/6] notes: don't leak memory in git_config_get_notes_strategy","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-30T00:38:50Z","receivedAt":"2016-03-30T00:38:50Z","isPatch":true,"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 | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex ed6f222..d6bab17 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -751,6 +751,7 @@ static int git_config_get_notes_strategy(const char *key,\n \tif (parse_notes_merge_strategy(value, strategy))\n \t\tgit_die_config(key, \"unknown notes merge strategy %s\", value);\n \n+\tfree((void*)value);\n \treturn 0;\n }\n \n-- \n2.8.0.8.g27a27a6.dirty\n"},{"id":"282157","messageId":"1459298333-21899-5-git-send-email-sbeller@google.com","threadId":"41867","inReplyTo":"1459298333-21899-1-git-send-email-sbeller@google.com","subject":"[PATCH 4/6] abbrev_sha1_in_line: don't leak memory","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-30T00:38:51Z","receivedAt":"2016-03-30T00:38:51Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"`split` is of type `struct strbuf **` and just before the new free,\nwe release the inner strbufs. Make sure to also release the memory\ncontaining the pointers to the individual strbufs.\n\nSigned-off-by: Stefan Beller \n---\n wt-status.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex ef74864..a78cfc0 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1065,7 +1065,7 @@ static void abbrev_sha1_in_line(struct strbuf *line)\n \t}\n \tfor (i = 0; split[i]; i++)\n \t\tstrbuf_release(split[i]);\n-\n+\tfree(split);\n }\n \n static void read_rebase_todolist(const char *fname, struct string_list *lines)\n-- \n2.8.0.8.g27a27a6.dirty\n"},{"id":"282160","messageId":"1459298333-21899-6-git-send-email-sbeller@google.com","threadId":"41867","inReplyTo":"1459298333-21899-1-git-send-email-sbeller@google.com","subject":"[PATCH 5/6] bundle: don't leak an fd in case of early return","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-30T00:38:52Z","receivedAt":"2016-03-30T00:38:52Z","isPatch":true,"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\nourselves.\n\nSigned-off-by: Stefan Beller \n---\n bundle.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 506ac49..04d62af 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -434,8 +434,11 @@ int create_bundle(struct bundle_header *header, const char *path,\n \tinit_revisions(&revs, NULL);\n \n \t/* write prerequisites */\n-\tif (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv))\n+\tif (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv)) {\n+\t\tif (!bundle_to_stdout)\n+\t\t\tclose(bundle_fd);\n \t\treturn -1;\n+\t}\n \n \targc = setup_revisions(argc, argv, &revs, NULL);\n \n@@ -447,8 +450,11 @@ int create_bundle(struct bundle_header *header, const char *path,\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+\telse if (ref_count < 0) {\n+\t\tif (!bundle_to_stdout)\n+\t\t\tclose(bundle_fd);\n \t\treturn -1;\n+\t}\n \n \t/* write pack */\n \tif (write_pack_data(bundle_fd, &revs))\n-- \n2.8.0.8.g27a27a6.dirty\n"},{"id":"282159","messageId":"1459298333-21899-7-git-send-email-sbeller@google.com","threadId":"41867","inReplyTo":"1459298333-21899-1-git-send-email-sbeller@google.com","subject":"[PATCH 6/6] credential-cache, send_request: close fd when done","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-30T00:38:53Z","receivedAt":"2016-03-30T00:38:53Z","isPatch":true,"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.8.g27a27a6.dirty\n"},{"id":"282163","messageId":"xmqq7fgkvl47.fsf@gitster.mtv.corp.google.com","threadId":"41867","inReplyTo":"1459298333-21899-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/6] path.c: allocate enough memory for string","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-30T00:56:56Z","receivedAt":"2016-03-30T00:56:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> `strlen` returns the length of a string without the terminating null byte.\n> To make sure enough memory is allocated we need to pass `strlen(..) + 1`\n> to the allocation function.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> path.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/path.c b/path.c\n> index 969b494..0ae8af5 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -155,7 +155,7 @@ static struct trie *make_trie_node(const char *key, void *value)\n> \tstruct trie *new_node = xcalloc(1, sizeof(*new_node));\n> \tnew_node->len = strlen(key);\n> \tif (new_node->len) {\n> -\t\tnew_node->contents = xmalloc(new_node->len);\n> +\t\tnew_node->contents = xmalloc(new_node->len + 1);\n> \t\tmemcpy(new_node->contents, key, new_node->len);\n> \t}\n\nThis structure looks like a counted string that does not\nwant to have a terminating NUL after the contents, judging from the\nway memcpy() copies only len and not len+1.\n\nDid I write this (wondering why this was addressed to me)?\n\n> \tnew_node->value = value;\n"},{"id":"282164","messageId":"CAPig+cSqC1muy+vcmpkp9uM9T3CC4pa5jWGUot_FtrVshEAUEw@mail.gmail.com","threadId":"41867","inReplyTo":"1459298333-21899-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/6] path.c: allocate enough memory for string","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-30T00:57:14Z","receivedAt":"2016-03-30T00:57:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 29, 2016 at 8:38 PM, Stefan Beller wrote:\n> `strlen` returns the length of a string without the terminating null byte.\n> To make sure enough memory is allocated we need to pass `strlen(..) + 1`\n> to the allocation function.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/path.c b/path.c\n> @@ -155,7 +155,7 @@ static struct trie *make_trie_node(const char *key, void *value)\n> struct trie *new_node = xcalloc(1, sizeof(*new_node));\n> new_node->len = strlen(key);\n> if (new_node->len) {\n> - new_node->contents = xmalloc(new_node->len);\n> + new_node->contents = xmalloc(new_node->len + 1);\n> memcpy(new_node->contents, key, new_node->len);\n\nHuh? This is a trie. It never accesses 'contents' as a NUL-terminated\nstring. Plus, no NUL is ever even copied, thus this is just\noverallocating. How is this an improvement?\n\n> }\n> new_node->value = value;\n> --\n"},{"id":"282166","messageId":"CAPig+cSX4Jye60CKxR=OjhtAai09WSMNMOau2WHUxgXbKX2aDQ@mail.gmail.com","threadId":"41867","inReplyTo":"1459298333-21899-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/6] imap-send.c, cram: allocate enough memory for null terminated string","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-30T01:02:15Z","receivedAt":"2016-03-30T01:02:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 29, 2016 at 8:38 PM, Stefan Beller wrote:\n> `strlen` returns the length of a string without the terminating null byte.\n> To make sure enough memory is allocated we need to pass `strlen(..) + 1`\n> to the allocation function.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> imap-send.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/imap-send.c b/imap-send.c\n> index 2c52027..f7e9909 100644\n> --- a/imap-send.c\n> +++ b/imap-send.c\n> @@ -872,7 +872,7 @@ static char *cram(const char *challenge_64, const char *user, const char *pass)\n> * enough upper bound for challenge (decoded result).\n> */\n> encoded_len = strlen(challenge_64);\n> - challenge = xmalloc(encoded_len);\n> + challenge = xmalloc(encoded_len + 1);\n\n'challenge' is never used as a NUL-terminated string, and\nEVP_DecodeBlock() is not advertised as adding a terminating NUL, so\nwhy is this an improvement?\n\n> decoded_len = EVP_DecodeBlock((unsigned char *)challenge,\n> (unsigned char *)challenge_64, encoded_len);\n> if (decoded_len < 0)\n> --\n"},{"id":"282168","messageId":"20160330010756.GA2237@sigill.intra.peff.net","threadId":"41867","inReplyTo":"1459298333-21899-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/6] imap-send.c, cram: allocate enough memory for null terminated string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-30T01:07:57Z","receivedAt":"2016-03-30T01:07:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 29, 2016 at 05:38:49PM -0700, Stefan Beller wrote:\n\n> `strlen` returns the length of a string without the terminating null byte.\n> To make sure enough memory is allocated we need to pass `strlen(..) + 1`\n> to the allocation function.\n> \n> Signed-off-by: Stefan Beller \n> ---\n> imap-send.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/imap-send.c b/imap-send.c\n> index 2c52027..f7e9909 100644\n> --- a/imap-send.c\n> +++ b/imap-send.c\n> @@ -872,7 +872,7 @@ static char *cram(const char *challenge_64, const char *user, const char *pass)\n> \t * enough upper bound for challenge (decoded result).\n> \t */\n> \tencoded_len = strlen(challenge_64);\n> -\tchallenge = xmalloc(encoded_len);\n> +\tchallenge = xmalloc(encoded_len + 1);\n> \tdecoded_len = EVP_DecodeBlock((unsigned char *)challenge,\n> \t\t\t\t (unsigned char *)challenge_64, encoded_len);\n> \tif (decoded_len < 0)\n\nOthers pointed out that patches 1 and 2 here probably don't need the\nextra byte. But as a side note, for any cases that do, please use\nxmallocz() instead of manually adding 1 to the length. Even if you don't\nneed the final byte to be NUL, it checks for integer overflow.\n\n-Peff\n"},{"id":"282169","messageId":"20160330011111.GB2237@sigill.intra.peff.net","threadId":"41867","inReplyTo":"1459298333-21899-5-git-send-email-sbeller@google.com","subject":"Re: [PATCH 4/6] abbrev_sha1_in_line: don't leak memory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-30T01:11:11Z","receivedAt":"2016-03-30T01:11:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 29, 2016 at 05:38:51PM -0700, Stefan Beller wrote:\n\n> `split` is of type `struct strbuf **` and just before the new free,\n> we release the inner strbufs. Make sure to also release the memory\n> containing the pointers to the individual strbufs.\n> \n> Signed-off-by: Stefan Beller \n> ---\n> wt-status.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/wt-status.c b/wt-status.c\n> index ef74864..a78cfc0 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -1065,7 +1065,7 @@ static void abbrev_sha1_in_line(struct strbuf *line)\n> \t}\n> \tfor (i = 0; split[i]; i++)\n> \t\tstrbuf_release(split[i]);\n> -\n> +\tfree(split);\n> }\n\nI think this can just combine with the for-loop above to become\nstrbuf_list_free().\n\n-Peff\n"},{"id":"282170","messageId":"CAPig+cQxtFDY8kAx-UD_dYatTJJnx260CaY5Ke3yDOS666YEtA@mail.gmail.com","threadId":"41867","inReplyTo":"1459298333-21899-4-git-send-email-sbeller@google.com","subject":"Re: [PATCH 3/6] notes: don't leak memory in git_config_get_notes_strategy","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-30T01:11:30Z","receivedAt":"2016-03-30T01:11:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 29, 2016 at 8:38 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> @@ -751,6 +751,7 @@ static int git_config_get_notes_strategy(const char *key,\n> if (parse_notes_merge_strategy(value, strategy))\n> git_die_config(key, \"unknown notes merge strategy %s\", value);\n>\n> + free((void*)value);\n\nI wonder if the intent would be clearer if you gave 'value' type 'char\n*' rather than 'const char *', and called git_config_get_string()\nrather than git_config_get_string_const().\n\n> return 0;\n> }\n"},{"id":"282171","messageId":"20160330011334.GC2237@sigill.intra.peff.net","threadId":"41867","inReplyTo":"1459298333-21899-4-git-send-email-sbeller@google.com","subject":"Re: [PATCH 3/6] notes: don't leak memory in git_config_get_notes_strategy","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-30T01:13:34Z","receivedAt":"2016-03-30T01:13:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 29, 2016 at 05:38:50PM -0700, Stefan Beller wrote:\n\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> builtin/notes.c | 1 +\n> 1 file changed, 1 insertion(+)\n> \n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index ed6f222..d6bab17 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -751,6 +751,7 @@ static int git_config_get_notes_strategy(const char *key,\n> \tif (parse_notes_merge_strategy(value, strategy))\n> \t\tgit_die_config(key, \"unknown notes merge strategy %s\", value);\n> \n> +\tfree((void*)value);\n> \treturn 0;\n> }\n\nI don't think this is wrong, but would it perhaps be simpler to call\ngit_config_get_value() in the first place, which does not make a copy of\nthe string?\n\n-Peff\n"},{"id":"282172","messageId":"20160330011743.GD2237@sigill.intra.peff.net","threadId":"41867","inReplyTo":"1459298333-21899-6-git-send-email-sbeller@google.com","subject":"Re: [PATCH 5/6] bundle: don't leak an fd in case of early return","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-30T01:17:43Z","receivedAt":"2016-03-30T01:17:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 29, 2016 at 05:38:52PM -0700, Stefan Beller wrote:\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> ourselves.\n> \n> Signed-off-by: Stefan Beller \n> ---\n> bundle.c | 10 ++++++++--\n> 1 file changed, 8 insertions(+), 2 deletions(-)\n> \n> diff --git a/bundle.c b/bundle.c\n> index 506ac49..04d62af 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -434,8 +434,11 @@ int create_bundle(struct bundle_header *header, const char *path,\n> \tinit_revisions(&revs, NULL);\n> \n> \t/* write prerequisites */\n> -\tif (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv))\n> +\tif (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv)) {\n> +\t\tif (!bundle_to_stdout)\n> +\t\t\tclose(bundle_fd);\n> \t\treturn -1;\n> +\t}\n\nMakes sense. Should we also be rolling back the lock file? It happens\nautomatically at program exit, of course, but we are in library code\nhere that should not rely on that.\n\n> @@ -447,8 +450,11 @@ int create_bundle(struct bundle_header *header, const char *path,\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> +\telse if (ref_count < 0) {\n> +\t\tif (!bundle_to_stdout)\n> +\t\t\tclose(bundle_fd);\n> \t\treturn -1;\n> +\t}\n\nDitto here, and in the error return from write_pack_data(). There are\nenough spots that it may be worth setting up a \"goto err\" path.\n\n-Peff\n"},{"id":"282173","messageId":"20160330011959.GE2237@sigill.intra.peff.net","threadId":"41867","inReplyTo":"1459298333-21899-7-git-send-email-sbeller@google.com","subject":"Re: [PATCH 6/6] credential-cache, send_request: close fd when done","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-30T01:20:00Z","receivedAt":"2016-03-30T01:20:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 29, 2016 at 05:38:53PM -0700, Stefan Beller wrote:\n\n> No need to keep it open any further.\n> \n> Signed-off-by: Stefan Beller \n> ---\n> credential-cache.c | 1 +\n> 1 file changed, 1 insertion(+)\n> \n> diff --git a/credential-cache.c b/credential-cache.c\n> index 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\nLooks good. I think nobody ever noticed because credential-cache\nbasically sends off the request and then exits.\n\n-Peff\n"},{"id":"282181","messageId":"CAPig+cR=Zc2QAQTPAqTt4M-nJRw4StCoqT2hYtmHOvi6LseN_A@mail.gmail.com","threadId":"41867","inReplyTo":"20160330011111.GB2237@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] abbrev_sha1_in_line: don't leak memory","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-30T01:30:38Z","receivedAt":"2016-03-30T01:30:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 29, 2016 at 9:11 PM, Jeff King wrote:\n> On Tue, Mar 29, 2016 at 05:38:51PM -0700, Stefan Beller wrote:\n>> `split` is of type `struct strbuf **` and just before the new free,\n>> we release the inner strbufs. Make sure to also release the memory\n>> containing the pointers to the individual strbufs.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> diff --git a/wt-status.c b/wt-status.c\n>> @@ -1065,7 +1065,7 @@ static void abbrev_sha1_in_line(struct strbuf *line)\n>> }\n>> for (i = 0; split[i]; i++)\n>> strbuf_release(split[i]);\n>> -\n>> + free(split);\n>> }\n>\n> I think this can just combine with the for-loop above to become\n> strbuf_list_free().\n\nThe implementation of strbuf_list_free() is this:\n\n struct strbuf **s = sbs;\n while (*s) {\n strbuf_release(*s);\n free(*s++);\n }\n free(sbs);\n\nwhich means that wt-status.c is leaking not only 'split', but also\neach element of split[], right?\n"},{"id":"282182","messageId":"20160330013127.GA6680@sigill.intra.peff.net","threadId":"41867","inReplyTo":"CAPig+cR=Zc2QAQTPAqTt4M-nJRw4StCoqT2hYtmHOvi6LseN_A@mail.gmail.com","subject":"Re: [PATCH 4/6] abbrev_sha1_in_line: don't leak memory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-30T01:31:28Z","receivedAt":"2016-03-30T01:31:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 29, 2016 at 09:30:38PM -0400, Eric Sunshine wrote:\n\n> On Tue, Mar 29, 2016 at 9:11 PM, Jeff King wrote:\n> > On Tue, Mar 29, 2016 at 05:38:51PM -0700, Stefan Beller wrote:\n> >> `split` is of type `struct strbuf **` and just before the new free,\n> >> we release the inner strbufs. Make sure to also release the memory\n> >> containing the pointers to the individual strbufs.\n> >>\n> >> Signed-off-by: Stefan Beller \n> >> ---\n> >> diff --git a/wt-status.c b/wt-status.c\n> >> @@ -1065,7 +1065,7 @@ static void abbrev_sha1_in_line(struct strbuf *line)\n> >> }\n> >> for (i = 0; split[i]; i++)\n> >> strbuf_release(split[i]);\n> >> -\n> >> + free(split);\n> >> }\n> >\n> > I think this can just combine with the for-loop above to become\n> > strbuf_list_free().\n> \n> The implementation of strbuf_list_free() is this:\n> \n> struct strbuf **s = sbs;\n> while (*s) {\n> strbuf_release(*s);\n> free(*s++);\n> }\n> free(sbs);\n> \n> which means that wt-status.c is leaking not only 'split', but also\n> each element of split[], right?\n\nYeah, I didn't notice that, but I think you're right.\n\n-Peff\n"},{"id":"282234","messageId":"CAGZ79kb6dsaSxQ+HaYODBurnFz3MbsisSNkdrckFZLvSCvS-JQ@mail.gmail.com","threadId":"41867","inReplyTo":"CAPig+cSqC1muy+vcmpkp9uM9T3CC4pa5jWGUot_FtrVshEAUEw@mail.gmail.com","subject":"Re: [PATCH 1/6] path.c: allocate enough memory for string","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-30T16:41:38Z","receivedAt":"2016-03-30T16:41:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Mar 29, 2016 at 5:57 PM, Eric Sunshine wrote:\n> On Tue, Mar 29, 2016 at 8:38 PM, Stefan Beller wrote:\n>> `strlen` returns the length of a string without the terminating null byte.\n>> To make sure enough memory is allocated we need to pass `strlen(..) + 1`\n>> to the allocation function.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> diff --git a/path.c b/path.c\n>> @@ -155,7 +155,7 @@ static struct trie *make_trie_node(const char *key, void *value)\n>> struct trie *new_node = xcalloc(1, sizeof(*new_node));\n>> new_node->len = strlen(key);\n>> if (new_node->len) {\n>> - new_node->contents = xmalloc(new_node->len);\n>> + new_node->contents = xmalloc(new_node->len + 1);\n>> memcpy(new_node->contents, key, new_node->len);\n>\n> Huh? This is a trie. It never accesses 'contents' as a NUL-terminated\n> string. Plus, no NUL is ever even copied, thus this is just\n> overallocating. How is this an improvement?\n\nBy using strlen, I assumed it was a standard C string.\nI missed that, though.\n\n>\n>> }\n>> new_node->value = value;\n>> --\n"},{"id":"282241","messageId":"xmqqzitfuc7y.fsf@gitster.mtv.corp.google.com","threadId":"41867","inReplyTo":"20160330013127.GA6680@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] abbrev_sha1_in_line: don't leak memory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-30T17:06:41Z","receivedAt":"2016-03-30T17:06:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Mar 29, 2016 at 09:30:38PM -0400, Eric Sunshine wrote:\n>\n>> The implementation of strbuf_list_free() is this:\n>> \n>> struct strbuf **s = sbs;\n>> while (*s) {\n>> strbuf_release(*s);\n>> free(*s++);\n>> }\n>> free(sbs);\n>> \n>> which means that wt-status.c is leaking not only 'split', but also\n>> each element of split[], right?\n>\n> Yeah, I didn't notice that, but I think you're right.\n\nCorrect.\n\nI suspect that we would be better off in the longer term if\nwe made conscious effort to deprecate and eradicate the use\nof strbuf_split* API functions. They are easy to write\ncrappy code with, inefficient, and error prone. You would\nrarely need to have N resulting pieces as strbufs to be\neasily manipulatable [*1*].\n\nThe function can be written by not using the \"split and then\nrebuild\" pattern, perhaps like so, and the result is even\neasier to read and understand, I would say. A sample rewrite\nis attached at the end.\n\n\n[Footnote]\n\n*1* I wouldn't be this harsh if the function were to fill an\n array of pointers or offsets into the original string.\n\n wt-status.c | 43 +++++++++++++++++++------------------------\n 1 file changed, 19 insertions(+), 24 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex ef74864..4886c35 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1037,35 +1037,30 @@ static int split_commit_in_progress(struct wt_status *s)\n */\n static void abbrev_sha1_in_line(struct strbuf *line)\n {\n-\tstruct strbuf **split;\n-\tint i;\n+\tconst char *cp, *ep, *abbrev;\n+\tunsigned char sha1[20];\n \n-\tif (starts_with(line->buf, \"exec \") ||\n-\t starts_with(line->buf, \"x \"))\n+\t/* the oddball \"exec\" does not have 40-hex as the second token */\n+\tif (starts_with(line->buf, \"exec \") || starts_with(line->buf, \"x \"))\n \t\treturn;\n \n-\tsplit = strbuf_split_max(line, ' ', 3);\n-\tif (split[0] && split[1]) {\n-\t\tunsigned char sha1[20];\n-\t\tconst char *abbrev;\n+\t/* find the second token on the line */\n+\tcp = strchr(line->buf, ' ');\n+\tif (!cp)\n+\t\treturn;\n+\tcp++;\n+\tep = strchr(cp, ' ');\n+\tif (!ep)\n+\t\treturn;\n \n-\t\t/*\n-\t\t * strbuf_split_max left a space. Trim it and re-add\n-\t\t * it after abbreviation.\n-\t\t */\n-\t\tstrbuf_trim(split[1]);\n-\t\tif (!get_sha1(split[1]->buf, sha1)) {\n-\t\t\tabbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV);\n-\t\t\tstrbuf_reset(split[1]);\n-\t\t\tstrbuf_addf(split[1], \"%s \", abbrev);\n-\t\t\tstrbuf_reset(line);\n-\t\t\tfor (i = 0; split[i]; i++)\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+\t/* is it 40-hex? */\n+\tif (ep - cp != GIT_SHA1_HEXSZ || get_sha1_hex(cp, sha1))\n+\t\treturn;\n \n+\t/* replace it with its abbreviation */\n+\tabbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV);\n+\tstrbuf_splice(line, cp - line->buf, GIT_SHA1_HEXSZ,\n+\t\t abbrev, strlen(abbrev));\n }\n \n static void read_rebase_todolist(const char *fname, struct string_list *lines)\n"},{"id":"282243","messageId":"xmqqr3erubs2.fsf@gitster.mtv.corp.google.com","threadId":"41867","inReplyTo":"CAGZ79kb6dsaSxQ+HaYODBurnFz3MbsisSNkdrckFZLvSCvS-JQ@mail.gmail.com","subject":"Re: [PATCH 1/6] path.c: allocate enough memory for string","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-30T17:16:13Z","receivedAt":"2016-03-30T17:16:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Tue, Mar 29, 2016 at 5:57 PM, Eric Sunshine wrote:\n>> On Tue, Mar 29, 2016 at 8:38 PM, Stefan Beller wrote:\n>>> `strlen` returns the length of a string without the terminating null byte.\n>>> To make sure enough memory is allocated we need to pass `strlen(..) + 1`\n>>> to the allocation function.\n>>>\n>>> Signed-off-by: Stefan Beller \n>>> ---\n>>> diff --git a/path.c b/path.c\n>>> @@ -155,7 +155,7 @@ static struct trie *make_trie_node(const char *key, void *value)\n>>> struct trie *new_node = xcalloc(1, sizeof(*new_node));\n>>> new_node->len = strlen(key);\n>>> if (new_node->len) {\n>>> - new_node->contents = xmalloc(new_node->len);\n>>> + new_node->contents = xmalloc(new_node->len + 1);\n>>> memcpy(new_node->contents, key, new_node->len);\n>>\n>> Huh? This is a trie. It never accesses 'contents' as a NUL-terminated\n>> string. Plus, no NUL is ever even copied, thus this is just\n>> overallocating. How is this an improvement?\n>\n> By using strlen, I assumed it was a standard C string.\n> I missed that, though.\n\nYou took hint from a wrong place. You are auditing the destination\nbuffer, so the correct place to take hint from is the memcpy() that\ntouches the destination.\n"},{"id":"282244","messageId":"xmqqmvpfubq8.fsf@gitster.mtv.corp.google.com","threadId":"41867","inReplyTo":"20160330011334.GC2237@sigill.intra.peff.net","subject":"Re: [PATCH 3/6] notes: don't leak memory in git_config_get_notes_strategy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-30T17:17:19Z","receivedAt":"2016-03-30T17:17:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I don't think this is wrong, but would it perhaps be simpler to call\n> git_config_get_value() in the first place, which does not make a copy of\n> the string?\n\nYup, I agree.\n"},{"id":"282245","messageId":"xmqqio03ubn6.fsf@gitster.mtv.corp.google.com","threadId":"41867","inReplyTo":"20160330011743.GD2237@sigill.intra.peff.net","subject":"Re: [PATCH 5/6] bundle: don't leak an fd in case of early return","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-30T17:19:09Z","receivedAt":"2016-03-30T17:19:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> +\tif (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv)) {\n>> +\t\tif (!bundle_to_stdout)\n>> +\t\t\tclose(bundle_fd);\n>> \t\treturn -1;\n>> +\t}\n>\n> Makes sense. Should we also be rolling back the lock file? It happens\n> automatically at program exit, of course, but we are in library code\n> here that should not rely on that.\n\nYeah, I agree. I suspect that the original wasn't even meant to be\nused in a \"library-ish\" fashion, but as long as we are adding this\nclose(), we should also be doing the rollback.\n\nThanks.\n"},{"id":"282246","messageId":"20160330172157.GA19675@sigill.intra.peff.net","threadId":"41867","inReplyTo":"xmqqzitfuc7y.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/6] abbrev_sha1_in_line: don't leak memory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-30T17:21:58Z","receivedAt":"2016-03-30T17:21:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 30, 2016 at 10:06:41AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Tue, Mar 29, 2016 at 09:30:38PM -0400, Eric Sunshine wrote:\n> >\n> >> The implementation of strbuf_list_free() is this:\n> >> \n> >> struct strbuf **s = sbs;\n> >> while (*s) {\n> >> strbuf_release(*s);\n> >> free(*s++);\n> >> }\n> >> free(sbs);\n> >> \n> >> which means that wt-status.c is leaking not only 'split', but also\n> >> each element of split[], right?\n> >\n> > Yeah, I didn't notice that, but I think you're right.\n> \n> Correct.\n> \n> I suspect that we would be better off in the longer term if\n> we made conscious effort to deprecate and eradicate the use\n> of strbuf_split* API functions. They are easy to write\n> crappy code with, inefficient, and error prone. You would\n> rarely need to have N resulting pieces as strbufs to be\n> easily manipulatable [*1*].\n\nYeah, I agree that it is a clunky interface, and I would be happy to see\nit go away. Many callers would be better off with string_list_split().\nWhen I've looked in the past, though, converting some of the callers was\nsomewhat non-trivial.\n\nBut in this case...\n\n> The function can be written by not using the \"split and then\n> rebuild\" pattern, perhaps like so, and the result is even\n> easier to read and understand, I would say. A sample rewrite\n> is attached at the end.\n\nI agree that the function is much simpler without it.\n\n> +\t/* find the second token on the line */\n> +\tcp = strchr(line->buf, ' ');\n> +\tif (!cp)\n> +\t\treturn;\n> +\tcp++;\n> +\tep = strchr(cp, ' ');\n> +\tif (!ep)\n> +\t\treturn;\n\nWould we ever see \"cmd sha1\" without a third token? If so, we'd want:\n\n ep = strchrnul(cp, ' ');\n\n-Peff\n"}]}