{"thread":{"id":"47399","subject":"[PATCH] am: release strbuf after use in split_mail_mbox()","startedAt":"2017-12-07T20:20:41Z","lastAt":"2017-12-07T21:21:13Z","messageCount":2,"participants":["René Scharfe","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"334359","messageId":"ad0af192-1bd4-eee2-d6e8-2f1776e87428@web.de","threadId":"47399","inReplyTo":null,"subject":"[PATCH] am: release strbuf after use in split_mail_mbox()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-12-07T20:20:19Z","receivedAt":"2017-12-07T20:20:41Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Signed-off-by: Rene Scharfe <l.s.r@web.de>\n---\n builtin/am.c | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 02853b3e05..1ac044da2e 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -708,6 +708,7 @@ static int split_mail_mbox(struct am_state *state, const char **paths,\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct strbuf last = STRBUF_INIT;\n+\tint ret;\n \n \tcp.git_cmd = 1;\n \targv_array_push(&cp.args, \"mailsplit\");\n@@ -721,13 +722,16 @@ static int split_mail_mbox(struct am_state *state, const char **paths,\n \targv_array_push(&cp.args, \"--\");\n \targv_array_pushv(&cp.args, paths);\n \n-\tif (capture_command(&cp, &last, 8))\n-\t\treturn -1;\n+\tret = capture_command(&cp, &last, 8);\n+\tif (ret)\n+\t\tgoto exit;\n \n \tstate->cur = 1;\n \tstate->last = strtol(last.buf, NULL, 10);\n \n-\treturn 0;\n+exit:\n+\tstrbuf_release(&last);\n+\treturn ret ? -1 : 0;\n }\n \n /**\n-- \n2.15.1\n"},{"id":"334373","messageId":"20171207212030.GC12850@sigill.intra.peff.net","threadId":"47399","inReplyTo":"ad0af192-1bd4-eee2-d6e8-2f1776e87428@web.de","subject":"Re: [PATCH] am: release strbuf after use in split_mail_mbox()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-07T21:20:30Z","receivedAt":"2017-12-07T21:21:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 07, 2017 at 09:20:19PM +0100, René Scharfe wrote:\n\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n>  builtin/am.c | 10 +++++++---\n>  1 file changed, 7 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/am.c b/builtin/am.c\n> index 02853b3e05..1ac044da2e 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -708,6 +708,7 @@ static int split_mail_mbox(struct am_state *state, const char **paths,\n>  {\n>  \tstruct child_process cp = CHILD_PROCESS_INIT;\n>  \tstruct strbuf last = STRBUF_INIT;\n> +\tint ret;\n>  \n>  \tcp.git_cmd = 1;\n>  \targv_array_push(&cp.args, \"mailsplit\");\n> @@ -721,13 +722,16 @@ static int split_mail_mbox(struct am_state *state, const char **paths,\n>  \targv_array_push(&cp.args, \"--\");\n>  \targv_array_pushv(&cp.args, paths);\n>  \n> -\tif (capture_command(&cp, &last, 8))\n> -\t\treturn -1;\n> +\tret = capture_command(&cp, &last, 8);\n> +\tif (ret)\n> +\t\tgoto exit;\n\nLooks good to me.\n\nCoupled with your third patch, it made me wonder if capture_command()\nshould free the strbuf when it sees an error. But probably not. Some\ncallers would want to see the output even from a failing command (and\ndoubly for pipe_command(), which may capture stderr).\n\n(And anyway, it wouldn't make this case any simpler; we were leaking in\nthe success code path, too!)\n\n-Peff\n"}]}