list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <>
To: "Ævar Arnfjörð Bjarmason" <>
Subject: Re: [PATCH 2/2] commit: use strbuf_release() instead of UNLEAK()
Date: Wed, 16 Feb 2022 10:30:18 -0800	[thread overview]
Message-ID: <xmqqtucyslz9.fsf@gitster.g> (raw)
In-Reply-To: <xmqqa6equ1rq.fsf@gitster.g> (Junio C. Hamano's message of "Wed, 16 Feb 2022 10:03:53 -0800")

Junio C Hamano <> writes:

> Ævar Arnfjörð Bjarmason  <> writes:
>> Convert the UNLEAK() added in 0e5bba53af7 (add UNLEAK annotation for
>> reducing leak false positives, 2017-09-08) to release the memory using
>> strbuf_release() instead.
>> The tests being marked as passing with
>> "TEST_PASSES_SANITIZE_LEAK=true" already passed before due to the
>> UNLEAK(), but now they really don't leak memory, so let's mark them as
>> such.
> That smells like a brave move.
> Specifically, the cited commit turned an existing strbuf_release()
> on &err into UNLEAK().  If that and the other strbuf (sb) were so
> easily releasable, why didn't we do so back then already?

I suspect that the answer to the above question is because these
allocations are in the top-level cmd_commit() function, which is
never called recursively or repeatedly as a subroutine.  The only
significant thing that happens after we return from it is to exit.

In such a code path, marking a variable as UNLEAK() is a better
thing to do than calling strbuf_release().  Both will work as a way
to squelch sanitizers from reporting a leak that does not matter,
but calling strbuf_release() means we'd spend extra cycles to return
pieces of memory to the pool, even though we know that the pool
itself will be cleaned immediately later at exit.

We already have UNLEAK to tell sanitizers not to distract us from
spotting and plugging real leaks by reporting these apparent leaks
that do not matter.  It is of somewhat dubious value to do a "we
care too much about pleasing sanitizer and spend extra cycles at
runtime while real users are doing real work" change.

  reply	other threads:[~2022-02-16 18:30 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-02-16  8:21 [PATCH 0/2] commit: trivial leak fix, add 2 tests to linux-leaks CI Ævar Arnfjörð Bjarmason
2022-02-16  8:21 ` [PATCH 1/2] commit: fix "author_ident" leak Ævar Arnfjörð Bjarmason
2022-02-16 17:59   ` Junio C Hamano
2022-02-16  8:21 ` [PATCH 2/2] commit: use strbuf_release() instead of UNLEAK() Ævar Arnfjörð Bjarmason
2022-02-16 18:03   ` Junio C Hamano
2022-02-16 18:30     ` Junio C Hamano [this message]
2022-02-18 12:35       ` Whether to keep using UNLEAK() in built-ins (was: [PATCH 2/2] commit: use strbuf_release() instead of UNLEAK()) Ævar Arnfjörð Bjarmason
2022-02-18 18:19         ` Whether to keep using UNLEAK() in built-ins Junio C Hamano
2022-02-18 19:31           ` Ævar Arnfjörð Bjarmason
2022-05-12 22:51 ` [PATCH] commit: fix "author_ident" leak Junio C Hamano
2022-05-17 13:48   ` Ævar Arnfjörð Bjarmason
2022-05-18 16:30     ` Junio C Hamano

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:

  List information:

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

  git send-email \
    --in-reply-to=xmqqtucyslz9.fsf@gitster.g \ \ \ \
    --subject='Re: [PATCH 2/2] commit: use strbuf_release() instead of UNLEAK()' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

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