git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Taylor Blau <me@ttaylorr.com>,
	git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS "suppressions"
Date: Wed, 27 Oct 2021 22:57:52 +0200	[thread overview]
Message-ID: <211027.865ytixjcw.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <YXkWhFGS/uTAnsI1@coredump.intra.peff.net>


On Wed, Oct 27 2021, Jeff King wrote:

> On Wed, Oct 27, 2021 at 10:04:17AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> >   - it keeps the annotations near the code. Yes, that creates conflicts
>> >     when the code is changed (or the leak is actually fixed), but that's
>> >     a feature. It keeps them from going stale.
>> 
>> I fully agree with you in general for "good" uses of UNLEAK(). E.g. consider:
>> 
>>     struct strbuf buf = xmalloc(...);
>>     [...]
>>     UNLEAK(buf);
>> 
>> If I was fixing leaks in that sort of code what I pointed out downthread
>> in [1] wouldn't apply.
>
> I'm OK with such a use of UNLEAK(), though of course you could probably
> just free the buffer in that instance.
>
> But...
>
>> So to clarify, I'm not asking in [1] that UNLEAK() not be used at all
>> while we have in-flight leak fixes. I.e. I'd run into a textual
>> conflict, but that would be trivial to resolve, and obvious what the
>> semantic & textual conflict was.
>> 
>> Rather, it's not marking specific leaks, but UNLEAK() on a container
>> struct that's problematic.
>
> ...that's the whole point of UNLEAK(), IMHO. You know that the container
> struct is leaked, but you don't care because the program is about to
> exit, or there's no easy way to avoid the leak.
>
> So it's not the "container" element, but rather it can be a problem if
> people annotate too broadly (you will miss some leaks). In the case of
> rev_info, there is no way to _not_ leak right now, because it has no
> cleanup function.

It doesn't have one, but there are uses of setup_revisions() and
rev_info usage that don't leak, as that builtin/rev-list.c case shows.

I mean, in that case it's not doing much of anything, but at least we
test that setup_revisions() itself doesn't leak right now, but wouldn't
with UNLEAK().

> In the long run we should probably add one.

After the current in-flight patches I've got making diff_free() a bit
more useful, and then a revisions_release() that'll handle most cases,
which gets us started on mass-marking the tests as leak free.

So just FWIW I'm not saying "hey can we hold off on that UNLEAK() for
far future xyz", but for a thing I've got queued up that I'd rather not
start rewriting...

> [...]But in the meantime, annotating them so we can find the
> _interesting_ leaks is worth doing (and that was the whole point of
> adding UNLEAK in the first place.
>
> And I think the UNLEAK() calls in Taylor's patch are fine in that sense.
> While yes, _some_ runs of those commands will not leak, the point is to
> say that when they do leak, they are not interesting. And they are not
> interesting because that rev_info is in scope until the program exits,
> so anything it points to is only leaked at a moment where we no longer
> care.

I don't per-se care about leaks at program exit either, but am making
them leak-free as a proxy for making the relevant APIs useful for
persistent use.

So I think the historical approach of just sprinkling UNLEAK() since
we're exiting anyway is taking too narrow of a view.

In that case if we'd like to see if revisions.[ch] is leak-free we'd
need parallel test-helper tests (or whatever) for all its features, or
we can just piggy-back on every "git log" invocation in the test
suite...

>> So we wouldn't just be marking a known specific leak, but hiding all
>> leaks & non-leaks in container from the top-level, and thus hide
>> potential regressions, an addition to attaining the end-goal of marking
>> some specific test as passing.
>
> I'd argue it _is_ a known specific leak. It is rev_info going out of
> scope that causes the leak, not rev_info holding on to memory in things
> like its pending array.
>
> Now those can be interesting, too (if it no longer needs the memory, can
> it perhaps get rid of it). But IMHO all of that is pretty secondary to
> clearing the noise so you can find "true" leaks (ones where some
> sub-function really is allocating and then losing the pointer entirely,
> especially if it's called an arbitrary number of times).

I wasn't trying to get into the philosophical discussion of whether a
struct or a struct it contains can be said to be the "real" source of a
leak :)

Yeah, I agree that from that POV there's no difference.

I'm merely referring to the practical aspect of patches I've got queued
up, which do things like fix all leaks of rev_info.pending, mark tests
that now pass, fix another rev_info.whatever, mark tests etc.

For rev_info in particular it's a bit iffy to UNLEAK() it, since it also
contains pointers to stuff it doesn't "own", sometimes things are
allocated and handed off to it, sometimes for a while, sometimes
permanently.

So by UNLEAK()ing it you're not just unleaking "its" memory, but
potentially hiding leaks in APIs that need to interact with it in that
way. E.g. mailmap.c is one of those cases.

>> >   - leak-checkers only know where things are allocated, not who is
>> >     supposed to own them. So you're often annotating the wrong thing;
>> >     it's not a strdup() call which is buggy and leaking, but the
>> >     function five layers up the stack which was supposed to take
>> >     ownership and didn't.
>> 
>> As noted in [3] this case is because the LSAN suppressions list was in
>> play, so we excluded the meaningful part of the stack trace (which is
>> shown in that mail).
>
> I don't think that's true at all. The annotations you showed in that
> message, for example, are pointing at add_rev_cmdline(). But that is
> _not_ the source of the leak, nor where it should be fixed. It is a
> necessary part of how rev_info works. The leak is when the rev_info goes
> out of scope without anybody cleaning it up.
>
>> Hrm, now that I think about it I think that the cases where I had to
>> resort to valgrind to get around such crappy stacktraces (when that was
>> all I got, even without suppressions) were probably all because there
>> was an UNLEAK() in play...
>
> I don't see how UNLEAK() would impact stack traces. It should either
> make something not-leaked-at-all (in which case LSan will no longer
> mention it), or it does nothing (it throws some wasted memory into a
> structure which is itself not leaked).

Yes, I think either categorically wrong here, or it applies to some
other case I wasn't able to dig up. Or maybe not, doesn't Taylor's
example take it from "Direct leak" to "Indirect leak" with the
suppression in play? I think those were related somehow (but don't have
that in front of me as I type this out).

E.g. (to reinforce your point) try compiling with SANITIZE=leak and running:

    $ TZ=UTC t/helper/test-tool date show:format:%z 1466000000 +0200
    1466000000 -> +0000
    +0200 -> +0000
    
    =================================================================
    ==335188==ERROR: LeakSanitizer: detected memory leaks
    
    Direct leak of 3 byte(s) in 1 object(s) allocated from:
        #0 0x7f31cdd21db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54
        #1 0x7f31cdb04e4a in __GI___strdup string/strdup.c:42
    
    SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).

Now try with "SANITIZE=" and valgrind can give you the "real" source:

    $ TZ=UTC valgrind --leak-check=full t/helper/test-tool date show:format:%z 1466000000 +0200
    ==336515== Memcheck, a memory error detector
    ==336515== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
    ==336515== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info
    ==336515== Command: t/helper/test-tool date show:format:%z 1466000000 +0200
    ==336515==
    1466000000 -> +0000
    +0200 -> +0000
    ==336515==
    ==336515== HEAP SUMMARY:
    ==336515==     in use at exit: 1,208 bytes in 13 blocks
    ==336515==   total heap usage: 50 allocs, 37 frees, 27,593 bytes allocated
    ==336515==
    ==336515== 3 bytes in 1 blocks are definitely lost in loss record 1 of 13
    ==336515==    at 0x483877F: malloc (vg_replace_malloc.c:307)
    ==336515==    by 0x49C6E4A: strdup (strdup.c:42)
    ==336515==    by 0x261E3A: xstrdup (wrapper.c:29)
    ==336515==    by 0x152785: parse_date_format (date.c:991)
    ==336515==    by 0x116A34: show_dates (test-date.c:39)
    ==336515==    by 0x116DE9: cmd__date (test-date.c:116)
    ==336515==    by 0x115227: cmd_main (test-tool.c:127)
    ==336515==    by 0x115334: main (common-main.c:52)
    ==336515==
    ==336515== LEAK SUMMARY:
    ==336515==    definitely lost: 3 bytes in 1 blocks
    ==336515==    indirectly lost: 0 bytes in 0 blocks
    ==336515==      possibly lost: 0 bytes in 0 blocks
    ==336515==    still reachable: 1,205 bytes in 12 blocks
    ==336515==         suppressed: 0 bytes in 0 blocks
    ==336515== Reachable blocks (those to which a pointer was found) are not shown.
    ==336515== To see them, rerun with: --leak-check=full --show-leak-kinds=all
    ==336515==
    ==336515== For lists of detected and suppressed errors, rerun with: -s
    ==336515== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

I don't know why it habitually loses track of seemingly easy-to-track
leaks sometimes, but it does. There isn't an UNLEAK() in play here.

When I run into these I try it with valgrind, fix the leak, then test
again with SANITIZE=leak. I haven't found cases where it actually misses
things, just cases where the stack traces suck.

  parent reply	other threads:[~2021-10-27 21:12 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 [this message]
2021-10-29 20:56                   ` Jeff King
2021-10-29 21:05                     ` Jeff King
2021-10-27  7:51           ` Ævar Arnfjörð Bjarmason
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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=211027.865ytixjcw.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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).