Re: [PATCH 0/6] [RFC] Create a 'safe' strbuf API

Hi Stolee

On 18/09/2026 14:02, Derrick Stolee via GitGitGadget wrote:

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 think having APIs that return errors rather than dying on allocation 
failures or overflow is reasonable. The xdiff and reftable code already 
have something similar. I'm not sure "safe" is a good description for 
those APIs though as it does not describe how they differ from the 
existing APIs. Instead of talking about safety I'd rather the 
documentation talked about returning errors on failure and the function 
naming somehow reflected that.

For the strbuf API having to check for failure on every function call 
does not sound attractive, I think having a sticky error bit like the 
stdio functions so that one can build a string and check there have been 
no failures once just before using it would be a nicer approach.

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:

I don't know enough about CodeQL to comment on this beyond noting that 
the implementation of sstrbuf_grow() in patch 4 contains a call to 
st_add3() which dies on overflow (it should be using st_add_overflows() 
instead) so something isn't right with these checks.

Thanks

Phillip

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

Derrick Stolee (6):
   strbuf: add header for 'safe' API
   wrapper: initialize GIT_ALLOC_LIMIT proactively
   wrapper: create safe_memory_limit_check()
   strbuf-safe: add sstrbuf_grow()
   json-writer: include strbuf-safe.h
   strbuf-safe: add init and release methods

  Documentation/git.adoc |  6 +++
  Makefile               |  1 +
  common-init.c          |  2 +
  environment.h          |  1 +
  json-writer.c          | 32 ++++++++------
  json-writer.h          |  7 +--
  meson.build            |  1 +
  strbuf-safe.c          | 52 ++++++++++++++++++++++
  strbuf-safe.h          | 97 ++++++++++++++++++++++++++++++++++++++++++
  strbuf.c               | 23 ++++------
  strbuf.h               | 74 ++------------------------------
  trace2/tr2_tgt_event.c |  1 +
  trace2/tr2_tgt_perf.c  |  1 +
  wrapper.c              | 67 ++++++++++++++++++++---------
  wrapper.h              |  9 ++++
  15 files changed, 253 insertions(+), 121 deletions(-)
  create mode 100644 strbuf-safe.c
  create mode 100644 strbuf-safe.h


base-commit: a80c36bda0e5aff1c9945d08f43079a6aa85ccad
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2230%2Fderrickstolee%2Fstrbuf-safe-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2230/derrickstolee/strbuf-safe-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2230