git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
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: Fri, 29 Oct 2021 16:56:31 -0400	[thread overview]
Message-ID: <YXxf/+dwx5giy6im@coredump.intra.peff.net> (raw)
In-Reply-To: <211027.865ytixjcw.gmgdl@evledraar.gmail.com>

On Wed, Oct 27, 2021 at 10:57:52PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > 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().

I don't think that's true. If you UNLEAK() the rev_info in the caller,
then it will only affect allocations that are still reachable from
rev_info. I.e., things that are by definition not a leak in
setup_revisions().

Now you could argue that setup_revisions() is "leaking" by allocating
things and stuffing them into rev_info that it should not be. But we can
never know that until we have an actual function that cleans up a
rev_info, which defines what it's "supposed" to have ownership of.

Maybe we have callers that explicitly try to de-allocate bits of the
rev_info. But IMHO that is the source of the whole problem: how is
random code using rev_info supposed to know which of its internal
details are owned or not? This should be documented and enforced with a
single function.

> 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...

Just to be clear: I am totally fine with dropping Taylor's UNLEAK
patches (as I've said already). I was only arguing here that annotating
via external files is worse than just adding an UNLEAK().

I'm also trying to combat what I see as mis-conceptions or inaccuracies
about what UNLEAK() does or its implications (or even what counts as a
"leak"). But I hope in the long run that we don't need _any_ kind of
annotation, because we'll actually be leak-free. And then we don't have
to care about any of this.

> > 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).

I don't think UNLEAK() can move something from "direct" to "indirect" in
LSan's terminology. If rev_info points to an array of structs, and those
structs point to allocated strings, then the array itself is a "direct"
leak, and the strings are "indirect" (they are leaked, but presumably
fixing the direct leak would also deallocate them).

If UNLEAK() makes the array not-leaked, then those indirect leaks don't
become direct. They should be transitively not-leaked, too.

> 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).

So these should be real leaks. Of course with the lousy stack trace it's
hard to see what they are. But I don't see how UNLEAK() is responsible
for making the lousy stack trace. You could try compiling with LSan but
_not_ -DSUPPRESS_ANNOTATED_LEAKS and see if the result is similarly bad
(but I expect it to be, since test-date.c does not have any UNLEAK()
calls in it).

-Peff

  reply	other threads:[~2021-10-29 20:56 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 [this message]
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=YXxf/+dwx5giy6im@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=me@ttaylorr.com \
    /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).