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

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