Re: linux-leaks CI failure on master

From: Jeff King <peff@peff.net>
Date: Oct 18, 2022, 19:40 UTC
Message-ID: <Y08BPbWBj7SNluXq@coredump.intra.peff.net>
In-Reply-To: <Y07yeEQu+C7AH7oN@nand.local>

On Tue, Oct 18, 2022 at 02:37:44PM -0400, Taylor Blau wrote:

Show 6 quoted lines

> After Junio pushed out the tags for v2.38.1 and friends, I noticed that
> our linux-leaks CI job is failing t1300.104 and t1300.109, claiming that
> there is a leak here:
> [...]
>     #14 0x55ba233acb36 in alias_lookup alias.c:35
>     #15 0x55ba232d0748 in handle_alias git.c:346

These are the interesting part of the trace. alias_lookup() returns an allocated string, and then split_cmdline() fails, so we call die() with:

  fatal: bad alias.split-cmdline-fix string: unclosed quote

So yeah, I think it's probably the same issue as discussed previously: the compiler presumably puts alias_string into a register, and then clobbers the register when calling die(), because it knows we're never coming back.

> I can't reproduce the failure locally with gcc (Debian 10.3.0-15)
> 10.3.0, but Victoria (CC'd) can reproduce the failure with 9.4.0.
> Interestingly, the failure only appears when compiling with `-O2`, but
> not `-O0` or `-O1`.

I can't reproduce on debian unstable using any of gcc 9-12 (but note gcc-9 here is 9.5.0). But then, I had trouble convincing gcc to find _actual_ leaks with lsan. Clang is much more reliable for me, but it turns up only the failure in t1300.135 that I reported earlier.

But given the trace above plus the findings on gcc 9.4.0, I feel pretty confident saying this is another instance of the same problem.

> I'm not sure if I'm content to treat the 9.4.0 behavior as a compiler
> bug, but definitely running the linux-leaks build with `-O2` is
> suspicious.

It's definitely not a compiler bug. What the optimizer is doing is perfectly reasonable; it's just that the leak checker interacts badly with it.

Show 7 quoted lines

> I suppose we could temporarily mark t1300 as not passing with
> SANITIZE=leak turned on, but I tend to agree with Peff that that feels
> like a hack working around compiler behavior, that will ultimately
> result in us playing whack-a-mole.
> 
> So my preference would be to run the linux-leaks build with `-O0` in its
> CFLAGS, optionally with a newer compiler if one is available for Focal.

Yes, I still think disabling optimizations is the best path forward. Not just to avoid whack-a-mole, but this is also something we'd eventually need to confront when the code base really is leak-free.

I don't think there's any need for a newer compiler. While the optimizer behavior may change between versions, none of what we've seen is any compiler being _wrong_, just different.

As a lesser change, I suspect that making NORETURN a noop in leak-checking builds would help in practice, because the compiler wouldn't realize that die() doesn't return. But it's not foolproof (the same thing might trigger with a direct exit() call, or one that becomes direct via inlining). Using -O0 is the more complete fix, and IMHO it's not important to try to get optimal performance during the leak-checking test run.

-Peff