Re: [PATCH] leak tests: free() before die for two API functions

From: Martin Ågren <martin.agren@gmail.com>
Date: Oct 21, 2021, 16:13 UTC
Message-ID: <CAN0heSqA4uqWahWKTa0ZLBCnFLG4jxEx+18aVQUieX0f6dzWMw@mail.gmail.com>
In-Reply-To: <patch-1.1-5a47bf2e9c9-20211021T114223Z-avarab@gmail.com>

On Thu, 21 Oct 2021 at 13:43, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 6 quoted lines

>
> Call free() just before die() in two API functions whose tests are
> asserted under SANITIZE=leak. Normally this would not be needed due to
> how SANITIZE=leak works, but in these cases my GCC version (10.2.1-6)
> will fail tests t0001 and t0017 under SANITIZE=leak depending on the
> optimization level.

Seems a bit unfortunate. I have to wonder why these in particular trigger this compiler bug or whatever it is, but oh well.

Show 7 quoted lines

> -       if (check_refname_format(full_ref, 0))
> +       if (check_refname_format(full_ref, 0)) {
> +               free(ret);
> +               free(full_ref);
>                 die(_("invalid branch name: %s = %s"), config_display_key, ret);
> +       }
>         free(full_ref);

This looks like use-after-free. Rather than complicating this by, e.g., first formatting the string, then freeing `ret`, then dying, could we -- if we really want this workaround -- make the workaround be `UNLEAK` instead?

Also, if we do something like this patch, I think we should try to avoid this free-before-die then being cargo-culted all across the codebase. How about

  UNLEAK(ret); /* work around compiler bug */
  UNLEAK(full_ref); /* work around compiler bug */

or something?

Martin