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

[Phillip Wood <phillip.wood123@gmail.com>]

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