Re: [PATCH 2/2] bugreport: slightly better memory management

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 7, 2021, 21:56 UTC
Message-ID: <xmqqy28882xg.fsf@gitster.g>
In-Reply-To: <20210904021231.88534-3-carenas@gmail.com>

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

Show 6 quoted lines

> 238b439d69 (bugreport: add tool to generate debugging info, 2020-04-16)
> introduces an UNLEAK for a strbuf that contains the buffer that gets
> flushed to disk earlier, instead of simply cleaning the buffer.
>
> do so, and while at it, move the free() call for another temporary string
> closer to its creator, so it is easier to keep track of.

'do so' -> 'Do so'.

Show 15 quoted lines

>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  builtin/bugreport.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index 17042381c3..a9bedde1e8 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -152,6 +152,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
>  	strbuf_addstr(&report_path, "git-bugreport-");
>  	strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);
>  	strbuf_addstr(&report_path, ".txt");
> +	free(prefixed_filename);

Correct, but it can be raised even further. We can free it after we addstr to report_path.

I looked at the existing callers of prefix_filename(), hoping that many of them might take it in a strbuf they have, in which case we may be able to expose an alternative interface to take a caller supplied strbuf to clean this one up. But it seems this is the only one, so "store in a temporary, strbuf_addstr it, and immediately free the temporary" here would be good.

Show 7 quoted lines

>  	switch (safe_create_leading_directories(report_path.buf)) {
>  	case SCLD_OK:
> @@ -181,6 +182,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
>  		die_errno(_("unable to write to %s"), report_path.buf);
>  
>  	close(report);
> +	strbuf_release(&buffer);

We are done with the strbuf once write_in_full() returns, but this is close enough.

Show 8 quoted lines

> @@ -191,8 +193,6 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
>  	fprintf(stderr, _("Created new report at '%s'.\n"),
>  		user_relative_path);
>  
> -	free(prefixed_filename);
> -	UNLEAK(buffer);
>  	UNLEAK(report_path);
>  	return !!launch_editor(report_path.buf, NULL, NULL);

Having reviewed all, I am not sure if my reaction is "good, now we are cleaner" or "meh, for the same reason why report_path can be left alive, it is fine to leave buffer alive, too".