On Thu, 21 Oct 2021 at 13:43, Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote:
>
> 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.
> - 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
