[PATCH] grep: plug a trivial memory leak

Ævar Arnfjörð Bjarmason· Apr 9, 2017, 19:59 UTC · lore

Change the cleanup phase for the grep command to free the pathspec struct that's allocated earlier in the same block, and used just a few lines earlier.

With "grep hi README.md" valgrind reports a loss of 239 bytes now, down from 351.

The relevant --num-callers=40 --leak-check=full --show-leak-kinds=all backtrace is:

    [...] 187 (112 direct, 75 indirect) bytes in 1 blocks are definitely lost in loss record 70 of 110
    [...]    at 0x4C2BBAF: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    [...]    by 0x60B339: do_xmalloc (wrapper.c:59)
    [...]    by 0x60B2F6: xmalloc (wrapper.c:86)
    [...]    by 0x576B37: parse_pathspec (pathspec.c:652)
    [...]    by 0x4519F0: cmd_grep (grep.c:1215)
    [...]    by 0x4062EF: run_builtin (git.c:371)
    [...]    by 0x40544D: handle_builtin (git.c:572)
    [...]    by 0x4060A2: run_argv (git.c:624)
    [...]    by 0x4051C6: cmd_main (git.c:701)
    [...]    by 0x4C5901: main (common-main.c:43)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Since pretty much any non-trivial git command leaks because it skips cleanup are patches in this category even accepted? Worth a try...

 builtin/grep.c | 1 +
 1 file changed, 1 insertion(+)

Show changes to builtin/grep.c +1 −0

diff --git a/builtin/grep.c b/builtin/grep.c
index 65070c52fc..3ffb5b4e81 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1299,6 +1299,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		hit |= wait_all();
 	if (hit && show_in_pager)
 		run_pager(&opt, prefix);
+	clear_pathspec(&pathspec);
 	free_grep_patterns(&opt);
 	return !hit;
 }

-- 
2.11.0

Brandon Williams· Apr 10, 2017, 17:17 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] grep: plug a trivial memory leak

On 04/09, Ævar Arnfjörð Bjarmason wrote:

Show 27 quoted lines

> Change the cleanup phase for the grep command to free the pathspec
> struct that's allocated earlier in the same block, and used just a few
> lines earlier.
> 
> With "grep hi README.md" valgrind reports a loss of 239 bytes now,
> down from 351.
> 
> The relevant --num-callers=40 --leak-check=full --show-leak-kinds=all
> backtrace is:
> 
>     [...] 187 (112 direct, 75 indirect) bytes in 1 blocks are definitely lost in loss record 70 of 110
>     [...]    at 0x4C2BBAF: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>     [...]    by 0x60B339: do_xmalloc (wrapper.c:59)
>     [...]    by 0x60B2F6: xmalloc (wrapper.c:86)
>     [...]    by 0x576B37: parse_pathspec (pathspec.c:652)
>     [...]    by 0x4519F0: cmd_grep (grep.c:1215)
>     [...]    by 0x4062EF: run_builtin (git.c:371)
>     [...]    by 0x40544D: handle_builtin (git.c:572)
>     [...]    by 0x4060A2: run_argv (git.c:624)
>     [...]    by 0x4051C6: cmd_main (git.c:701)
>     [...]    by 0x4C5901: main (common-main.c:43)
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> 
> Since pretty much any non-trivial git command leaks because it skips
> cleanup are patches in this category even accepted? Worth a try...

Some may argue that the program will terminate soon and that the leak is bounded etc., but I think that plugging leaks is a good thing. As such, I like this patch :)

Show 19 quoted lines

> 
>  builtin/grep.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 65070c52fc..3ffb5b4e81 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -1299,6 +1299,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  		hit |= wait_all();
>  	if (hit && show_in_pager)
>  		run_pager(&opt, prefix);
> +	clear_pathspec(&pathspec);
>  	free_grep_patterns(&opt);
>  	return !hit;
>  }
> -- 
> 2.11.0
>

-- 
Brandon Williams

patchgrep: plug a trivial memory leak