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

[Andrzej Hunt <andrzej@ahunt.org>]

> On 21 Oct 2021, at 13:42, Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.

I’m curious - to me this seems like a compiler/sanitiser bug, can it also be reproduced with clang, or even newer versions of gcc? Similarly, can it be reproduced with your gcc version, using ASAN+LSAN (as opposed to LSAN by itself)? I remember seeing some false positives in the past for some permutations of compilers and sanitisers, but I’ve lost track of the details.

These kinds of fixes seem noisy if it’s just to work around what appears to be a bug (and to be philosophical: we wouldn’t want to do the same for all “leaks” up the call stack if a specific compiler complained about them after a die() - after all there will be many more allocations that didn’t get free’d floating around - so why is it OK for these “leaks”?)

If it this is a gcc-specific or LSAN-only-specific bug, I would suggest giving up on that combination for leak checking instead of adding such workarounds. After all the code seems correct - and while such compiler-specific workarounds are probably justified for user-visible bugs, these fixes seem to just be silencing a non-issue that only happens with what is probably a  “broken” setup?

(From what I can remember, I never saw these when running t00* using clang 11 or 12, always using LSAN+ASAN, but that was a while back. I’ve not spent much time using gcc.)

> 
> See 956d2e4639b (tests: add a test mode for SANITIZE=leak, run it in
> CI, 2021-09-23) for the commit that marked t0017 for testing with
> SANITIZE=leak, and c150064dbe2 (leak tests: run various built-in tests
> in t00*.sh SANITIZE=leak, 2021-10-12) for t0001 (currently in "next").
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> config.c | 4 +++-
> refs.c   | 5 ++++-
> 2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/config.c b/config.c
> index 2dcbe901b6b..93979d39b21 100644
> --- a/config.c
> +++ b/config.c
> @@ -159,11 +159,13 @@ static int handle_path_include(const char *path, struct config_include_data *inc
>    }
> 
>    if (!access_or_die(path, R_OK, 0)) {
> -        if (++inc->depth > MAX_INCLUDE_DEPTH)
> +        if (++inc->depth > MAX_INCLUDE_DEPTH) {
> +            free(expanded);
>            die(_(include_depth_advice), MAX_INCLUDE_DEPTH, path,
>                !cf ? "<unknown>" :
>                cf->name ? cf->name :
>                "the command line");
> +        }
>        ret = git_config_from_file(git_config_include, path, inc);
>        inc->depth--;
>    }
> diff --git a/refs.c b/refs.c
> index 7f019c2377e..52929286032 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -590,8 +590,11 @@ char *repo_default_branch_name(struct repository *r, int quiet)
>    }
> 
>    full_ref = xstrfmt("refs/heads/%s", ret);
> -    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);
> 
>    return ret;
> -- 
> 2.33.1.1486.gb2bc4955b90
>