Create a 'safe' strbuf API

This is based on ds/trace2-tolerate-failed-timestamps [1] [2].

[1]
https://lore.kernel.org/git/[email protected]/

[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/[email protected]/

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

From: Derrick Stolee <[email protected]>

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.

---
 strbuf-safe.h | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
 strbuf.h      | 74 +++----------------------------------------
 2 files changed, 92 insertions(+), 70 deletions(-)
 create mode 100644 strbuf-safe.h

new file mode 100644
index 0000000000..3cf14545bb
@@ -0,0 +1,88 @@
+#ifndef STRBUF_SAFE_H
+#define STRBUF_SAFE_H
+
+/*
+ * NOTE FOR STRBUF DEVELOPERS
+ *
+ * strbuf is a low-level primitive; as such it should interact only
+ * with other low-level primitives. Do not introduce new functions
+ * which interact with higher-level APIs.
+ *
+ * This header file specifically conatins the "safe" API surface for
+ * working with strbufs. The implementations of these methods avoid
+ * using die() and other exits. Thus, these methods are appropriate
+ * for use within lower-level APIs such as trace2.
+ */
+
+struct string_list;
+
+/**
+ * strbufs are meant to be used with all the usual C string and memory
+ * APIs. Given that the length of the buffer is known, it's often better to
+ * use the mem* functions than a str* one (e.g., memchr vs. strchr).
+ * Though, one has to be careful about the fact that str* functions often
+ * stop on NULs and that strbufs may have embedded NULs.
+ *
+ * A strbuf is NUL terminated for convenience, but no function in the
+ * strbuf API actually relies on the string being free of NULs.
+ *
+ * strbufs have some invariants that are very important to keep in mind:
+ *
+ *  - The `buf` member is never NULL, so it can be used in any usual C
+ *    string operations safely. strbufs _have_ to be initialized either by
+ *    `strbuf_init()` or by `= STRBUF_INIT` before the invariants, though.
+ *
+ *    Do *not* assume anything on what `buf` really is (e.g. if it is
+ *    allocated memory or not), use `strbuf_detach()` to unwrap a memory
+ *    buffer from its strbuf shell in a safe way. That is the sole supported
+ *    way. This will give you a malloced buffer that you can later `free()`.
+ *
+ *    However, it is totally safe to modify anything in the string pointed by
+ *    the `buf` member, between the indices `0` and `len-1` (inclusive).
+ *
+ *  - The `buf` member is a byte array that has at least `len + 1` bytes
+ *    allocated. The extra byte is used to store a `'\0'`, allowing the
+ *    `buf` member to be a valid C-string. All strbuf functions ensure this
+ *    invariant is preserved.
+ *
+ *    NOTE: It is OK to "play" with the buffer directly if you work it this
+ *    way:
+ *
+ *        strbuf_grow(sb, SOME_SIZE); <1>
+ *        strbuf_setlen(sb, sb->len + SOME_OTHER_SIZE);
+ *
+ *    <1> Here, the memory array starting at `sb->buf`, and of length
+ *    `strbuf_avail(sb)` is all yours, and you can be sure that
+ *    `strbuf_avail(sb)` is at least `SOME_SIZE`.
+ *
+ *    NOTE: `SOME_OTHER_SIZE` must be smaller or equal to `strbuf_avail(sb)`.
+ *
+ *    Doing so is safe, though if it has to be done in many places, adding the
+ *    missing API to the strbuf module is the way to go.
+ *
+ *    WARNING: Do _not_ assume that the area that is yours is of size `alloc
+ *    - 1` even if it's true in the current implementation. Alloc is somehow a
+ *    "private" member that should not be messed with. Use `strbuf_avail()`
+ *    instead.
+*/
+
+/**
+ * Data Structures
+ * ---------------
+ */
+
+/**
+ * This is the string buffer structure. The `len` member can be used to
+ * determine the current length of the string, and `buf` member provides
+ * access to the string itself.
+ */
+struct strbuf {
+	size_t alloc;
+	size_t len;
+	char *buf;
+};
+
+extern char strbuf_slopbuf[];
+#define STRBUF_INIT  { .buf = strbuf_slopbuf }
+
+#endif /* STRBUF_SAFE_H */

gitgitgadget

From: Derrick Stolee <[email protected]>

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.

---
 Documentation/git.adoc |  6 ++++++
 common-init.c          |  2 ++
 environment.h          |  1 +
 wrapper.c              | 26 +++++++++++++++++---------
 wrapper.h              |  6 ++++++
 5 files changed, 32 insertions(+), 9 deletions(-)

index 8a5cdd3b3d..07da5c4f12 100644
@@ -688,6 +688,12 @@ For each path `GIT_EXTERNAL_DIFF` is called, two environment variables,
 
 other
 ~~~~~
+
+`GIT_ALLOC_LIMIT`::
+	A number limiting how much memory can be allocated in a single
+	hunk. This only limits single allocations and does not limit the
+	total memory used by the process.
+
 `GIT_MERGE_VERBOSITY`::
 	A number controlling the amount of output shown by
 	the recursive merge strategy.  Overrides merge.verbosity.

index d26c9c1f20..bf73c754b4 100644
@@ -39,6 +39,8 @@ static void setup_environment(void)
 	char *git_replace_ref_base;
 	const char *replace_ref_base;
 
+	initialize_git_alloc_limit();
+
 	if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
 		disable_replace_refs();
 	replace_ref_base = getenv(GIT_REPLACE_REF_BASE_ENVIRONMENT);

index e7ec5b0437..86b67da877 100644
@@ -5,6 +5,7 @@
 #include "branch.h"
 
 /* Double-check local_repo_env below if you add to this list. */
+#define GIT_ALLOC_LIMIT "GIT_ALLOC_LIMIT"
 #define GIT_DIR_ENVIRONMENT "GIT_DIR"
 #define GIT_COMMON_DIR_ENVIRONMENT "GIT_COMMON_DIR"
 #define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"

index 561f9ee9c9..3de6b21cc2 100644
@@ -6,6 +6,7 @@
 
 #include "git-compat-util.h"
 #include "abspath.h"
+#include "environment.h"
 #include "parse.h"
 #include "gettext.h"
 #include "strbuf.h"

@@ -18,22 +19,29 @@
 #undef SystemFunction036
 #endif
 
-static int memory_limit_check(size_t size, int gentle)
+static size_t git_alloc_limit = 0;
+
+void initialize_git_alloc_limit(void)
 {
-	static size_t limit = 0;
-	if (!limit) {
-		limit = git_env_ulong("GIT_ALLOC_LIMIT", 0);
-		if (!limit)
-			limit = SIZE_MAX;
+	if (!git_alloc_limit) {
+		git_alloc_limit = git_env_ulong(GIT_ALLOC_LIMIT, 0);
+		if (!git_alloc_limit)
+			git_alloc_limit = SIZE_MAX;
 	}
-	if (size > limit) {
+}
+
+static int memory_limit_check(size_t size, int gentle)
+{
+	initialize_git_alloc_limit();
+
+	if (size > git_alloc_limit) {
 		if (gentle) {
 			error("attempting to allocate %"PRIuMAX" over limit %"PRIuMAX,
-			      (uintmax_t)size, (uintmax_t)limit);
+			      (uintmax_t)size, (uintmax_t)git_alloc_limit);
 			return -1;
 		} else
 			die("attempting to allocate %"PRIuMAX" over limit %"PRIuMAX,
-			    (uintmax_t)size, (uintmax_t)limit);
+			    (uintmax_t)size, (uintmax_t)git_alloc_limit);
 	}
 	return 0;
 }

index a6287d7f4d..69df68ee7a 100644
@@ -180,4 +180,10 @@ static inline unsigned log2u(uintmax_t sz)
 	return l - 1;
 }
 
+/*
+ * Initialize the global state for GIT_ALLOC_LIMIT at an appropriate
+ * time so it can be effective for safe allocation methods.
+ */
+void initialize_git_alloc_limit(void);
+
 #endif /* WRAPPER_H */

gitgitgadget

From: Derrick Stolee <[email protected]>

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.

---
 wrapper.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

index 3de6b21cc2..97a29bda75 100644
@@ -30,22 +30,31 @@ void initialize_git_alloc_limit(void)
 	}
 }
 
-static int memory_limit_check(size_t size, int gentle)
+static int safe_memory_limit_check(size_t size, int verbose)
 {
-	initialize_git_alloc_limit();
-
-	if (size > git_alloc_limit) {
-		if (gentle) {
+	size_t limit = git_alloc_limit ? git_alloc_limit : SIZE_MAX;
+	if (size > limit) {
+		if (verbose)
 			error("attempting to allocate %"PRIuMAX" over limit %"PRIuMAX,
 			      (uintmax_t)size, (uintmax_t)git_alloc_limit);
-			return -1;
-		} else
-			die("attempting to allocate %"PRIuMAX" over limit %"PRIuMAX,
-			    (uintmax_t)size, (uintmax_t)git_alloc_limit);
+		return -1;
 	}
 	return 0;
 }
 
+static int memory_limit_check(size_t size, int gentle)
+{
+	int res;
+	initialize_git_alloc_limit();
+
+	res = safe_memory_limit_check(size, gentle);
+	if (res && !gentle) {
+		die("attempting to allocate %"PRIuMAX" over limit %"PRIuMAX,
+		    (uintmax_t)size, (uintmax_t)git_alloc_limit);
+	}
+	return res;
+}
+
 char *xstrdup(const char *str)
 {
 	char *ret = strdup(str);

gitgitgadget

From: Derrick Stolee <[email protected]>

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.

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

index d4b775953d..5943853219 100644
@@ -1327,6 +1327,7 @@ LIB_OBJS += sparse-index.o
 LIB_OBJS += split-index.o
 LIB_OBJS += stable-qsort.o
 LIB_OBJS += statinfo.o
+LIB_OBJS += strbuf-safe.o
 LIB_OBJS += strbuf.o
 LIB_OBJS += string-list.o
 LIB_OBJS += strmap.o

index d86f2acd2b..368fdd00d5 100644
@@ -532,6 +532,7 @@ libgit_sources = [
   'split-index.c',
   'stable-qsort.c',
   'statinfo.c',
+  'strbuf-safe.c',
   'strbuf.c',
   'string-list.c',
   'strmap.c',

new file mode 100644
index 0000000000..e4a0707d63
@@ -0,0 +1,34 @@
+#include "git-compat-util.h"
+#include "strbuf-safe.h"
+#include "banned-die.h"
+
+/*
+ * A safe version of ALLOC_GROW from git-compat-util.h and
+ * xrealloc() from wrapper.c.
+ */
+#define SAFE_ALLOC_GROW(x, nr, alloc) \
+	do { \
+		if ((nr) > alloc) { \
+			if (alloc_nr(alloc) < (nr)) \
+				alloc = (nr); \
+			else \
+				alloc = alloc_nr(alloc); \
+			if (srealloc((void **)&(x), alloc)) \
+				return MEMORY_ERROR; \
+		} \
+	} while (0)
+
+enum safe_result sstrbuf_grow(struct strbuf *sb, size_t extra)
+{
+	int new_buf = !sb->alloc;
+	size_t new_len = st_add3(sb->len, extra, 1);
+	if (new_buf)
+		sb->buf = NULL;
+
+	SAFE_ALLOC_GROW(sb->buf, new_len, sb->alloc);
+
+	if (new_buf)
+		sb->buf[0] = '\0';
+
+	return SUCCESS;
+}

index 3cf14545bb..f6adf7434b 100644
@@ -85,4 +85,11 @@ struct strbuf {
 extern char strbuf_slopbuf[];
 #define STRBUF_INIT  { .buf = strbuf_slopbuf }
 
+enum safe_result {
+	SUCCESS = 0,
+	MEMORY_ERROR,
+};
+
+enum safe_result sstrbuf_grow(struct strbuf *sb, size_t extra);
+
 #endif /* STRBUF_SAFE_H */

index 44955669e8..d005666a07 100644
@@ -8,6 +8,8 @@
 #include "utf8.h"
 #include "date.h"
 
+#define STRBUF_DIE(f) die(_("unexpected error during string manipulation: %s"), f)
+
 bool starts_with(const char *str, const char *prefix)
 {
 	for (; ; str++, prefix++)

@@ -105,13 +107,8 @@ void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc)
 
 void strbuf_grow(struct strbuf *sb, size_t extra)
 {
-	int new_buf = !sb->alloc;
-	size_t new_len = st_add3(sb->len, extra, 1);
-	if (new_buf)
-		sb->buf = NULL;
-	ALLOC_GROW(sb->buf, new_len, sb->alloc);
-	if (new_buf)
-		sb->buf[0] = '\0';
+	if (sstrbuf_grow(sb, extra))
+		STRBUF_DIE("strbuf_grow");
 }
 
 void strbuf_trim(struct strbuf *sb)

index 97a29bda75..69ff9a8ff6 100644
@@ -144,20 +144,28 @@ int xstrncmpz(const char *s, const char *t, size_t len)
 	return s[len] == '\0' ? 0 : 1;
 }
 
-void *xrealloc(void *ptr, size_t size)
+int srealloc(void **ptr, size_t size)
 {
-	void *ret;
-
 	if (!size) {
-		free(ptr);
-		return xmalloc(0);
+		free(*ptr);
+		if ((*ptr = malloc(1)))
+			return 0;
+		return -1;
 	}
 
-	memory_limit_check(size, 0);
-	ret = realloc(ptr, size);
-	if (!ret)
+	if (safe_memory_limit_check(size, 0))
+		return -1;
+	if ((*ptr = realloc(*ptr, size)))
+		return 0;
+
+	return -1;
+}
+
+void *xrealloc(void *ptr, size_t size)
+{
+	if (srealloc(&ptr, size))
 		die("Out of memory, realloc failed");
-	return ret;
+	return ptr;
 }
 
 void *xcalloc(size_t nmemb, size_t size)

index 69df68ee7a..956de2c534 100644
@@ -27,6 +27,9 @@ char *xgetcwd(void);
 FILE *fopen_for_writing(const char *path);
 FILE *fopen_or_warn(const char *path, const char *mode);
 
+/* safe versions of helpers above. */
+int srealloc(void **ptr, size_t size);
+
 /*
  * Like strncmp, but only return zero if s is NUL-terminated and exactly len
  * characters long.  If it is not, consider it greater than t.

gitgitgadget

From: Derrick Stolee <[email protected]>

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.

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

index 34577dc25f..e7fc5775da 100644
@@ -2,6 +2,7 @@
 
 #include "git-compat-util.h"
 #include "json-writer.h"
+#include "strbuf.h"
 
 void jw_init(struct json_writer *jw)
 {

index 8f845d4d29..fa8cf02253 100644
@@ -70,7 +70,7 @@
  * of the given strings.
  */
 
-#include "strbuf.h"
+#include "strbuf-safe.h"
 
 struct json_writer
 {

index 36a746cc10..b25fa0fb30 100644
@@ -5,6 +5,7 @@
 #include "json-writer.h"
 #include "repository.h"
 #include "run-command.h"
+#include "strbuf.h"
 #include "version.h"
 #include "trace2/tr2_dst.h"
 #include "trace2/tr2_tbuf.h"

index 96a5bc7f10..5554081c3c 100644
@@ -7,6 +7,7 @@
 #include "quote.h"
 #include "version.h"
 #include "json-writer.h"
+#include "strbuf.h"
 #include "trace2/tr2_dst.h"
 #include "trace2/tr2_sid.h"
 #include "trace2/tr2_sysenv.h"

gitgitgadget

From: Derrick Stolee <[email protected]>

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.

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

index e7fc5775da..38351f3439 100644
@@ -3,6 +3,8 @@
 #include "git-compat-util.h"
 #include "json-writer.h"
 #include "strbuf.h"
+/* banned-die must be last. */
+#include "banned-die.h"
 
 void jw_init(struct json_writer *jw)
 {

@@ -10,10 +12,15 @@ void jw_init(struct json_writer *jw)
 	memcpy(jw, &blank, sizeof(*jw));;
 }
 
-void jw_release(struct json_writer *jw)
+int jw_release(struct json_writer *jw)
 {
-	strbuf_release(&jw->json);
-	strbuf_release(&jw->open_stack);
+	enum safe_result result = SUCCESS;
+
+	/* attempt both removals without short-circuiting. */
+	result = sstrbuf_release(&jw->json) || result;
+	result = sstrbuf_release(&jw->open_stack) || result;
+
+	return result;
 }
 
 /*

@@ -99,16 +106,17 @@ static void maybe_add_comma(struct json_writer *jw)
 		jw->need_comma = 1;
 }
 
-static void fmt_double(struct json_writer *jw, int precision,
-			      double value)
+static int fmt_double(struct json_writer *jw, int precision,
+		      double value)
 {
 	if (precision < 0) {
 		strbuf_addf(&jw->json, "%f", value);
+		return 0;
 	} else {
 		struct strbuf fmt = STRBUF_INIT;
 		strbuf_addf(&fmt, "%%.%df", precision);
 		strbuf_addf(&jw->json, fmt.buf, value);
-		strbuf_release(&fmt);
+		return sstrbuf_release(&fmt);
 	}
 }
 

@@ -235,8 +243,8 @@ static void kill_indent(struct strbuf *sb,
 	}
 }
 
-static void append_sub_jw(struct json_writer *jw,
-			  const struct json_writer *value)
+static int append_sub_jw(struct json_writer *jw,
+			 const struct json_writer *value)
 {
 	/*
 	 * If both are pretty, increase the indentation of the sub_jw

@@ -255,18 +263,17 @@ static void append_sub_jw(struct json_writer *jw,
 		struct strbuf sb = STRBUF_INIT;
 		increase_indent(&sb, value, jw->open_stack.len * 2);
 		strbuf_addbuf(&jw->json, &sb);
-		strbuf_release(&sb);
-		return;
+		return sstrbuf_release(&sb);
 	}
 	if (!jw->pretty && value->pretty) {
 		struct strbuf sb = STRBUF_INIT;
 		kill_indent(&sb, value);
 		strbuf_addbuf(&jw->json, &sb);
-		strbuf_release(&sb);
-		return;
+		return sstrbuf_release(&sb);
 	}
 
 	strbuf_addbuf(&jw->json, &value->json);
+	return 0;
 }
 
 void jw_object_sub_jw(struct json_writer *jw, const char *key,

index fa8cf02253..72277d9839 100644
@@ -103,9 +103,10 @@ struct json_writer
 void jw_init(struct json_writer *jw);
 
 /*
- * Release the internal buffers of a json_writer.
+ * Release the internal buffers of a json_writer. Returns nonzero on
+ * failure.
  */
-void jw_release(struct json_writer *jw);
+int jw_release(struct json_writer *jw);
 
 /*
  * Begin the json_writer using an object as the top-level data structure. If

index e4a0707d63..7a8701e827 100644
@@ -32,3 +32,21 @@ enum safe_result sstrbuf_grow(struct strbuf *sb, size_t extra)
 
 	return SUCCESS;
 }
+
+enum safe_result sstrbuf_init(struct strbuf *sb, size_t hint)
+{
+	struct strbuf blank = STRBUF_INIT;
+	memcpy(sb, &blank, sizeof(*sb));
+	if (!hint)
+		return 0;
+	return sstrbuf_grow(sb, hint);
+}
+
+enum safe_result sstrbuf_release(struct strbuf *sb)
+{
+	if (sb->alloc) {
+		free(sb->buf);
+		return sstrbuf_init(sb, 0);
+	}
+	return 0;
+}

index f6adf7434b..fe04d9cf62 100644
@@ -91,5 +91,7 @@ enum safe_result {
 };
 
 enum safe_result sstrbuf_grow(struct strbuf *sb, size_t extra);
+enum safe_result sstrbuf_init(struct strbuf *sb, size_t hint);
+enum safe_result sstrbuf_release(struct strbuf *sb);
 
 #endif /* STRBUF_SAFE_H */

index d005666a07..835238dc64 100644
@@ -70,18 +70,14 @@ char strbuf_slopbuf[1];
 
 void strbuf_init(struct strbuf *sb, size_t hint)
 {
-	struct strbuf blank = STRBUF_INIT;
-	memcpy(sb, &blank, sizeof(*sb));
-	if (hint)
-		strbuf_grow(sb, hint);
+	if (sstrbuf_init(sb, hint))
+		STRBUF_DIE("strbuf_init");
 }
 
 void strbuf_release(struct strbuf *sb)
 {
-	if (sb->alloc) {
-		free(sb->buf);
-		strbuf_init(sb, 0);
-	}
+	if (sstrbuf_release(sb))
+		STRBUF_DIE("strbuf_release");
 }
 
 char *strbuf_detach(struct strbuf *sb, size_t *sz)

gitgitgadget

Hi Stolee

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

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.

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.

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

Mark Craig Chu-Carroll (@MarkChuCarroll at gitlab)
*** Software Tools/Math Geek - Software Engineer at Gitlab
*** Work Email: [email protected] / [email protected]
*** Personal Blog: http://goodmath.org/blog / Personal email: [email protected]

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

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

...> So what I had imagined when seeing the initial subject lines was not

Thanks for taking the time to consider the RFC. Sorry I'm so late in responding, but I find your feedback insightful. It requires starting over on this direction, but I don't currently have time to do so. Maybe I'll give this a try again in the future, but I'll set it down for now.

Thanks,
-Stolee