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] https://github.com/gitgitgadget/git/pull/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

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

From: Derrick Stolee <stolee@gmail.com>

The strbuf library is an important API used all over the Git codebase. Contributors use it in nearly any string-manipulating action. However, the implementation uses other helping functions that die() on failure instead of returning an error code. Thus, the strbuf API isn't _safe_.

In particular, we cannot include 'banned-die.h' in 'strbuf.c'.

To start the creation of a safe strbuf API, move the struct definition into a new 'strbuf-safe.h' header file. All consumers of 'strbuf.h' will consume that header transitively.

In the future, we will hope to have consumers that need a 'safe' API will include 'strbuf-safe.h' instead of 'strbuf.h'.

We will see in future changes the inclusion of new implementations that return an error code instead of halting.

Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
 strbuf-safe.h | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
 strbuf.h      | 74 +++----------------------------------------
 2 files changed, 92 insertions(+), 70 deletions(-)
 create mode 100644 strbuf-safe.h

From: Derrick Stolee <stolee@gmail.com>

Before making a safe version of memory_limit_check(), create initialize_git_alloc_limit() to externalize the static memory limit stored in that method. Initialize this intentionally during setup_environment() instead of implicitly during lower-level allocations.

This will allow a future version of memory_limit_check() that doesn't call die() at all, which will require not calling git_env_ulong() directly. This comes with some assumption that initialize_git_alloc_limit() is called before moving into safe APIs, though we will make some reaonable assumptions in those cases.

The GIT_ALLOC_LIMIT environment variable is used by some tests, but is otherwise not advertised. It was added by d41489a642 (Add more large blob test cases, 2012-03-07), which may predate the GIT_TEST_ pattern. This is long enough that it may be possible that someone depends on it in the wild. Thus, I'm choosing to document it instead of renaming it to GIT_TEST_ALLOC_LIMIT.

Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
 Documentation/git.adoc |  6 ++++++
 common-init.c          |  2 ++
 environment.h          |  1 +
 wrapper.c              | 26 +++++++++++++++++---------
 wrapper.h              |  6 ++++++
 5 files changed, 32 insertions(+), 9 deletions(-)

From: Derrick Stolee <stolee@gmail.com>

The existing memory_limit_check() is used in many places within wrapper.c, but because it initializes the GIT_ALLOC_LIMIT environment variable _and_ can call die() when not in gentle mode, this method isn't appropriate for a safe API.

Modify the implementation to be safe_memory_limit_check() and to keep calling error() when there is an allocation problem. The original method calls that version but will die() instead when failing and not gentle.

The one potential behavior change is that when git_alloc_limit is unset we must assume SIZE_MAX instead of loading the environment variable. Since we load this environment variable proactively in setup_environment(), this should only matter for that brief window before setup_environment() and the safe APIs that call this version. If such safe APIs are used in that window, then they should allocate small enough amounts of memory to fit under any reasonable values of GIT_ALLOC_LIMIT.

Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
 wrapper.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

From: Derrick Stolee <stolee@gmail.com>

After a few changes in preparation, we are now ready to create our first 'safe' strbuf API method: sstrbuf_grow(). This is a safe version of strbuf_grow().

On naming: For safe equivalents of existing methods, I'm prepending a single 's' character. The intention is to make the safe API non-intrusive. Alternatives could be to append '_gentle' like many other APIs that avoid a die() on malformed user data, but we need to be even safer than these gentle methods, which still die() on allocation failures or other system-level errors. This 's' prefix is similar to the 'x' prefix used by git-compat-util helpers.

I selected strbuf_grow() as the first method to move because it doesn't depend on any other strbuf API method, but is called by many other strbuf API calls, including strbuf_release() or strbuf_init(). Thus, this will be a helper to several other implementations that are coming in upcoming changes.

No callers directly depend on sstrbuf_grow(), but the non-safe strbuf_grow() now uses it as declared in strbuf-safe.h.

Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
 Makefile      |  1 +
 meson.build   |  1 +
 strbuf-safe.c | 34 ++++++++++++++++++++++++++++++++++
 strbuf-safe.h |  7 +++++++
 strbuf.c      | 11 ++++-------
 wrapper.c     | 26 +++++++++++++++++---------
 wrapper.h     |  3 +++
 7 files changed, 67 insertions(+), 16 deletions(-)
 create mode 100644 strbuf-safe.c

From: Derrick Stolee <stolee@gmail.com>

We will use json-writer.c as our first 'safe' API, after removing some unsafe methods that reach die(), especially the strbuf API. However, the fact that json-writer.h includes strbuf.h causes someheadaches here.

Normally, we would only declare 'struct strbuf;' as a way to anonymously define a struct and have implementations include the full header as needed. However, 'struct json_writer' needs the full struct info and JSON_WRITER_INIT needs access to STRBUF_INIT.

Thankfully, both are included in strbuf-safe.h, so we can have the header include the safe API and move an include of strbuf.h to json-writer.c.

However, this has some implications to the trace2 API that includes json-writer.h and implicitly depends on that includes of strbuf.h. Have those files include strbuf.h directly to compensate.

Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
 json-writer.c          | 1 +
 json-writer.h          | 2 +-
 trace2/tr2_tgt_event.c | 1 +
 trace2/tr2_tgt_perf.c  | 1 +
 4 files changed, 4 insertions(+), 1 deletion(-)

From: Derrick Stolee <stolee@gmail.com>

Continue extending the strbuf-safe API by adding these safe versions of the initialize and release methods:

* sstrbuf_init()
* sstrbuf_release()

These both depend on sstrbuf_grow() that was introduced in the previous change.

As we are working to make json-writer.c a safe API, adapt its use of strbuf_release() to the safe version. To properly handle the responses of the safe versions, some methods are converted to return their own error codes. However, callers of those methods are not adapted at this time and will be adapted in future changes. This leaves a window where json-writer consumers may continue running after an error occurs, potentially leading to a different error in the future.

Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
 json-writer.c | 31 +++++++++++++++++++------------
 json-writer.h |  5 +++--
 strbuf-safe.c | 18 ++++++++++++++++++
 strbuf-safe.h |  2 ++
 strbuf.c      | 12 ++++--------
 5 files changed, 46 insertions(+), 22 deletions(-)

Hi Stolee

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

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 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

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

I have to wonder if this is somewhat backwards, in that the longer term goal for us should be to make most of the service routines like strbuf, string_list, csum_file, etc., free of die() and be "safe".

A recent trend under the label "libification" is to make the use of the_repository more explicit and pass a "struct repository *" as a parameter instead more widely throughout the code flow, but it would be equally if not more useful change to expand the "safe" API surface so that callers of more service routines take responsibility to act on errors.

And picking strbuf as the first instance of such generic service library certainly is a good idea. Its interface is well defined.

We may want to rename functions that _happen_ to use a strbuf to return their results but otherwise has nothing to do with strbuf away from strbuf_ prefix (strbuf_realpath() etc. in abspath.h are prime examples) as part of this first step, though.

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

The code is prepared for a case where git_alloc_limit is set to 0, in which case SIZE_MAX is used as a stand-in value. When the check detects a request with overly large 'size', the error message tells us that 'size' is over 'git_alloc_limit', the latter is zero and any concrete value of 'size' certainly would be over that. Which may be a bit confusing.

Shouldn't we be giving the local "limit" instead in the message?

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

This overrites *ptr with whatever realloc() returns, and then checks if we had an error, thereby losing whatever pointer *ptr originally had. When realloc() does fail, we have already clobbered *ptr, and very likely have robbed our caller the pointer it had to the region of memory. Aren't we leaking that piece of memory as the result?

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

This is puzzling in a few ways.

"enum safe_result" so far has been SUCCESS==0 and MEMORY_ERROR==1. Presumably in some future we would gain other kind of error symbols, but when that happens is this meant to act as an enumeration of different kinds errors? Or an enumeration of bitmasks that can signal different kinds of errors?

If we mean "enum safe_result" is an enumeration of different kinds of errors, then the "result" variable and the returned value from here would be able to report a *single* kind of error, and it may be common to report the first error we encounter, in which case

    enum safe_result result = SUCCESS;
    enum safe_result res;

    res = sstrbuf_release(&jw->json);
    if (!result && res)
	result = res;
    res = sstrbuf_release(&jw->open_stack);
    if (!result && res)
	result = res;
    return result;

would be slightly longer, far easier to reason about, and is a lot more futureproof. What you wrote, with "||", does not really allow anything other than "is it still zero, or coalesce any non-zero value to 1".

On the other hand, if we mean "enum safe_result" is an enumeration of bitmasks, each bit representing different kind of error, then

    enum safe_result result = 0;

    result |= sstrbuf_release(&jw->json);
    result |= sstrbuf_release(&jw->open_stack);
    return result;

would probably be what you want. That way you can add different functions that returns different bit to signal a different kind of error and or it in.

    result |= some_function();

Junio C Hamano <gitster@pobox.com> writes:

I forgot the third possibility. Regardless of which interpretation of "enum safe_result" we use, if jw_release() is designed to say "0 for success, non-zero for failure", then almost as written but declaring "result" as a plain "int"

    int result = 0;

    result = sstrbuf_release(&jw->json) || result;
    result = sstrbuf_release(&jw->open_stack) || result;

    return result;

would probably make sense, even though the "|| result" construct is a bit unusual in C.

Thanks.

General comment: I really like the idea of this. While I haven't encountered this specific issue with git, I've dealt with similar issues in other systems, and even if the cascading error case is rare, it's incredibly frustrating to deal with the loss of error details because they used unsafe operations to generate their messages!

I'm not really qualified to comment much on the code yet, but there's a couple of small writing style things that I'll nitpick for clarity/readibilty. Feel free to ignore these if you disagree.

On Fri Sep 18, 2026 at 9:02 AM EDT, Derrick Stolee via GitGitGadget wrote:

I think we prefer to avoid "we" in these comments; and the "in particular" here feels a little abrupt - maybe "In order to ensure that strbuf functions can't call die, strbuf.c should not include ..."

Again, avoiding we; and I don't think the quotes belong there. 

Maybe "In the future, consumers that need a safe API will include ..."

The structure of this sentence is confusing. I had to read it a couple of times to figure out how to parse it. Better something like: "Future changes will include new implementations that return an error code instead of halting."

As with the prose comments above, safe shouldn't be in quotes.

I think this should be stronger - they shouldn't just be kept in mind, they should be strictly enforced: "strbufs have some invariants that _must_ be maintained".

I think "must" is better that "have to" here; I know some non-native english speakers get confused by that.

I think there should be a "must" before ensure".

I don't think "play" is good, and in any case it shouldn't be in quotes. Maybe "It is OK to manipulate the buffer directly..."

Again, the quote around private. (I had an undergrad advisor who was a stickler about the right way to use quotes, and he pounded into me so that now I die a little bit every time I see them used as emphasis or as a marker of "not really".)

Repeat comments above for the repetitions in the other file.

On Sat, Sep 19, 2026 at 04:23:41PM +0100, Phillip Wood wrote:

Yeah, I don't love the term "safe" here for two reasons:

  1. It implies the normal strbuf functions aren't safe. The flaw here
     is the "malloc failure is fatal" scheme, but that's just fine and
     shared with most of the rest of our code. I think we usually
     reserve safe/unsafe for things that you should be extra careful of
     using (like refs_resolve_ref_unsafe, or the unsafe hash algos).

  2. There are many types of safety, and this is just implementing one
     of them. ;) In particular, for the use case in the trace code we're
     looking at, I'd expect that we would want to avoid calling malloc()
     at all, because it relies on locks. So this new code would not be
     safe to call from a signal handler, for example.

So what I had imagined when seeing the initial subject lines was not strbufs who report malloc errors, but rather strbuf-like functions that operate on a fixed-size buffer (with either a static max-size like 4k, or perhaps a per-variable max-size recorded in the struct).

You can still run into errors, of course; we might run out of room in the buffer. But we'd see those cases deterministically for a given input, rather than occasionally when races or other external factors cause malloc to unexpectedly fail.

Agreed. I think that is a good approach for the static_strbuf idea above, too.

-Peff

"Mark C. Chu-Carroll" <markchucarroll@fastmail.com> writes:

If I understand correctly what this topic aims at, you'll see the "loss of error details" either way. Either we ran out of memory inside strbuf call and die, or we fail to allocate memory to format the details and end up not showing it.

The third word of your comment should not be "we" but "I", if that "we" intends to include me and others who wrote many commit log messages ;-)