[RFC] Create a 'safe' strbuf API

Documentation/git.adoc

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

Makefile

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

common-init.c

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

environment.h

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

json-writer.c

@@ -2,17 +2,25 @@
 
 #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)
 {
 	struct json_writer blank = JSON_WRITER_INIT;
 	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;
 }
 
 /*
@@ -98,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);
 	}
 }
 
@@ -234,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
@@ -254,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,

json-writer.h

@@ -70,7 +70,7 @@
  * of the given strings.
  */
 
-#include "strbuf.h"
+#include "strbuf-safe.h"
 
 struct json_writer
 {
@@ -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

meson.build

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

strbuf-safe.c

@@ -0,0 +1,52 @@
+#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;
+}
+
+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;
+}

strbuf-safe.h

@@ -0,0 +1,97 @@
+#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 }
+
+enum safe_result {
+	SUCCESS = 0,
+	MEMORY_ERROR,
+};
+
+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 */

strbuf.c

@@ -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++)
@@ -68,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)
@@ -105,13 +103,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)