From: Derrick Stolee <stolee@gmail.com>
Date: Mon, 31 Aug 2026 12:38:01 GMT
Subject: Re: [PATCH v2 1/7] banned-die: create header for banning of functions
Message-ID: <9a84da3f-3409-49ea-be57-1e17e373273c@gmail.com>
In-Reply-To: <20260827051053.GB176544@coredump.intra.peff.net>

On 8/27/2026 1:10 AM, Jeff King wrote:
> 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.

Yes, this makes it tricky to be 100% sure without some kind of static
analysis.

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

This does have some benefit that we can gradually remove these
transitive callers in the multi-commit series. But it's unsatisfying
as a full protection in the end.

> 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.
The other alternative that we could consider is to reorganize the
codebase in such a way that certain sections of code don't have
access to headers that could lead to die() or other "higher" methods
that are acceptable for user-facing processes but are best to avoid
in library APIs. Even then, we'd need some checks at compile time to
avoid crossing boundaries.

I don't think such a reorganization is desirable overall, because
that will be very disruptive to the project and file history.

Having some amount of protection through this header gives us a
mechanism to demonstrate and enforce some protection.

Thanks,
-Stolee