[RFC] Create a 'safe' strbuf API

[derrickstolee]

This is based on ds/trace2-tolerate-failed-timestamps [1] [2].

[1] https://lore.kernel.org/git/pull.2178.v3.git.1788197143.gitgitgadget@gmail.com/

[2] #2178

While investigating the fact that the trace2 API can trigger recursive die() loops if allocation fails, Peff pointed out [3] that trace2 uses json-writer which in turn uses the strbuf API. If a strbuf fails to allocate, grow, or otherwise mutate the given strings, then trace2 can hit this problem!

[3] https://lore.kernel.org/git/20260901050129.GB1075462@coredump.intra.peff.net/

The goal of this short RFC, such as it is, is to get some feedback on whether this is a worthwhile direction to pursue or if I should abandon this idea of having this definition of "safe" for some APIs. This decision may also determine if we should abandon ds/trace2-tolerate-failed-timestamps or leave the existing behavior as-is.

I had discussed earlier that what we'd really need is a guarantee that we can't transitively reach die() from any "safe" API. The eventual goal would be to include json-writer.c and the trace2 code files into the "safe" bucket, but for now I'm making sure that strbuf-safe.c satisfies this CodeQL query:

import cpp

class SafeFunction extends Function {
  SafeFunction() {
    getFile().getRelativePath() = "strbuf-safe.c"
  }
}

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


from SafeFunction source, Function sink
where
  (sink.getName() = "die" or sink.getName() = "exit") and
  directlyCalls+(source, sink)
select source,
  "This safe function can transitively reach " + sink.getName() + "()."

If we went with this approach, then I'd explore how to make this a build-time requirement during CI.

In regards to the structure of this RFC:

1. The safe API needs the same structures, but shouldn't import more than necessary. Some movement of structs across headers is done before anything else.
2. In order to make even the smallest safe method work, we first need to figure out how to handle GIT_ALLOC_LIMIT, which is an undocumented environment variable. I explain that I think this should be GIT_TEST_ALLOC_LIMIT, but maybe the ship has sailed due to Hyrum's Law. So I make an effort to document it but also to initialize it proactively within the process startup instead of implicitly at the lowest level. This allows us to avoid a die() when checking the environment variable.
3. Thus, we get a 'safe' version of a memory allocation size check. This is our first example of creating a safe version that is then called by the non-safe version to prevent repeated code.
4. We can then create our first safe strbuf method: sstrbuf_grow(). I explain why I prepend with s instead of appending _gently in the commit.
5. Some trace2 code implicitly depends on strbuf.h through json-writer.h, so we drop that in favor of strbuf-safe.h to keep the dependence on the full struct definition without forever having the non-safe methods reachable. The goal eventually is to drop the strbuf.h include from json-writer.c, but that isn't accomplished in this RFC.
6. Finally, create safe init and release methods and use them in json-writer.c. This does show some of the "transition risk" where some json-writer methods become "safe" but I haven't done the hard work to make sure the callers of those methods respond to the new return values. If we proceed with the RFC, then I'd split this into a creation of the safe strbuf methods and then the refactoring required to respond correctly to errors in json-writer.c

Thanks in advance for your thoughts!

Thanks,
-Stolee

cc: gitster@pobox.com
cc: peff@peff.net
cc: newren@gmail.com

[derrickstolee]

/submit

[gitgitgadget]

Submitted as pull.2230.git.1789736540.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2230/derrickstolee/strbuf-safe-v1

To fetch this version to local tag pr-2230/derrickstolee/strbuf-safe-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2230/derrickstolee/strbuf-safe-v1