[PATCH] Makefile: suppress annotated leaks with certain ASan options

[PATCH] Makefile: suppress annotated leaks with certain ASan options

[Taylor Blau]

When building with `SANITIZE=leak`, we define `SUPPRESS_ANNOTATED_LEAKS`
in order to make the `UNLEAK()` macro work (without the aforementioned
define, `UNLEAK()` is a noop). This is from `UNLEAK()`'s introduction in
0e5bba53af (add UNLEAK annotation for reducing leak false positives,
2017-09-08), where `UNLEAK()` is a noop for performance reasons unless
we are using the leak sanitizer.

However, it is possible to use the leak sanitizer without
`SANITIZE=leak`. This happens when building with `SANITIZE=address` and
enabling the leak sanitizer via the `ASAN_OPTIONS` variable (by
including the string "detect_leaks=1").

This renders `UNLEAK()` useless when doing `SANITIZE=address` builds
which also use the leak checker.

Update our Makefile to pretend as if `SANITIZE=leak` was given when
`SANITIZE=address` is given and the leak checker is enabled via
`ASAN_OPTIONS`.

Playing around with all five options (two spelling "enabled", two
spelling "disabled", and the empty set of options) yields the correct
behavior:

    for opt in '' detect_leaks=1 detect_leaks=true detect_leaks=0 detect_leaks=false
    do
      echo "==> ${opt:-(nothing)}"
      make -B builtin/add.o V=1 SANITIZE=address ASAN_OPTIONS="$opt" 2>&1 |
        grep -o -- '-DSUPPRESS_ANNOTATED_LEAKS'
    done

gives us:

    ==> (nothing)
    -DSUPPRESS_ANNOTATED_LEAKS
    ==> detect_leaks=1
    -DSUPPRESS_ANNOTATED_LEAKS
    ==> detect_leaks=true
    -DSUPPRESS_ANNOTATED_LEAKS
    ==> detect_leaks=0
    ==> detect_leaks=false

Making it possible to rely on `UNLEAK()` when implicitly using the leak
checker via SANITIZE=address builds.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
I found this while playing around with GitHub's ASan-enabled CI builds
for our internal fork following a merge with v2.38.3.

The check-chainlint recipe in t/Makefile started using "git diff" via
d00113ec34 (t/Makefile: apply chainlint.pl to existing self-tests,
2022-09-01), which triggered a leak in some of GitHub's custom code. I
was surprised when marking the variable with UNLEAK() didn't do the
trick, and ended up down this rabbit hole ;-).

 Makefile | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index db447d0738..b00bb8bd1e 100644
--- a/Makefile
+++ b/Makefile
@@ -1445,13 +1445,18 @@ ifneq ($(filter undefined,$(SANITIZERS)),)
 BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS
 endif
 ifneq ($(filter leak,$(SANITIZERS)),)
-BASIC_CFLAGS += -DSUPPRESS_ANNOTATED_LEAKS
-BASIC_CFLAGS += -O0
 SANITIZE_LEAK = YesCompiledWithIt
 endif
 ifneq ($(filter address,$(SANITIZERS)),)
 NO_REGEX = NeededForASAN
 SANITIZE_ADDRESS = YesCompiledWithIt
+ifeq ($(filter $(patsubst detect_leaks=%,%,$(ASAN_OPTIONS)),0 false),)
+SANITIZE_LEAK = YesViaASanOptions
+endif
+endif
+ifneq ($(SANITIZE_LEAK),)
+BASIC_CFLAGS += -DSUPPRESS_ANNOTATED_LEAKS
+BASIC_CFLAGS += -O0
 endif
 endif

--
2.38.0.16.g393fd4c6db

Re: [PATCH] Makefile: suppress annotated leaks with certain ASan options

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> However, it is possible to use the leak sanitizer without
> `SANITIZE=leak`. This happens when building with `SANITIZE=address` and
> enabling the leak sanitizer via the `ASAN_OPTIONS` variable (by
> including the string "detect_leaks=1").

Yuck.  I cannot tell if this falls into "don't do it then if it
hurts" or pretty common thing people do that is worth helping.

> Making it possible to rely on `UNLEAK()` when implicitly using the leak
> checker via SANITIZE=address builds.

But as long as you did all the work, sure, why not ;-).

> I found this while playing around with GitHub's ASan-enabled CI builds
> for our internal fork following a merge with v2.38.3.
>
> The check-chainlint recipe in t/Makefile started using "git diff" via
> d00113ec34 (t/Makefile: apply chainlint.pl to existing self-tests,
> 2022-09-01), which triggered a leak in some of GitHub's custom code. I
> was surprised when marking the variable with UNLEAK() didn't do the
> trick, and ended up down this rabbit hole ;-).

Thanks.  Will queue.

Re: [PATCH] Makefile: suppress annotated leaks with certain ASan options

[Jeff King]

On Fri, Jan 20, 2023 at 01:46:16PM -0500, Taylor Blau wrote:

> However, it is possible to use the leak sanitizer without
> `SANITIZE=leak`. This happens when building with `SANITIZE=address` and
> enabling the leak sanitizer via the `ASAN_OPTIONS` variable (by
> including the string "detect_leaks=1").
> 
> This renders `UNLEAK()` useless when doing `SANITIZE=address` builds
> which also use the leak checker.

Yeah. I focused on LSan when adding the sanitize/unleak infrastructure
just because it was faster than a full ASan run, which made iterating on
fixes easier. I do think in the long run, once the test suite is leak
free, we may want to support leak-checking via ASan for the simple
reason that it can be done for "free" during the existing ASan
build/test, rather than requiring an extra LSan job.

I do think there's some complexity here, though.

One problem UNLEAK() is that compile-time switch, but whether ASan does
leak detection is a run-time choice. So you are stuck with either:

  - you always turn on UNLEAK() for ASan builds, in which case test runs
    using the default ASAN_OPTIONS we set do the extra work even though
    they are not doing any leak detection. I doubt it's very measurable,
    though (it's just shoving a few bytes onto a linked list),
    especially compared to the overall slowness of ASan.

  - you predicate the build-time choice on ASAN_OPTIONS. But this means
    that:

      make SANITIZE=address
      cd t
      ASAN_OPTIONS=detect_leaks=1 ./t0000-*.sh

    will confusingly fail to use UNLEAK().

Your patch does the second one, but I think the first may be the
least-bad option.

The other issue I'd worry about is optimizations. Generally you want to
use -O2 with ASan, because it speeds things up (even more than just
regular -O2, I think, because it is optimizing the ASan instrumentation
code, too). I don't know offhand of any cases where it would find or not
find cases based on optimization level, though I could believe they
exist.

But for leak-checking, we've already seen real cases where using LSan
with higher optimization levels can lead to false positives (because the
optimizer drops a value that is still in scope but not used in a code
path that leads to exit()).

So it may be that we really do want to keep leak-checking to "-O0
-fsanitize=leak", and reserve "-O2 -fsanitize=address" for finding
address bugs.

> I found this while playing around with GitHub's ASan-enabled CI builds
> for our internal fork following a merge with v2.38.3.
> 
> The check-chainlint recipe in t/Makefile started using "git diff" via
> d00113ec34 (t/Makefile: apply chainlint.pl to existing self-tests,
> 2022-09-01), which triggered a leak in some of GitHub's custom code. I
> was surprised when marking the variable with UNLEAK() didn't do the
> trick, and ended up down this rabbit hole ;-).

Yes, but I'd ask why your ASan builds were checking for leaks in the
first place. There are presumably tons of leaks they'd detect, since the
test suite is far from leak-free.

-Peff

Re: [PATCH] Makefile: suppress annotated leaks with certain ASan options

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I do think there's some complexity here, though.
>
> One problem UNLEAK() is that compile-time switch, but whether ASan does
> leak detection is a run-time choice. So you are stuck with either:
>
>   - you always turn on UNLEAK() for ASan builds, in which case test runs
>     using the default ASAN_OPTIONS we set do the extra work even though
>     they are not doing any leak detection. I doubt it's very measurable,
>     though (it's just shoving a few bytes onto a linked list),
>     especially compared to the overall slowness of ASan.
>
>   - you predicate the build-time choice on ASAN_OPTIONS. But this means
>     that:
>
>       make SANITIZE=address
>       cd t
>       ASAN_OPTIONS=detect_leaks=1 ./t0000-*.sh
>
>     will confusingly fail to use UNLEAK().
>
> Your patch does the second one, but I think the first may be the
> least-bad option.

Thanks, I totally missed the fact that ASAN_OPTIONS was a runtime
thing.  If we were to pursue this topic of enabling UNLEAK() outside
LSan, I agree the first would be necessary.

> But for leak-checking, we've already seen real cases where using LSan
> with higher optimization levels can lead to false positives (because the
> optimizer drops a value that is still in scope but not used in a code
> path that leads to exit()).
> ...
> So it may be that we really do want to keep leak-checking to "-O0
> -fsanitize=leak", and reserve "-O2 -fsanitize=address" for finding
> address bugs.

Yup, we have been burned a few times with this, IIRC.

Re: [PATCH] Makefile: suppress annotated leaks with certain ASan options

[Jeff King]

On Fri, Jan 20, 2023 at 03:15:10PM -0500, Jeff King wrote:

> One problem UNLEAK() is that compile-time switch, but whether ASan does
> leak detection is a run-time choice. So you are stuck with either:

Er, this should be "one problem is that UNLEAK() is a compile-time
switch".

-Peff