Re: [PATCH v2 0/7] trace2: stop allowing die()

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:

quoted

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