{"thread":{"id":"45815","subject":"[PATCH 0/3] fix memory leak in git-am","startedAt":"2017-04-27T03:25:14Z","lastAt":"2017-04-27T05:43:39Z","messageCount":5,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"318031","messageId":"20170427032503.rgz3bnctnxdn3hin@sigill.intra.peff.net","threadId":"45815","inReplyTo":null,"subject":"[PATCH 0/3] fix memory leak in git-am","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-27T03:25:04Z","receivedAt":"2017-04-27T03:25:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This was noticed by Coverity. The leak isn't new, but I think was\n\"re-found\" by Coverity because some nearby code did an unrelated\ns/sha1/oid/ change, throwing off its heuristics.\n\nI also checked whether this was in Dscho's big pack of Coverity fixups\nfrom earlier today, and it's not.\n\nThe first one is the actual fix. The second one is pure cleanup, but I\nthink worth doing. The final one is arguably just churn, and maybe\npeople even like the end result less. I'm OK to drop it.\n\n  [1/3]: am: fix commit buffer leak in get_commit_info()\n  [2/3]: am: simplify allocations in get_commit_info()\n  [3/3]: am: shorten ident_split variable name in get_commit_info()\n\n builtin/am.c | 34 ++++++++++++++--------------------\n 1 file changed, 14 insertions(+), 20 deletions(-)\n\n-Peff\n"},{"id":"318032","messageId":"20170427032555.imjr5to4uys2cz4k@sigill.intra.peff.net","threadId":"45815","inReplyTo":"20170427032503.rgz3bnctnxdn3hin@sigill.intra.peff.net","subject":"[PATCH 1/3] am: fix commit buffer leak in get_commit_info()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-27T03:25:55Z","receivedAt":"2017-04-27T03:26:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Calling logmsg_reencode() may allocate a buffer for the\ncommit message (because we need to load it from disk, or\nbecause it needs re-encoded). We must \"unuse\" it afterwards\nto free it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/am.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 805f56cec..baab951ed 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1421,6 +1421,7 @@ static void get_commit_info(struct am_state *state, struct commit *commit)\n \t\tdie(_(\"unable to parse commit %s\"), oid_to_hex(&commit->object.oid));\n \tstate->msg = xstrdup(msg + 2);\n \tstate->msg_len = strlen(state->msg);\n+\tunuse_commit_buffer(commit, buffer);\n }\n \n /**\n-- \n2.13.0.rc0.465.ga1e654229\n\n"},{"id":"318033","messageId":"20170427032717.hzs3og4ql5vceqlf@sigill.intra.peff.net","threadId":"45815","inReplyTo":"20170427032503.rgz3bnctnxdn3hin@sigill.intra.peff.net","subject":"[PATCH 2/3] am: simplify allocations in get_commit_info()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-27T03:27:17Z","receivedAt":"2017-04-27T03:27:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After we call split_ident_line(), we have several begin/end\npairs for various parts of the ident. We then copy each into\na strbuf to create a single string, and then detach that\nstring.  We can instead skip the strbuf entirely and just\nduplicate the strings directly.\n\nThis is shorter, and it makes it more obvious that we are\nnot leaking the strbuf (we were not before, because every\ncode path either died or hit a strbuf_detach).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/am.c | 25 ++++++++++---------------\n 1 file changed, 10 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex baab951ed..e08f03326 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1380,40 +1380,35 @@ static int get_mail_commit_oid(struct object_id *commit_id, const char *mail)\n  */\n static void get_commit_info(struct am_state *state, struct commit *commit)\n {\n-\tconst char *buffer, *ident_line, *author_date, *msg;\n+\tconst char *buffer, *ident_line, *msg;\n \tsize_t ident_len;\n \tstruct ident_split ident_split;\n-\tstruct strbuf sb = STRBUF_INIT;\n \n \tbuffer = logmsg_reencode(commit, NULL, get_commit_output_encoding());\n \n \tident_line = find_commit_header(buffer, \"author\", &ident_len);\n \n-\tif (split_ident_line(&ident_split, ident_line, ident_len) < 0) {\n-\t\tstrbuf_add(&sb, ident_line, ident_len);\n-\t\tdie(_(\"invalid ident line: %s\"), sb.buf);\n-\t}\n+\tif (split_ident_line(&ident_split, ident_line, ident_len) < 0)\n+\t\tdie(_(\"invalid ident line: %.*s\"), (int)ident_len, ident_line);\n \n \tassert(!state->author_name);\n \tif (ident_split.name_begin) {\n-\t\tstrbuf_add(&sb, ident_split.name_begin,\n-\t\t\tident_split.name_end - ident_split.name_begin);\n-\t\tstate->author_name = strbuf_detach(&sb, NULL);\n+\t\tstate->author_name =\n+\t\t\txmemdupz(ident_split.name_begin,\n+\t\t\t\t ident_split.name_end - ident_split.name_begin);\n \t} else\n \t\tstate->author_name = xstrdup(\"\");\n \n \tassert(!state->author_email);\n \tif (ident_split.mail_begin) {\n-\t\tstrbuf_add(&sb, ident_split.mail_begin,\n-\t\t\tident_split.mail_end - ident_split.mail_begin);\n-\t\tstate->author_email = strbuf_detach(&sb, NULL);\n+\t\tstate->author_email =\n+\t\t\txmemdupz(ident_split.mail_begin,\n+\t\t\t\t ident_split.mail_end - ident_split.mail_begin);\n \t} else\n \t\tstate->author_email = xstrdup(\"\");\n \n-\tauthor_date = show_ident_date(&ident_split, DATE_MODE(NORMAL));\n-\tstrbuf_addstr(&sb, author_date);\n \tassert(!state->author_date);\n-\tstate->author_date = strbuf_detach(&sb, NULL);\n+\tstate->author_date = xstrdup(show_ident_date(&ident_split, DATE_MODE(NORMAL)));\n \n \tassert(!state->msg);\n \tmsg = strstr(buffer, \"\\n\\n\");\n-- \n2.13.0.rc0.465.ga1e654229\n\n"},{"id":"318034","messageId":"20170427032830.sushxlvqb44px7lx@sigill.intra.peff.net","threadId":"45815","inReplyTo":"20170427032503.rgz3bnctnxdn3hin@sigill.intra.peff.net","subject":"[PATCH 3/3] am: shorten ident_split variable name in get_commit_info()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-27T03:28:31Z","receivedAt":"2017-04-27T03:28:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The local ident_split variable is often mentioned three\ntimes per line when dealing with its begin/end pointer\npairs. Let's use a shorter name which lets us get rid of\nsome long lines.  Since this is a short self-contained\nfunction, readability doesn't suffer.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/am.c | 20 +++++++++-----------\n 1 file changed, 9 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex e08f03326..2aed3ef01 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1382,33 +1382,31 @@ static void get_commit_info(struct am_state *state, struct commit *commit)\n {\n \tconst char *buffer, *ident_line, *msg;\n \tsize_t ident_len;\n-\tstruct ident_split ident_split;\n+\tstruct ident_split id;\n \n \tbuffer = logmsg_reencode(commit, NULL, get_commit_output_encoding());\n \n \tident_line = find_commit_header(buffer, \"author\", &ident_len);\n \n-\tif (split_ident_line(&ident_split, ident_line, ident_len) < 0)\n+\tif (split_ident_line(&id, ident_line, ident_len) < 0)\n \t\tdie(_(\"invalid ident line: %.*s\"), (int)ident_len, ident_line);\n \n \tassert(!state->author_name);\n-\tif (ident_split.name_begin) {\n+\tif (id.name_begin)\n \t\tstate->author_name =\n-\t\t\txmemdupz(ident_split.name_begin,\n-\t\t\t\t ident_split.name_end - ident_split.name_begin);\n-\t} else\n+\t\t\txmemdupz(id.name_begin, id.name_end - id.name_begin);\n+\telse\n \t\tstate->author_name = xstrdup(\"\");\n \n \tassert(!state->author_email);\n-\tif (ident_split.mail_begin) {\n+\tif (id.mail_begin)\n \t\tstate->author_email =\n-\t\t\txmemdupz(ident_split.mail_begin,\n-\t\t\t\t ident_split.mail_end - ident_split.mail_begin);\n-\t} else\n+\t\t\txmemdupz(id.mail_begin, id.mail_end - id.mail_begin);\n+\telse\n \t\tstate->author_email = xstrdup(\"\");\n \n \tassert(!state->author_date);\n-\tstate->author_date = xstrdup(show_ident_date(&ident_split, DATE_MODE(NORMAL)));\n+\tstate->author_date = xstrdup(show_ident_date(&id, DATE_MODE(NORMAL)));\n \n \tassert(!state->msg);\n \tmsg = strstr(buffer, \"\\n\\n\");\n-- \n2.13.0.rc0.465.ga1e654229\n"},{"id":"318045","messageId":"xmqqd1byo196.fsf@gitster.mtv.corp.google.com","threadId":"45815","inReplyTo":"20170427032503.rgz3bnctnxdn3hin@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] fix memory leak in git-am","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-27T05:41:57Z","receivedAt":"2017-04-27T05:43:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This was noticed by Coverity. The leak isn't new, but I think was\n> \"re-found\" by Coverity because some nearby code did an unrelated\n> s/sha1/oid/ change, throwing off its heuristics.\n>\n> I also checked whether this was in Dscho's big pack of Coverity fixups\n> from earlier today, and it's not.\n>\n> The first one is the actual fix. The second one is pure cleanup, but I\n> think worth doing. The final one is arguably just churn, and maybe\n> people even like the end result less. I'm OK to drop it.\n>\n>   [1/3]: am: fix commit buffer leak in get_commit_info()\n>   [2/3]: am: simplify allocations in get_commit_info()\n>   [3/3]: am: shorten ident_split variable name in get_commit_info()\n\nAll looked sensible.  Thanks.\n\n\n"}]}