Re: [PATCH v2 4/6] tr2: fix memory leak & logic error in 2f732bf15e6

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Subject: Re: [PATCH v2 4/6] tr2: fix memory leak & logic error in 2f732bf15e6

Please remember to write your commit titles to help readers of "git
shortlog".  This commit fixes get_ancestry_names() function.

    get_ancestry_names(): leave the parent list empty upon failure

or something like that.  The rest of the proposed log message looks
perfect and so do the changes to the code.

> In a subsequent commit I'll be replacing most of this code to log N
> parents, but let's first fix bugs introduced in the recent
> 2f732bf15e6 (tr2: log parent process name, 2021-07-21).
>
> It was using the strbuf_read_file() in the wrong way, its return value
> is either a length or a negative value on error. If we didn't have a
> procfs, or otherwise couldn't access it we'd end up pushing an empty
> string to the trace2 ancestry array.
>
> It was also using the strvec_push() API the wrong way. That API always
> does an xstrdup(), so by detaching the strbuf here we'd leak
> memory. Let's instead pass in our pointer for strvec_push() to
> xstrdup(), and then free our own strbuf. I do have some WIP changes to
> make strvec_push_nodup() non-static, which makes this and some other
> callsites nicer, but let's just follow the prevailing pattern of using
> strvec_push() for now.
>
> We'll also need to free that "procfs_path" strbuf whether or not
> strbuf_read_file() succeeds, which was another source of memory leaks
> in 2f732bf15e6, i.e. we'd leak that memory as well if we weren't on a
> system where we could read the file from procfs.
>
> Let's move all the freeing of the memory to the end of the
> function. If we're still at STRBUF_INIT with "name" due to not haven
> taken the branch where the strbuf_read_file() succeeds freeing it is
> redundant, so we could move it into the body of the "if", but just
> handling freeing the same way for all branches of the function makes
> it more readable.
>
> In combination with the preceding commit this makes all of
> t[0-9]*trace2*.sh pass under SANITIZE=leak on Linux.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  compat/linux/procinfo.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/compat/linux/procinfo.c b/compat/linux/procinfo.c
> index 62f8aaed4cc..bd01f017bc7 100644
> --- a/compat/linux/procinfo.c
> +++ b/compat/linux/procinfo.c
> @@ -18,12 +18,14 @@ static void get_ancestry_names(struct strvec *names)
>  
>  	/* try to use procfs if it's present. */
>  	strbuf_addf(&procfs_path, "/proc/%d/comm", getppid());
> -	if (strbuf_read_file(&name, procfs_path.buf, 0)) {
> -		strbuf_release(&procfs_path);
> +	if (strbuf_read_file(&name, procfs_path.buf, 0) > 0) {
>  		strbuf_trim_trailing_newline(&name);
> -		strvec_push(names, strbuf_detach(&name, NULL));
> +		strvec_push(names, name.buf);
>  	}
>  
> +	strbuf_release(&procfs_path);
> +	strbuf_release(&name);
> +
>  	return;
>  }