From: Derrick Stolee <stolee@gmail.com>
Date: Mon, 31 Aug 2026 13:27:49 GMT
Subject: Re: [PATCH v2 0/7] trace2: stop allowing die()
Message-ID: <a41bdb3b-1fe7-4c1e-9d16-72390d93503b@gmail.com>
In-Reply-To: <20260827052318.GC176544@coredump.intra.peff.net>

On 8/27/2026 1:23 AM, Jeff King wrote:
> On Tue, Aug 25, 2026 at 06:56:14PM +0000, Derrick Stolee via GitGitGadget wrote:
> 
>> This starts with a new banned-die.h header file at the root of the repo and
>> including it from all trace2 API *.c files. It starts empty, but the later
>> patches will add one method at a time:
>>
>>  * xsnprintf() : This is the original patch, but made more complete by
>>    adding the method to banned-die.h.
>>  * xstrdup()
>>  * ALLOC_ARRAY()
>>  * xstrfmt()
>>  * ALLOC_GROW()
>>  * xcalloc()
> 
> OK. This feels like the tip of the iceberg, though. All of strbuf would
> have to be off-limits, too (both because it calls malloc directly, but
> also because it will bail if snprintf() returns -1). I won't be
> surprised if there are other indirect calls hiding in various places
> (e.g., all of json-writer.c).

You're absolutely right. Not only in json-writer.c, but several direct
calls to the strbuf API. The only real way to fix that would be to
create a "safe strbuf" library. This is potentially an interesting
direction that I might want to pursue and send an RFC after getting
started. 
> I think if you really want to avoid allocations in trace2 it would
> probably need to be a ground-up no-dependency rewrite.

Or to update the dependencies to be "safe". Not an easy thing, either
way.

I don't have much knowledge of CodeQL, but the following vibe-coded
.ql script is able to detect these transitive calls and demonstrate
the issue:

----

import cpp

class Trace2Function extends Function {
  Trace2Function() {
    getFile().getRelativePath() = "trace2.c" or
    getFile().getRelativePath().matches("trace2/%.c")
  }
}

predicate directlyCalls(Function caller, Function callee) {
  exists(FunctionCall call |
    call.getEnclosingFunction() = caller and
    call.getTarget() = callee
  )
}

from Trace2Function source, Function sink
where
  sink.getName() = "die" and
  directlyCalls+(source, sink)
select source, "This Trace2 function can transitively reach die()."

----

Adding such a check now would obviously fail and not provide any
ability to demonstrate incremental progress like banned-die.h.

I know that microsoft/git is running CodeQL analysis to look for
security issues [1] but doesn't appear to be running specific
queries like this one.

[1] https://github.com/microsoft/git/commit/6b367b94752b7ae0fada0629a542e90ea0a1892c

Perhaps this is something we could investigate in the future.

Thanks,
-Stolee