Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS "suppressions"

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 26, 2021, 20:23 UTC
Message-ID: <YXhjstW2XAnixEqh@nand.local>
In-Reply-To: <patch-1.1-9190f3c128f-20211022T102725Z-avarab@gmail.com>

On Fri, Oct 22, 2021 at 12:32:17PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 55 quoted lines

> Extend the SANITIZE=leak testing mode added in 956d2e4639b (tests: add
> a test mode for SANITIZE=leak, run it in CI, 2021-09-23) to optionally
> be able to add a "suppressions" file to the $LSAN_OPTIONS.
>
> This allows for marking tests as passing with
> "TEST_PASSES_SANITIZE_LEAK=true" when they still have failure due more
> general widespread memory leaks, such as from the "git log" family of
> commands. We can now mark the "git -C" tests as passing.
>
> For getting specific tests to pass this is preferable to using
> UNLEAK() in these codepaths, as I'll have fixes for those leaks soon,
> and being able to atomically mark relevant tests as passing with
> "TEST_PASSES_SANITIZE_LEAK=true" helps to explain those changes. See
> [1] for more details.
>
> 1. https://lore.kernel.org/git/211022.86sfwtl6uj.gmgdl@evledraar.gmail.com/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> On Fri, Oct 22 2021, Ævar Arnfjörð Bjarmason wrote:
>
> > On Fri, Oct 22 2021, Taylor Blau wrote:
> >
> >> On Thu, Oct 21, 2021 at 01:50:55PM +0200, Ævar Arnfjörð Bjarmason wrote:
> >>>
> >>> On Wed, Oct 20 2021, Taylor Blau wrote:
> [...]
> > If you want to pick that approach up and run with it I think it would
> > probably make sense to factor that suggested test_expect_success out
> > into a function in test-lib-functions.sh or whatever, and call it as
> > e.g.:
> >
> >     TEST_PASSES_SANITIZE_LEAK=true
> >      . ./test-lib.sh
> >     declare_known_leaks <<-\EOF
> >     add_rev_cmdline
> >     [...]
> >     EOF
> >
> > Then pipe it through sed 's/^/leak:/' and have it set LSAN_OPTIONS for
> > you.
> >
> > Doing it that way would be less boilerplate for each test that wants it,
> > and is also more likely to work with other non-LSAN leak appoaches,
> > i.e. as long as something can take a list of lines matching stack traces
> > we can feed that to that leak checker's idea of a whitelist.
>
> I just went ahead and hacked that up. If you're OK with that approach
> it would really help reduce the work for leak changes I've got
> planned, and as noted gives you the end-state of a passing 5319.
>
> I don't know if it makes more sense for you to base on top of this
> if/when Junio picks it up, or to integrate it into your series
> etc. Maybe Junio will chime in ...

Hmm. This seems neat, but I haven't been able to get it to work without encountering a rather annoying bug along the way.

Here is the preamble of my modified t5319 to include all of the leaks I found in the previous round (but decided not to fix):

TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh todo_leaks <<-\EOF ^add_rev_cmdline$ ^cmd_log_init_finish$ ^prepare_revision_walk$ ^prep_parse_options$ EOF

So running that when git is compiled with SANITIZE=leak *should* work, but instead produces this rather annoying leak detection after t5319.7:

  =================================================================
  ==1785298==ERROR: LeakSanitizer: detected memory leaks

  Indirect leak of 41 byte(s) in 3 object(s) allocated from:
      #0 0x7f2f2f866db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54
      #1 0x7f2f2f64b4aa in __GI___strdup string/strdup.c:42

  -----------------------------------------------------
  Suppressions used:
    count      bytes template
        1        576 ^add_rev_cmdline$
  -----------------------------------------------------

  SUMMARY: LeakSanitizer: 41 byte(s) leaked in 3 allocation(s).
  Aborted

Huh? Looking past __GI___strdup (which I assume is just a symbolification failure on ASan's part), who calls strdup()? That trace is annoyingly incomplete, and doesn't really give us anything to go off of.

It does seem to remind me of this:

  https://github.com/google/sanitizers/issues/148

though that issue goes in so many different directions that I'm not sure whether it really is the same issue or not. In any case, that leak *still* shows up even when suppressing xmalloc() and xrealloc(), so I almost think that it's a leak within ASan itself.

But most interesting is that those calls go away when I stop setting `suppressions` in $LSAN_OPTIONS by dropping the call to your todo_leaks.

So this all feels like a bug in ASan to me. I'm curious if it works on your system, but in the meantime I think the best path forward is to drop the last patch of my original series (the one with the three UNLEAK() calls) and to avoid relying on this patch for the time being.

Thanks, Taylor