Re: [PATCH 1/2] log: UNLEAK rev to silence a large number of leaks

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 20, 2021, 06:06 UTC
Message-ID: <CAPig+cT-ajKsoj19ChPnkNByf-6P-vX=SG0NmgYt8CXyNH8y-w@mail.gmail.com>
In-Reply-To: <YUes7yxKHKW7cXcl@carlos-mbp.lan>

On Sun, Sep 19, 2021 at 5:34 PM Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote:

Show 8 quoted lines

> 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().

Show 18 quoted lines

> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> 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);