Re: [PATCH 2/2] bugreport: slightly better memory management
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.
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".
Bagas Sanjaya
