{"thread":{"id":"45643","subject":"[PATCH] grep: plug a trivial memory leak","startedAt":"2017-04-09T19:59:16Z","lastAt":"2017-04-10T17:17:38Z","messageCount":2,"participants":["Ævar Arnfjörð Bjarmason","Brandon Williams"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"316451","messageId":"20170409195900.32701-1-avarab@gmail.com","threadId":"45643","inReplyTo":null,"subject":"[PATCH] grep: plug a trivial memory leak","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-09T19:59:00Z","receivedAt":"2017-04-09T19:59:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the cleanup phase for the grep command to free the pathspec\nstruct that's allocated earlier in the same block, and used just a few\nlines earlier.\n\nWith \"grep hi README.md\" valgrind reports a loss of 239 bytes now,\ndown from 351.\n\nThe relevant --num-callers=40 --leak-check=full --show-leak-kinds=all\nbacktrace is:\n\n    [...] 187 (112 direct, 75 indirect) bytes in 1 blocks are definitely lost in loss record 70 of 110\n    [...]    at 0x4C2BBAF: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)\n    [...]    by 0x60B339: do_xmalloc (wrapper.c:59)\n    [...]    by 0x60B2F6: xmalloc (wrapper.c:86)\n    [...]    by 0x576B37: parse_pathspec (pathspec.c:652)\n    [...]    by 0x4519F0: cmd_grep (grep.c:1215)\n    [...]    by 0x4062EF: run_builtin (git.c:371)\n    [...]    by 0x40544D: handle_builtin (git.c:572)\n    [...]    by 0x4060A2: run_argv (git.c:624)\n    [...]    by 0x4051C6: cmd_main (git.c:701)\n    [...]    by 0x4C5901: main (common-main.c:43)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nSince pretty much any non-trivial git command leaks because it skips\ncleanup are patches in this category even accepted? Worth a try...\n\n builtin/grep.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 65070c52fc..3ffb5b4e81 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1299,6 +1299,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\thit |= wait_all();\n \tif (hit && show_in_pager)\n \t\trun_pager(&opt, prefix);\n+\tclear_pathspec(&pathspec);\n \tfree_grep_patterns(&opt);\n \treturn !hit;\n }\n-- \n2.11.0\n\n"},{"id":"316503","messageId":"20170410171719.GB113751@google.com","threadId":"45643","inReplyTo":"20170409195900.32701-1-avarab@gmail.com","subject":"Re: [PATCH] grep: plug a trivial memory leak","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-10T17:17:19Z","receivedAt":"2017-04-10T17:17:38Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 04/09, Ævar Arnfjörð Bjarmason wrote:\n> Change the cleanup phase for the grep command to free the pathspec\n> struct that's allocated earlier in the same block, and used just a few\n> lines earlier.\n> \n> With \"grep hi README.md\" valgrind reports a loss of 239 bytes now,\n> down from 351.\n> \n> The relevant --num-callers=40 --leak-check=full --show-leak-kinds=all\n> backtrace is:\n> \n>     [...] 187 (112 direct, 75 indirect) bytes in 1 blocks are definitely lost in loss record 70 of 110\n>     [...]    at 0x4C2BBAF: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)\n>     [...]    by 0x60B339: do_xmalloc (wrapper.c:59)\n>     [...]    by 0x60B2F6: xmalloc (wrapper.c:86)\n>     [...]    by 0x576B37: parse_pathspec (pathspec.c:652)\n>     [...]    by 0x4519F0: cmd_grep (grep.c:1215)\n>     [...]    by 0x4062EF: run_builtin (git.c:371)\n>     [...]    by 0x40544D: handle_builtin (git.c:572)\n>     [...]    by 0x4060A2: run_argv (git.c:624)\n>     [...]    by 0x4051C6: cmd_main (git.c:701)\n>     [...]    by 0x4C5901: main (common-main.c:43)\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n> \n> Since pretty much any non-trivial git command leaks because it skips\n> cleanup are patches in this category even accepted? Worth a try...\n\nSome may argue that the program will terminate soon and that the leak is\nbounded etc., but I think that plugging leaks is a good thing.  As such,\nI like this patch :)\n\n> \n>  builtin/grep.c | 1 +\n>  1 file changed, 1 insertion(+)\n> \n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 65070c52fc..3ffb5b4e81 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -1299,6 +1299,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>  \t\thit |= wait_all();\n>  \tif (hit && show_in_pager)\n>  \t\trun_pager(&opt, prefix);\n> +\tclear_pathspec(&pathspec);\n>  \tfree_grep_patterns(&opt);\n>  \treturn !hit;\n>  }\n> -- \n> 2.11.0\n> \n\n-- \nBrandon Williams\n"}]}