Git development
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, gitster@pobox.com,
	Taylor Blau <ttaylorr@openai.com>,
	Derrick Stolee <stolee@gmail.com>
Subject: Re: [PATCH v2 1/7] banned-die: create header for banning of functions
Date: Thu, 27 Aug 2026 01:10:53 -0400	[thread overview]
Message-ID: <20260827051053.GB176544@coredump.intra.peff.net> (raw)
In-Reply-To: <84634717e2eca479026d1cdf39a089a8f61d131e.1787684181.git.gitgitgadget@gmail.com>

On Tue, Aug 25, 2026 at 06:56:15PM +0000, Derrick Stolee via GitGitGadget wrote:

> We have universally-banned functions listed in banned.h since
> c8af66ab8ad (automatically ban strcpy(), 2018-07-26), but some layers of
> the code should be more strict than others.
> 
> One such example is the trace2 API which runs during atexit() and can
> prove to cause die()-handler recursion problems if it calls die().
> 
> Create a new banned-die.h header file that will ban some Git methods
> that call die(). Include that in all trace2 API implementation files.
> This currently only bans die() itself, and that was already not used.

There's a subtle but big difference between the universal code bans in
banned.h and this banned-die.h. In the former case we are deciding
strcpy() is unfit for our code base and outlawing it everywhere. The
potential problem is in the source code, so catching it while compiling
the source code is OK.

But we are not doing that with die(). It is a perfectly OK function in
general, but we do not want to ever trigger its runtime effects from
certain code paths. Banning it from being called from those code paths
can catch _some_ instances, but not any transitive calls. If we call
foo(), it may call die() itself, and we would not want to ban foo() from
doing so. And recursively for functions called by foo() and so on.

So you end up playing whack-a-mole with functions that might call die()
and adding them to this ban list.

I think that's _probably_ the best we can do in practice. I think the
framing above suggests that we could approach the problem more directly
with a runtime flag: when we enter those code paths, set a flag to avoid
the unwanted behavior, and have the low-level code respect that. But
die() is a special case here, because we'd want to suppress its
no-return behavior. And its callers are not prepared for die() to
suddenly start returning because of some global flag.

So I think the whack-a-mole is the best we can do. But I would not want
to see this strategy extended to other areas. In most cases some kind of
runtime support is probably a better solution.

-Peff

  parent reply	other threads:[~2026-08-27  5:10 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-15 16:12 [PATCH] trace2: tolerate failed timestamp formatting Derrick Stolee via GitGitGadget
2026-07-17 16:24 ` Taylor Blau
2026-07-18 15:01   ` Derrick Stolee
2026-07-20 14:29     ` Junio C Hamano
2026-07-20 14:37       ` Taylor Blau
2026-07-29 21:35       ` Junio C Hamano
2026-07-31 13:26         ` Derrick Stolee
2026-07-31 15:57           ` Junio C Hamano
2026-08-25 18:56 ` [PATCH v2 0/7] trace2: stop allowing die() Derrick Stolee via GitGitGadget
2026-08-25 18:56   ` [PATCH v2 1/7] banned-die: create header for banning of functions Derrick Stolee via GitGitGadget
2026-08-25 20:34     ` Junio C Hamano
2026-08-25 22:14     ` Elijah Newren
2026-08-27  5:10     ` Jeff King [this message]
2026-08-25 18:56   ` [PATCH v2 2/7] trace2: tolerate failed timestamp formatting Derrick Stolee via GitGitGadget
2026-08-25 18:56   ` [PATCH v2 3/7] trace2: remove use of xstrdup() Derrick Stolee via GitGitGadget
2026-08-25 22:14     ` Elijah Newren
2026-08-25 18:56   ` [PATCH v2 4/7] trace2: remove use of ALLOC_ARRAY() Derrick Stolee via GitGitGadget
2026-08-25 18:56   ` [PATCH v2 5/7] trace2: remove use of xstrfmt() Derrick Stolee via GitGitGadget
2026-08-25 22:14     ` Elijah Newren
2026-08-25 22:36       ` Junio C Hamano
2026-08-25 18:56   ` [PATCH v2 6/7] trace2: remove use of ALLOC_GROW() Derrick Stolee via GitGitGadget
2026-08-25 22:14     ` Elijah Newren
2026-08-25 18:56   ` [PATCH v2 7/7] trace2: remove use of xcalloc() Derrick Stolee via GitGitGadget
2026-08-27  5:23   ` [PATCH v2 0/7] trace2: stop allowing die() Jeff King

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=20260827051053.GB176544@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=stolee@gmail.com \
    --cc=ttaylorr@openai.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.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox