git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Jeff King <peff@peff.net>, Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS "suppressions"
Date: Wed, 27 Oct 2021 09:51:38 +0200	[thread overview]
Message-ID: <211027.86zgquyk52.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <YXhjstW2XAnixEqh@nand.local>


On Tue, Oct 26 2021, Taylor Blau wrote:

> On Fri, Oct 22, 2021 at 12:32:17PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> 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.

There are similar cases where LSAN doesn't provide as meaningful of a
stacktrace as valgrind, sometimes when tracing leaks I get a relatively
bad stacktrace like that ending in some __GI___*<stdlib-name>
function. I'll usually compile without SANITIZE=leak and just run a
slower:

    valgrind --leak-check=full --show-leak-kinds=all

In this case however it's not a bug or bad leak tracing, but an artifact
of us using these stacktrace exclusions.

If you run this manually you'll see:
    
    $ ~/g/git/git -c core.multiPackIndex=false rev-list --objects --all
    cd0747a9352b58d112f0010134351efc7bbad4a6
    [... snipped output ...]
    
    =================================================================
    ==58023==ERROR: LeakSanitizer: detected memory leaks
    
    Direct leak of 576 byte(s) in 1 object(s) allocated from:
        #0 0x7fd0bfae50c5 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:82
        #1 0x5637b3bd9163 in xrealloc /home/avar/g/git/wrapper.c:126
        #2 0x5637b3b6a1e4 in add_rev_cmdline /home/avar/g/git/revision.c:1482
        #3 0x5637b3b6a412 in handle_one_ref /home/avar/g/git/revision.c:1534
        #4 0x5637b3b49c4e in do_for_each_ref_helper /home/avar/g/git/refs.c:1483
        #5 0x5637b3b54afc in do_for_each_repo_ref_iterator refs/iterator.c:418
        #6 0x5637b3b49cc8 in do_for_each_ref /home/avar/g/git/refs.c:1498
        #7 0x5637b3b49d07 in refs_for_each_ref /home/avar/g/git/refs.c:1504
        #8 0x5637b3b6a563 in handle_refs /home/avar/g/git/revision.c:1578
        #9 0x5637b3b6e0e7 in handle_revision_pseudo_opt /home/avar/g/git/revision.c:2597
        #10 0x5637b3b6e9d5 in setup_revisions /home/avar/g/git/revision.c:2738
        #11 0x5637b39ebc58 in cmd_rev_list builtin/rev-list.c:550
        #12 0x5637b3932a89 in run_builtin /home/avar/g/git/git.c:461
        #13 0x5637b3932e98 in handle_builtin /home/avar/g/git/git.c:713
        #14 0x5637b3933105 in run_argv /home/avar/g/git/git.c:780
        #15 0x5637b39335ae in cmd_main /home/avar/g/git/git.c:911
        #16 0x5637b3a1a898 in main /home/avar/g/git/common-main.c:52
        #17 0x7fd0bf860d09 in __libc_start_main ../csu/libc-start.c:308
    
    Indirect leak of 41 byte(s) in 3 object(s) allocated from:
        #0 0x7fd0bfae4db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54
        #1 0x7fd0bf8c7e4a in __GI___strdup string/strdup.c:42
    
    SUMMARY: LeakSanitizer: 617 byte(s) leaked in 4 allocation(s).

Which is why I put the "strdup" there, but forgot to tell you when I
submitted the real PATCH version that that one shouldn't have the ^$
anchoring.

FWIW this will work on top of your series:
    
    diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
    index a3c72b68f7c..891de720b2c 100755
    --- a/t/t5319-multi-pack-index.sh
    +++ b/t/t5319-multi-pack-index.sh
    @@ -2,6 +2,17 @@
     
     test_description='multi-pack-indexes'
     . ./test-lib.sh
    +todo_leaks <<-\EOF
    +^add_rev_cmdline$
    +^cmd_log_init_finish$
    +^prep_parse_options$
    +^prepare_revision_walk$
    +^start_progress_delay$
    +
    +# An indirect leak reported because we excluded the leaks above
    +strdup$
    +EOF
    +
     
     GIT_TEST_MULTI_PACK_INDEX=0
     objdir=.git/objects

  parent reply	other threads:[~2021-10-27  7:58 UTC|newest]

Thread overview: 63+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-21  3:39 [PATCH 00/11] midx: clean up t5319 under 'SANITIZE=leak' Taylor Blau
2021-10-21  3:39 ` [PATCH 01/11] midx.c: clean up chunkfile after reading the MIDX Taylor Blau
2021-10-21  5:50   ` Junio C Hamano
2021-10-21 11:34   ` Ævar Arnfjörð Bjarmason
2021-10-21 16:16   ` Junio C Hamano
2021-10-22  3:04     ` Taylor Blau
2021-10-21  3:39 ` [PATCH 02/11] midx.c: don't leak MIDX from verify_midx_file Taylor Blau
2021-10-21  5:00   ` Eric Sunshine
2021-10-21  5:54     ` Junio C Hamano
2021-10-21 16:27   ` Junio C Hamano
2021-10-21  3:39 ` [PATCH 03/11] t/helper/test-read-midx.c: free MIDX within read_midx_file() Taylor Blau
2021-10-21  3:39 ` [PATCH 04/11] builtin/pack-objects.c: don't leak memory via arguments Taylor Blau
2021-10-21  3:39 ` [PATCH 05/11] builtin/repack.c: avoid leaking child arguments Taylor Blau
2021-10-21 13:32   ` Derrick Stolee
2021-10-21 18:47     ` Junio C Hamano
2021-10-21 16:37   ` Junio C Hamano
2021-10-22  3:21     ` Taylor Blau
2021-10-21  3:40 ` [PATCH 06/11] builtin/multi-pack-index.c: don't leak concatenated options Taylor Blau
2021-10-21  3:40 ` [PATCH 07/11] pack-bitmap.c: avoid leaking via midx_bitmap_filename() Taylor Blau
2021-10-21 16:54   ` Junio C Hamano
2021-10-22  4:27     ` Taylor Blau
2021-10-21  3:40 ` [PATCH 08/11] pack-bitmap.c: don't leak type-level bitmaps Taylor Blau
2021-10-21 16:59   ` Junio C Hamano
2021-10-21  3:40 ` [PATCH 09/11] pack-bitmap.c: more aggressively free in free_bitmap_index() Taylor Blau
2021-10-21  5:10   ` Eric Sunshine
2021-10-21 18:32     ` Junio C Hamano
2021-10-22  4:29       ` Taylor Blau
2021-10-21 18:43   ` Junio C Hamano
2021-10-21  3:40 ` [PATCH 10/11] pack-bitmap-write.c: don't return without stop_progress() Taylor Blau
2021-10-21  5:12   ` Eric Sunshine
2021-10-21 11:31   ` Ævar Arnfjörð Bjarmason
2021-10-21 18:39     ` Junio C Hamano
2021-10-22  4:32       ` Taylor Blau
2021-10-23 20:28       ` Junio C Hamano
2021-10-23 20:32         ` SubmittingPatchs: clarify choice of base and testing Junio C Hamano
2021-10-23 20:59           ` Ævar Arnfjörð Bjarmason
2021-10-23 21:31             ` Junio C Hamano
2021-10-23 21:40             ` Junio C Hamano
2021-10-25  8:59           ` Fabian Stelzer
2021-10-25 16:48             ` Junio C Hamano
2021-10-25 16:56               ` Junio C Hamano
2021-10-25 17:00                 ` Junio C Hamano
2021-12-23 23:12           ` [PATCH v2] " Junio C Hamano
2021-12-28 17:47             ` Elijah Newren
2021-12-30 10:20             ` Fabian Stelzer
2021-12-30 20:18               ` Re* " Junio C Hamano
2021-10-21  3:40 ` [PATCH 11/11] t5319: UNLEAK() the remaining leaks Taylor Blau
2021-10-21 11:50 ` [PATCH 00/11] midx: clean up t5319 under 'SANITIZE=leak' Ævar Arnfjörð Bjarmason
2021-10-22  4:39   ` Taylor Blau
2021-10-22  8:23     ` Ævar Arnfjörð Bjarmason
2021-10-22 10:32       ` [PATCH] leak tests: add an interface to the LSAN_OPTIONS "suppressions" Ævar Arnfjörð Bjarmason
2021-10-26 20:23         ` Taylor Blau
2021-10-26 21:11           ` Jeff King
2021-10-26 21:30             ` Taylor Blau
2021-10-26 21:48               ` Jeff King
2021-10-27  8:04             ` Ævar Arnfjörð Bjarmason
2021-10-27  9:06               ` Jeff King
2021-10-27 20:21                 ` Junio C Hamano
2021-10-27 20:57                 ` Ævar Arnfjörð Bjarmason
2021-10-29 20:56                   ` Jeff King
2021-10-29 21:05                     ` Jeff King
2021-10-27  7:51           ` Ævar Arnfjörð Bjarmason [this message]
2021-10-21 13:37 ` [PATCH 00/11] midx: clean up t5319 under 'SANITIZE=leak' Derrick Stolee

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=211027.86zgquyk52.gmgdl@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=me@ttaylorr.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).