| Message ID | 20210904021231.88534-3-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] bugreport: avoid duplicating options in usage() | expand |
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > 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'. > > 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. > 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. > @@ -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".
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); 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 want to print the path relative to the user, but we still need the @@ -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); }
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. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- builtin/bugreport.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)