Hi Junio,
On Fri, 10 Feb 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@xxxxxx> writes:
>
> > diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> > index ff13e59e1db..84af2802f6f 100644
> > --- a/builtin/rev-parse.c
> > +++ b/builtin/rev-parse.c
> > @@ -545,6 +545,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> > unsigned int flags = 0;
> > const char *name = NULL;
> > struct object_context unused;
> > + struct strbuf buf = STRBUF_INIT;
> >
> > if (argc > 1 && !strcmp("--parseopt", argv[1]))
> > return cmd_parseopt(argc - 1, argv + 1, prefix);
> > @@ -599,7 +600,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> > if (!strcmp(arg, "--git-path")) {
> > if (!argv[i + 1])
> > die("--git-path requires an argument");
> > - puts(git_path("%s", argv[i + 1]));
> > + strbuf_reset(&buf);
> > + puts(relative_path(git_path("%s", argv[i + 1]),
> > + prefix, &buf));
> > i++;
> > continue;
> > }
> > @@ -821,8 +824,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> > continue;
> > }
> > if (!strcmp(arg, "--git-common-dir")) {
> > - const char *pfx = prefix ? prefix : "";
> > - puts(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));
> > + strbuf_reset(&buf);
> > + puts(relative_path(get_git_common_dir(),
> > + prefix, &buf));
> > continue;
> > }
> > if (!strcmp(arg, "--is-inside-git-dir")) {
> > @@ -845,7 +849,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> > die(_("Could not read the index"));
> > if (the_index.split_index) {
> > const unsigned char *sha1 = the_index.split_index->base_sha1;
> > - puts(git_path("sharedindex.%s", sha1_to_hex(sha1)));
> > + const char *path = git_path("sharedindex.%s", sha1_to_hex(sha1));
> > + strbuf_reset(&buf);
> > + puts(relative_path(path, prefix, &buf));
> > }
> > continue;
> > }
> > @@ -906,5 +912,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> > die_no_single_rev(quiet);
> > } else
> > show_default();
> > + strbuf_release(&buf);
>
> This uses "reset then use" pattern for repeated use of strbuf, and
> causes the string last held in the strbuf to leak on early return,
... which cannot happen due to the lack of an early return...
> which can be mitigated by using "use then reset" pattern. I.e.
>
> if (!strcmp(arg, "--git-common-dir")) {
> puts(relative_path(get_git_common_dir(),
> prefix, &buf));
> strbuf_reset(&buf);
> continue;
> }
>
> I'd think.
This would not release the memory, though:
#define strbuf_reset(sb) strbuf_setlen(sb, 0)
and
static inline void strbuf_setlen(struct strbuf *sb, size_t len)
{
if (len > (sb->alloc ? sb->alloc - 1 : 0))
die("BUG: strbuf_setlen() beyond buffer");
sb->len = len;
sb->buf[len] = '\0';
}
There is not a single free() statement there.
So the "use then reset" scheme would leak *just the same*.
> You'd still want to release it at the end anyway for good code hygiene,
> though.
Which I do.
Technically, this is not even necessary because all of the cmd_*()
functions are immediately followed by a call to exit(). Wasn't that the
genius idea in the early Git days, that we could simply get away with
sloppy memory management because the program exit()s shortly afterwards,
anyway? ;-)
In any case, I adjusted the commit message to clarify why the "reset then
use" scheme is correct here.
Ciao,
Johannes