[PATCH] tests: opt "git --config-env" test out of SANITIZE=leak

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

In 676063016ad (leak tests: mark some config tests as passing with
SANITIZE=leak, 2021-10-31) t1300*.sh as a whole was marked as passing
with SANITIZE=leak, but as reported[1] certain compiler options and
optimization levels will re-arrange the generated code to have the
sanitizer fire.

This issue is reproducible with e.g. clang v12 with CFLAGS=-O2. The
issue is that -fsanitize=leak will combine with other optimization
options, and as we eliminate functions, variables etc. the
-fsanitize=leak criteria for considering something a leak might
effectively change.

This is arguably not a compiler issue or bug, as the very notion of a
variable being "in scope" as far as the leak checker is concerned is
fuzzy at best in the face of compiler optimizations.

A similar issue came up before related to the config.c code in
[2]. There the consensus seemed to be to avoid hacks in the C code to
work around these compiler edge cases. In this case however skipping
one test is easy enough. We can deal with these "!SANITIZE_LEAK"
issues later, when we have fewer overall leaks.

1. https://lore.kernel.org/git/Yxl91jfycCo7O7Pp@coredump.intra.peff.net/
2. https://lore.kernel.org/git/patch-v3-5.6-9a44204c4c9-20211022T175227Z-avarab@gmail.com/
---

On Thu, Sep 08 2022, Jeff King wrote:

> If I run:
>
>   make SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=true CFLAGS=-O0
>
> locally, then all tests pass or are skipped. But if switch to -O2, then
> t1300 starts failing!
>
> The command that supposedly leaks is this:
>
>   git --config-env=foo.flag= config --bool foo.flag
>
> and the backtrace looks like:
>
>   Direct leak of 9 byte(s) in 1 object(s) allocated from:
>       #0 0x56195ae6dc62 in __interceptor_malloc (git+0x78c62) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)
>       #1 0x56195b0ad6b6 in do_xmalloc wrapper.c
>       #2 0x56195b0ad7bd in xmemdupz (git+0x2b87bd) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)
>       #3 0x56195af6f8d0 in git_config_push_env (git+0x17a8d0) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)
>       #4 0x56195ae71689 in handle_options git.c
>       #5 0x56195ae7060f in cmd_main (git+0x7b60f) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)
>       #6 0x56195af370c6 in main (git+0x1420c6) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)
>       #7 0x7f45b9029209 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
>       #8 0x7f45b90292bb in __libc_start_main csu/../csu/libc-start.c:389:3
>       #9 0x56195ae420c0 in _start (git+0x4d0c0) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)
>
> But here's the weird part. The function looks like this:
>
>   void git_config_push_env(const char *spec)
>   {
>           char *key;
>           const char *env_name;
>           const char *env_value;
>   
>           env_name = strrchr(spec, '=');
>           if (!env_name)
>                   die(_("invalid config format: %s"), spec);
>           key = xmemdupz(spec, env_name - spec);
>           env_name++;
>           if (!*env_name)
>                   die(_("missing environment variable name for configuration '%.*s'"),
>                       (int)(env_name - spec - 1), spec);
>   
>           env_value = getenv(env_name);
>           if (!env_value)
>                   die(_("missing environment variable '%s' for configuration '%.*s'"),
>                       env_name, (int)(env_name - spec - 1), spec);
>   
>           git_config_push_split_parameter(key, env_value);
>           free(key);
>   }
>
> And it is complaining that we've leaked "key". But as you can see, we
> always free it. The problem is that for this particular invocation we
> die("missing environment variable name"), so of course we don't make it
> to the free(). Normally this is OK, though. The "key" variable is still
> on the stack, so the leak-checker should realize that it's still
> reachable.

[Re-arranged your E-Mail a bit for the purposes of the flow of this
reply]

> Maybe this is a known problem, but it was certainly surprising to me.

It is surprising, but it's a (at least to me) known "issue". See
e.g. https://lore.kernel.org/git/patch-v3-5.6-9a44204c4c9-20211022T175227Z-avarab@gmail.com/
for a previous discussion between us. I took the consensus there to be
that we shouldn't bend over backwards to worry about this.

I have been diligently ensuring that I don't mark tests as leak-free
if they "pass", but then "fail" on higher optimization levels.

But I see this one slipped through because I locally changed my
default compiler from "clang" to "gcc" (again) a while ago, and thus
missed testing clang at higher optimization levels. Usually it's GCC
that's unhappy about this sort of thing.

> But if you run this in a debugger, you'll find that under -O2 the "key"
> variable has been optimized out! So LSan is producing the wrong result
> due to the optimization. It doesn't know that "key" is conceptually
> still reachable.
> [...]

I don't know if we (or the compiler implementors) can call it the
"wrong" result. Just as some warnings only pop up depending on
optimization levels, this behavior also depends on it.

I've seen quite a few of these around die() codepaths. Presumably some
re-arranging/eliminaton of the code around that NORETURN codepath...

> [...]
> I'm not sure if we should do anything about it or not. It doesn't seem
> to trigger in CI, even though I don't see us taking any steps there to
> use -O0 or similar. So we can perhaps ignore it for now, and this
> message can serve as a warning. But if we think LSan isn't reliable
> under higher optimizations, we could perhaps teach the Makefile to
> prefer -O0 when it sees SANITIZE=leak.

I think it's good to not force -O0 with SANITIZE=leak, but this is
annoying, so let's do the below change. With it the tests pass for me
with clang under -O2/-O3.

FWIW if you play with the "check" mode you'll probably also find that
we have some tests that are "flaky" as far as the leak checking is
concerned ("t2012 t6415 t6435", I think, at least I skip those locally
when using it). IIRC some of that also depends on -On.

 t/t1300-config.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index c6661e61af5..a4953881db6 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1398,7 +1398,7 @@ test_expect_success 'git --config-env with missing value' '
 	grep "invalid config format: config" error
 '
 
-test_expect_success 'git --config-env fails with invalid parameters' '
+test_expect_success !SANITIZE_LEAK 'git --config-env fails with invalid parameters' '
 	test_must_fail git --config-env=foo.flag config --bool foo.flag 2>error &&
 	test_i18ngrep "invalid config format: foo.flag" error &&
 	test_must_fail git --config-env=foo.flag= config --bool foo.flag 2>error &&
-- 
2.37.3.1492.gbb5af15b83b