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

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/ (local)

[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/ (local)

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