On Sun, Sep 19, 2021 at 5:34 PM Carlo Marcelo Arenas Belón
<carenas@xxxxxxxxx> wrote:
> Subject: [PATCH] revision: remove dup() of name in add_rev_cmdline()
>
> df835d3a0c (add_rev_cmdline(): make a copy of the name argument,
> 2013-05-25) adds it, probably introducing a leak.
>
> All names we will ever get will either come from the commandline
> or be pointers to a static buffer in hex.c, so it is safe not to
> xstrdup and clean them up (just like the struct object *item).
I haven't been following this thread closely, but the mention of the
static buffer in hex.c invalidates the premise of this patch, as far
as I can tell. The "static buffer" is actually a ring of four buffers
which oid_to_hex() uses, one after another, into which it formats an
OID as hex. This allows a caller to format up to -- and only up to --
four OIDs without worrying about allocating its own memory for the hex
result. Beyond four, the caller can't use oid_to_hex() without doing
some sort of memory management itself, whether that be duplicating the
result of oid_to_hex() or by allocating its own buffers and calling
oid_to_hex_r() instead.
In this particular case, one of the callers of add_rev_cmdline() is
add_rev_cmdline_list(), which does this:
while (commit_list) {
...
add_rev_cmdline(..., oid_to_hex(...), ...);
...
}
which may call add_rev_cmdline() any number of times, quite possibly
more than four.
Therefore (if I'm reading this correctly), it is absolutely correct
for add_rev_cmdline() to be duplicating that string to ensure that the
hexified OID value remains valid, and incorrect for this patch to be
removing the call to xstrdup().
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@xxxxxxxxx>
> ---
> diff --git a/revision.c b/revision.c
> @@ -1481,7 +1480,7 @@ static void add_rev_cmdline(struct rev_info *revs,
> info->rev[nr].item = item;
> - info->rev[nr].name = xstrdup(name);
> + info->rev[nr].name = name;
> info->rev[nr].whence = whence;
> @@ -1490,10 +1489,6 @@ static void add_rev_cmdline(struct rev_info *revs,
> static void clear_rev_cmdline(struct rev_info *revs)
> {
> struct rev_cmdline_info *info = &revs->cmdline;
> - size_t i, nr = info->nr;
> -
> - for (i = 0; i < nr; i++)
> - free(info->rev[i].name);
>
> FREE_AND_NULL(info->rev);
