{"thread":{"id":"66346","subject":"[PATCH 0/6] [RFC] Create a 'safe' strbuf API","startedAt":"2026-09-18T13:02:23Z","lastAt":"2026-09-23T20:14:41Z","messageCount":16,"participants":["Derrick Stolee via GitGitGadget","Phillip Wood","Junio C Hamano","Mark C. Chu-Carroll","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"552857","messageId":"pull.2230.git.1789736540.gitgitgadget@gmail.com","threadId":"66346","inReplyTo":null,"subject":"[PATCH 0/6] [RFC] Create a 'safe' strbuf API","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-18T13:02:14Z","receivedAt":"2026-09-18T13:02:23Z","isPatch":true,"body":"This is based on ds/trace2-tolerate-failed-timestamps [1] [2].\n\n[1]\nhttps://lore.kernel.org/git/pull.2178.v3.git.1788197143.gitgitgadget@gmail.com/\n\n[2] https://github.com/gitgitgadget/git/pull/2178\n\nWhile investigating the fact that the trace2 API can trigger recursive die()\nloops if allocation fails, Peff pointed out [3] that trace2 uses json-writer\nwhich in turn uses the strbuf API. If a strbuf fails to allocate, grow, or\notherwise mutate the given strings, then trace2 can hit this problem!\n\n[3]\nhttps://lore.kernel.org/git/20260901050129.GB1075462@coredump.intra.peff.net/\n\nThe goal of this short RFC, such as it is, is to get some feedback on\nwhether this is a worthwhile direction to pursue or if I should abandon this\nidea of having this definition of \"safe\" for some APIs. This decision may\nalso determine if we should abandon ds/trace2-tolerate-failed-timestamps or\nleave the existing behavior as-is.\n\nI had discussed earlier that what we'd really need is a guarantee that we\ncan't transitively reach die() from any \"safe\" API. The eventual goal would\nbe to include json-writer.c and the trace2 code files into the \"safe\"\nbucket, but for now I'm making sure that strbuf-safe.c satisfies this CodeQL\nquery:\n\nimport cpp\n\nclass SafeFunction extends Function {\n SafeFunction() {\n getFile().getRelativePath() = \"strbuf-safe.c\"\n }\n}\n\npredicate directlyCalls(Function caller, Function callee) {\n exists(FunctionCall call |\n call.getEnclosingFunction() = caller and\n call.getTarget() = callee\n )\n}\n\n\nfrom SafeFunction source, Function sink\nwhere\n (sink.getName() = \"die\" or sink.getName() = \"exit\") and\n directlyCalls+(source, sink)\nselect source,\n \"This safe function can transitively reach \" + sink.getName() + \"().\"\n\n\nIf we went with this approach, then I'd explore how to make this a\nbuild-time requirement during CI.\n\nIn regards to the structure of this RFC:\n\n 1. The safe API needs the same structures, but shouldn't import more than\n necessary. Some movement of structs across headers is done before\n anything else.\n 2. In order to make even the smallest safe method work, we first need to\n figure out how to handle GIT_ALLOC_LIMIT, which is an undocumented\n environment variable. I explain that I think this should be\n GIT_TEST_ALLOC_LIMIT, but maybe the ship has sailed due to Hyrum's Law.\n So I make an effort to document it but also to initialize it proactively\n within the process startup instead of implicitly at the lowest level.\n This allows us to avoid a die() when checking the environment variable.\n 3. Thus, we get a 'safe' version of a memory allocation size check. This is\n our first example of creating a safe version that is then called by the\n non-safe version to prevent repeated code.\n 4. We can then create our first safe strbuf method: sstrbuf_grow(). I\n explain why I prepend with s instead of appending _gently in the commit.\n 5. Some trace2 code implicitly depends on strbuf.h through json-writer.h,\n so we drop that in favor of strbuf-safe.h to keep the dependence on the\n full struct definition without forever having the non-safe methods\n reachable. The goal eventually is to drop the strbuf.h include from\n json-writer.c, but that isn't accomplished in this RFC.\n 6. Finally, create safe init and release methods and use them in\n json-writer.c. This does show some of the \"transition risk\" where some\n json-writer methods become \"safe\" but I haven't done the hard work to\n make sure the callers of those methods respond to the new return values.\n If we proceed with the RFC, then I'd split this into a creation of the\n safe strbuf methods and then the refactoring required to respond\n correctly to errors in json-writer.c\n\nThanks in advance for your thoughts!\n\nThanks, -Stolee\n\nDerrick Stolee (6):\n strbuf: add header for 'safe' API\n wrapper: initialize GIT_ALLOC_LIMIT proactively\n wrapper: create safe_memory_limit_check()\n strbuf-safe: add sstrbuf_grow()\n json-writer: include strbuf-safe.h\n strbuf-safe: add init and release methods\n\n Documentation/git.adoc | 6 +++\n Makefile | 1 +\n common-init.c | 2 +\n environment.h | 1 +\n json-writer.c | 32 ++++++++------\n json-writer.h | 7 +--\n meson.build | 1 +\n strbuf-safe.c | 52 ++++++++++++++++++++++\n strbuf-safe.h | 97 ++++++++++++++++++++++++++++++++++++++++++\n strbuf.c | 23 ++++------\n strbuf.h | 74 ++------------------------------\n trace2/tr2_tgt_event.c | 1 +\n trace2/tr2_tgt_perf.c | 1 +\n wrapper.c | 67 ++++++++++++++++++++---------\n wrapper.h | 9 ++++\n 15 files changed, 253 insertions(+), 121 deletions(-)\n create mode 100644 strbuf-safe.c\n create mode 100644 strbuf-safe.h\n\n\nbase-commit: a80c36bda0e5aff1c9945d08f43079a6aa85ccad\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2230%2Fderrickstolee%2Fstrbuf-safe-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2230/derrickstolee/strbuf-safe-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/2230\n-- \ngitgitgadget\n"},{"id":"552858","messageId":"b1779709120adc9c1df40c7210481d6bed9791c5.1789736540.git.gitgitgadget@gmail.com","threadId":"66346","inReplyTo":"pull.2230.git.1789736540.gitgitgadget@gmail.com","subject":"[PATCH 1/6] strbuf: add header for 'safe' API","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-18T13:02:15Z","receivedAt":"2026-09-18T13:02:24Z","isPatch":true,"body":"From: Derrick Stolee \n\nThe strbuf library is an important API used all over the Git codebase.\nContributors use it in nearly any string-manipulating action. However, the\nimplementation uses other helping functions that die() on failure instead of\nreturning an error code. Thus, the strbuf API isn't _safe_.\n\nIn particular, we cannot include 'banned-die.h' in 'strbuf.c'.\n\nTo start the creation of a safe strbuf API, move the struct definition into\na new 'strbuf-safe.h' header file. All consumers of 'strbuf.h' will consume\nthat header transitively.\n\nIn the future, we will hope to have consumers that need a 'safe' API will\ninclude 'strbuf-safe.h' instead of 'strbuf.h'.\n\nWe will see in future changes the inclusion of new implementations that\nreturn an error code instead of halting.\n\nSigned-off-by: Derrick Stolee \n---\n strbuf-safe.h | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++\n strbuf.h | 74 +++----------------------------------------\n 2 files changed, 92 insertions(+), 70 deletions(-)\n create mode 100644 strbuf-safe.h\n\ndiff --git a/strbuf-safe.h b/strbuf-safe.h\nnew file mode 100644\nindex 0000000000..3cf14545bb\n--- /dev/null\n+++ b/strbuf-safe.h\n@@ -0,0 +1,88 @@\n+#ifndef STRBUF_SAFE_H\n+#define STRBUF_SAFE_H\n+\n+/*\n+ * NOTE FOR STRBUF DEVELOPERS\n+ *\n+ * strbuf is a low-level primitive; as such it should interact only\n+ * with other low-level primitives. Do not introduce new functions\n+ * which interact with higher-level APIs.\n+ *\n+ * This header file specifically conatins the \"safe\" API surface for\n+ * working with strbufs. The implementations of these methods avoid\n+ * using die() and other exits. Thus, these methods are appropriate\n+ * for use within lower-level APIs such as trace2.\n+ */\n+\n+struct string_list;\n+\n+/**\n+ * strbufs are meant to be used with all the usual C string and memory\n+ * APIs. Given that the length of the buffer is known, it's often better to\n+ * use the mem* functions than a str* one (e.g., memchr vs. strchr).\n+ * Though, one has to be careful about the fact that str* functions often\n+ * stop on NULs and that strbufs may have embedded NULs.\n+ *\n+ * A strbuf is NUL terminated for convenience, but no function in the\n+ * strbuf API actually relies on the string being free of NULs.\n+ *\n+ * strbufs have some invariants that are very important to keep in mind:\n+ *\n+ * - The `buf` member is never NULL, so it can be used in any usual C\n+ * string operations safely. strbufs _have_ to be initialized either by\n+ * `strbuf_init()` or by `= STRBUF_INIT` before the invariants, though.\n+ *\n+ * Do *not* assume anything on what `buf` really is (e.g. if it is\n+ * allocated memory or not), use `strbuf_detach()` to unwrap a memory\n+ * buffer from its strbuf shell in a safe way. That is the sole supported\n+ * way. This will give you a malloced buffer that you can later `free()`.\n+ *\n+ * However, it is totally safe to modify anything in the string pointed by\n+ * the `buf` member, between the indices `0` and `len-1` (inclusive).\n+ *\n+ * - The `buf` member is a byte array that has at least `len + 1` bytes\n+ * allocated. The extra byte is used to store a `'\\0'`, allowing the\n+ * `buf` member to be a valid C-string. All strbuf functions ensure this\n+ * invariant is preserved.\n+ *\n+ * NOTE: It is OK to \"play\" with the buffer directly if you work it this\n+ * way:\n+ *\n+ * strbuf_grow(sb, SOME_SIZE); <1>\n+ * strbuf_setlen(sb, sb->len + SOME_OTHER_SIZE);\n+ *\n+ * <1> Here, the memory array starting at `sb->buf`, and of length\n+ * `strbuf_avail(sb)` is all yours, and you can be sure that\n+ * `strbuf_avail(sb)` is at least `SOME_SIZE`.\n+ *\n+ * NOTE: `SOME_OTHER_SIZE` must be smaller or equal to `strbuf_avail(sb)`.\n+ *\n+ * Doing so is safe, though if it has to be done in many places, adding the\n+ * missing API to the strbuf module is the way to go.\n+ *\n+ * WARNING: Do _not_ assume that the area that is yours is of size `alloc\n+ * - 1` even if it's true in the current implementation. Alloc is somehow a\n+ * \"private\" member that should not be messed with. Use `strbuf_avail()`\n+ * instead.\n+*/\n+\n+/**\n+ * Data Structures\n+ * ---------------\n+ */\n+\n+/**\n+ * This is the string buffer structure. The `len` member can be used to\n+ * determine the current length of the string, and `buf` member provides\n+ * access to the string itself.\n+ */\n+struct strbuf {\n+\tsize_t alloc;\n+\tsize_t len;\n+\tchar *buf;\n+};\n+\n+extern char strbuf_slopbuf[];\n+#define STRBUF_INIT { .buf = strbuf_slopbuf }\n+\n+#endif /* STRBUF_SAFE_H */\ndiff --git a/strbuf.h b/strbuf.h\nindex 1089ae687b..b41f8ef901 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -1,85 +1,19 @@\n #ifndef STRBUF_H\n #define STRBUF_H\n \n+#include \"strbuf-safe.h\"\n+\n /*\n * NOTE FOR STRBUF DEVELOPERS\n *\n * strbuf is a low-level primitive; as such it should interact only\n * with other low-level primitives. Do not introduce new functions\n * which interact with higher-level APIs.\n- */\n-\n-struct string_list;\n-\n-/**\n- * strbufs are meant to be used with all the usual C string and memory\n- * APIs. Given that the length of the buffer is known, it's often better to\n- * use the mem* functions than a str* one (e.g., memchr vs. strchr).\n- * Though, one has to be careful about the fact that str* functions often\n- * stop on NULs and that strbufs may have embedded NULs.\n- *\n- * A strbuf is NUL terminated for convenience, but no function in the\n- * strbuf API actually relies on the string being free of NULs.\n- *\n- * strbufs have some invariants that are very important to keep in mind:\n- *\n- * - The `buf` member is never NULL, so it can be used in any usual C\n- * string operations safely. strbufs _have_ to be initialized either by\n- * `strbuf_init()` or by `= STRBUF_INIT` before the invariants, though.\n- *\n- * Do *not* assume anything on what `buf` really is (e.g. if it is\n- * allocated memory or not), use `strbuf_detach()` to unwrap a memory\n- * buffer from its strbuf shell in a safe way. That is the sole supported\n- * way. This will give you a malloced buffer that you can later `free()`.\n- *\n- * However, it is totally safe to modify anything in the string pointed by\n- * the `buf` member, between the indices `0` and `len-1` (inclusive).\n- *\n- * - The `buf` member is a byte array that has at least `len + 1` bytes\n- * allocated. The extra byte is used to store a `'\\0'`, allowing the\n- * `buf` member to be a valid C-string. All strbuf functions ensure this\n- * invariant is preserved.\n- *\n- * NOTE: It is OK to \"play\" with the buffer directly if you work it this\n- * way:\n *\n- * strbuf_grow(sb, SOME_SIZE); <1>\n- * strbuf_setlen(sb, sb->len + SOME_OTHER_SIZE);\n- *\n- * <1> Here, the memory array starting at `sb->buf`, and of length\n- * `strbuf_avail(sb)` is all yours, and you can be sure that\n- * `strbuf_avail(sb)` is at least `SOME_SIZE`.\n- *\n- * NOTE: `SOME_OTHER_SIZE` must be smaller or equal to `strbuf_avail(sb)`.\n- *\n- * Doing so is safe, though if it has to be done in many places, adding the\n- * missing API to the strbuf module is the way to go.\n- *\n- * WARNING: Do _not_ assume that the area that is yours is of size `alloc\n- * - 1` even if it's true in the current implementation. Alloc is somehow a\n- * \"private\" member that should not be messed with. Use `strbuf_avail()`\n- * instead.\n-*/\n-\n-/**\n- * Data Structures\n- * ---------------\n+ * Also see strbuf-safe.h for the struct definitions and safe versions\n+ * of some methods declared in this header file.\n */\n \n-/**\n- * This is the string buffer structure. The `len` member can be used to\n- * determine the current length of the string, and `buf` member provides\n- * access to the string itself.\n- */\n-struct strbuf {\n-\tsize_t alloc;\n-\tsize_t len;\n-\tchar *buf;\n-};\n-\n-extern char strbuf_slopbuf[];\n-#define STRBUF_INIT { .buf = strbuf_slopbuf }\n-\n struct object_id;\n \n /**\n-- \ngitgitgadget\n\n"},{"id":"552859","messageId":"8d30730feac37d2cd42c969dca3f33c007c2065e.1789736540.git.gitgitgadget@gmail.com","threadId":"66346","inReplyTo":"pull.2230.git.1789736540.gitgitgadget@gmail.com","subject":"[PATCH 2/6] wrapper: initialize GIT_ALLOC_LIMIT proactively","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-18T13:02:16Z","receivedAt":"2026-09-18T13:02:25Z","isPatch":true,"body":"From: Derrick Stolee \n\nBefore making a safe version of memory_limit_check(), create\ninitialize_git_alloc_limit() to externalize the static memory limit stored\nin that method. Initialize this intentionally during setup_environment()\ninstead of implicitly during lower-level allocations.\n\nThis will allow a future version of memory_limit_check() that doesn't call\ndie() at all, which will require not calling git_env_ulong() directly. This\ncomes with some assumption that initialize_git_alloc_limit() is called\nbefore moving into safe APIs, though we will make some reaonable assumptions\nin those cases.\n\nThe GIT_ALLOC_LIMIT environment variable is used by some tests, but is\notherwise not advertised. It was added by d41489a642 (Add more large blob\ntest cases, 2012-03-07), which may predate the GIT_TEST_ pattern. This is\nlong enough that it may be possible that someone depends on it in the wild.\nThus, I'm choosing to document it instead of renaming it to\nGIT_TEST_ALLOC_LIMIT.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/git.adoc | 6 ++++++\n common-init.c | 2 ++\n environment.h | 1 +\n wrapper.c | 26 +++++++++++++++++---------\n wrapper.h | 6 ++++++\n 5 files changed, 32 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git.adoc b/Documentation/git.adoc\nindex 8a5cdd3b3d..07da5c4f12 100644\n--- a/Documentation/git.adoc\n+++ b/Documentation/git.adoc\n@@ -688,6 +688,12 @@ For each path `GIT_EXTERNAL_DIFF` is called, two environment variables,\n \n other\n ~~~~~\n+\n+`GIT_ALLOC_LIMIT`::\n+\tA number limiting how much memory can be allocated in a single\n+\thunk. This only limits single allocations and does not limit the\n+\ttotal memory used by the process.\n+\n `GIT_MERGE_VERBOSITY`::\n \tA number controlling the amount of output shown by\n \tthe recursive merge strategy. Overrides merge.verbosity.\ndiff --git a/common-init.c b/common-init.c\nindex d26c9c1f20..bf73c754b4 100644\n--- a/common-init.c\n+++ b/common-init.c\n@@ -39,6 +39,8 @@ static void setup_environment(void)\n \tchar *git_replace_ref_base;\n \tconst char *replace_ref_base;\n \n+\tinitialize_git_alloc_limit();\n+\n \tif (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))\n \t\tdisable_replace_refs();\n \treplace_ref_base = getenv(GIT_REPLACE_REF_BASE_ENVIRONMENT);\ndiff --git a/environment.h b/environment.h\nindex e7ec5b0437..86b67da877 100644\n--- a/environment.h\n+++ b/environment.h\n@@ -5,6 +5,7 @@\n #include \"branch.h\"\n \n /* Double-check local_repo_env below if you add to this list. */\n+#define GIT_ALLOC_LIMIT \"GIT_ALLOC_LIMIT\"\n #define GIT_DIR_ENVIRONMENT \"GIT_DIR\"\n #define GIT_COMMON_DIR_ENVIRONMENT \"GIT_COMMON_DIR\"\n #define GIT_NAMESPACE_ENVIRONMENT \"GIT_NAMESPACE\"\ndiff --git a/wrapper.c b/wrapper.c\nindex 561f9ee9c9..3de6b21cc2 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -6,6 +6,7 @@\n \n #include \"git-compat-util.h\"\n #include \"abspath.h\"\n+#include \"environment.h\"\n #include \"parse.h\"\n #include \"gettext.h\"\n #include \"strbuf.h\"\n@@ -18,22 +19,29 @@\n #undef SystemFunction036\n #endif\n \n-static int memory_limit_check(size_t size, int gentle)\n+static size_t git_alloc_limit = 0;\n+\n+void initialize_git_alloc_limit(void)\n {\n-\tstatic size_t limit = 0;\n-\tif (!limit) {\n-\t\tlimit = git_env_ulong(\"GIT_ALLOC_LIMIT\", 0);\n-\t\tif (!limit)\n-\t\t\tlimit = SIZE_MAX;\n+\tif (!git_alloc_limit) {\n+\t\tgit_alloc_limit = git_env_ulong(GIT_ALLOC_LIMIT, 0);\n+\t\tif (!git_alloc_limit)\n+\t\t\tgit_alloc_limit = SIZE_MAX;\n \t}\n-\tif (size > limit) {\n+}\n+\n+static int memory_limit_check(size_t size, int gentle)\n+{\n+\tinitialize_git_alloc_limit();\n+\n+\tif (size > git_alloc_limit) {\n \t\tif (gentle) {\n \t\t\terror(\"attempting to allocate %\"PRIuMAX\" over limit %\"PRIuMAX,\n-\t\t\t (uintmax_t)size, (uintmax_t)limit);\n+\t\t\t (uintmax_t)size, (uintmax_t)git_alloc_limit);\n \t\t\treturn -1;\n \t\t} else\n \t\t\tdie(\"attempting to allocate %\"PRIuMAX\" over limit %\"PRIuMAX,\n-\t\t\t (uintmax_t)size, (uintmax_t)limit);\n+\t\t\t (uintmax_t)size, (uintmax_t)git_alloc_limit);\n \t}\n \treturn 0;\n }\ndiff --git a/wrapper.h b/wrapper.h\nindex a6287d7f4d..69df68ee7a 100644\n--- a/wrapper.h\n+++ b/wrapper.h\n@@ -180,4 +180,10 @@ static inline unsigned log2u(uintmax_t sz)\n \treturn l - 1;\n }\n \n+/*\n+ * Initialize the global state for GIT_ALLOC_LIMIT at an appropriate\n+ * time so it can be effective for safe allocation methods.\n+ */\n+void initialize_git_alloc_limit(void);\n+\n #endif /* WRAPPER_H */\n-- \ngitgitgadget\n\n"},{"id":"552860","messageId":"3b3c67243d200a42aa105981b64228e2cbb35a6c.1789736540.git.gitgitgadget@gmail.com","threadId":"66346","inReplyTo":"pull.2230.git.1789736540.gitgitgadget@gmail.com","subject":"[PATCH 3/6] wrapper: create safe_memory_limit_check()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-18T13:02:17Z","receivedAt":"2026-09-18T13:02:27Z","isPatch":true,"body":"From: Derrick Stolee \n\nThe existing memory_limit_check() is used in many places within wrapper.c,\nbut because it initializes the GIT_ALLOC_LIMIT environment variable _and_\ncan call die() when not in gentle mode, this method isn't appropriate for a\nsafe API.\n\nModify the implementation to be safe_memory_limit_check() and to keep\ncalling error() when there is an allocation problem. The original method\ncalls that version but will die() instead when failing and not gentle.\n\nThe one potential behavior change is that when git_alloc_limit is unset we\nmust assume SIZE_MAX instead of loading the environment variable. Since we\nload this environment variable proactively in setup_environment(), this\nshould only matter for that brief window before setup_environment() and the\nsafe APIs that call this version. If such safe APIs are used in that window,\nthen they should allocate small enough amounts of memory to fit under any\nreasonable values of GIT_ALLOC_LIMIT.\n\nSigned-off-by: Derrick Stolee \n---\n wrapper.c | 27 ++++++++++++++++++---------\n 1 file changed, 18 insertions(+), 9 deletions(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 3de6b21cc2..97a29bda75 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -30,22 +30,31 @@ void initialize_git_alloc_limit(void)\n \t}\n }\n \n-static int memory_limit_check(size_t size, int gentle)\n+static int safe_memory_limit_check(size_t size, int verbose)\n {\n-\tinitialize_git_alloc_limit();\n-\n-\tif (size > git_alloc_limit) {\n-\t\tif (gentle) {\n+\tsize_t limit = git_alloc_limit ? git_alloc_limit : SIZE_MAX;\n+\tif (size > limit) {\n+\t\tif (verbose)\n \t\t\terror(\"attempting to allocate %\"PRIuMAX\" over limit %\"PRIuMAX,\n \t\t\t (uintmax_t)size, (uintmax_t)git_alloc_limit);\n-\t\t\treturn -1;\n-\t\t} else\n-\t\t\tdie(\"attempting to allocate %\"PRIuMAX\" over limit %\"PRIuMAX,\n-\t\t\t (uintmax_t)size, (uintmax_t)git_alloc_limit);\n+\t\treturn -1;\n \t}\n \treturn 0;\n }\n \n+static int memory_limit_check(size_t size, int gentle)\n+{\n+\tint res;\n+\tinitialize_git_alloc_limit();\n+\n+\tres = safe_memory_limit_check(size, gentle);\n+\tif (res && !gentle) {\n+\t\tdie(\"attempting to allocate %\"PRIuMAX\" over limit %\"PRIuMAX,\n+\t\t (uintmax_t)size, (uintmax_t)git_alloc_limit);\n+\t}\n+\treturn res;\n+}\n+\n char *xstrdup(const char *str)\n {\n \tchar *ret = strdup(str);\n-- \ngitgitgadget\n\n"},{"id":"552861","messageId":"ebd91b95209d778727dca1bfcce17dcb76b3151f.1789736540.git.gitgitgadget@gmail.com","threadId":"66346","inReplyTo":"pull.2230.git.1789736540.gitgitgadget@gmail.com","subject":"[PATCH 4/6] strbuf-safe: add sstrbuf_grow()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-18T13:02:18Z","receivedAt":"2026-09-18T13:02:30Z","isPatch":true,"body":"From: Derrick Stolee \n\nAfter a few changes in preparation, we are now ready to create our first\n'safe' strbuf API method: sstrbuf_grow(). This is a safe version of\nstrbuf_grow().\n\nOn naming: For safe equivalents of existing methods, I'm prepending a single\n's' character. The intention is to make the safe API non-intrusive.\nAlternatives could be to append '_gentle' like many other APIs that avoid a\ndie() on malformed user data, but we need to be even safer than these gentle\nmethods, which still die() on allocation failures or other system-level\nerrors. This 's' prefix is similar to the 'x' prefix used by git-compat-util\nhelpers.\n\nI selected strbuf_grow() as the first method to move because it doesn't\ndepend on any other strbuf API method, but is called by many other strbuf\nAPI calls, including strbuf_release() or strbuf_init(). Thus, this will be a\nhelper to several other implementations that are coming in upcoming changes.\n\nNo callers directly depend on sstrbuf_grow(), but the non-safe strbuf_grow()\nnow uses it as declared in strbuf-safe.h.\n\nSigned-off-by: Derrick Stolee \n---\n Makefile | 1 +\n meson.build | 1 +\n strbuf-safe.c | 34 ++++++++++++++++++++++++++++++++++\n strbuf-safe.h | 7 +++++++\n strbuf.c | 11 ++++-------\n wrapper.c | 26 +++++++++++++++++---------\n wrapper.h | 3 +++\n 7 files changed, 67 insertions(+), 16 deletions(-)\n create mode 100644 strbuf-safe.c\n\ndiff --git a/Makefile b/Makefile\nindex d4b775953d..5943853219 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1327,6 +1327,7 @@ LIB_OBJS += sparse-index.o\n LIB_OBJS += split-index.o\n LIB_OBJS += stable-qsort.o\n LIB_OBJS += statinfo.o\n+LIB_OBJS += strbuf-safe.o\n LIB_OBJS += strbuf.o\n LIB_OBJS += string-list.o\n LIB_OBJS += strmap.o\ndiff --git a/meson.build b/meson.build\nindex d86f2acd2b..368fdd00d5 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -532,6 +532,7 @@ libgit_sources = [\n 'split-index.c',\n 'stable-qsort.c',\n 'statinfo.c',\n+ 'strbuf-safe.c',\n 'strbuf.c',\n 'string-list.c',\n 'strmap.c',\ndiff --git a/strbuf-safe.c b/strbuf-safe.c\nnew file mode 100644\nindex 0000000000..e4a0707d63\n--- /dev/null\n+++ b/strbuf-safe.c\n@@ -0,0 +1,34 @@\n+#include \"git-compat-util.h\"\n+#include \"strbuf-safe.h\"\n+#include \"banned-die.h\"\n+\n+/*\n+ * A safe version of ALLOC_GROW from git-compat-util.h and\n+ * xrealloc() from wrapper.c.\n+ */\n+#define SAFE_ALLOC_GROW(x, nr, alloc) \\\n+\tdo { \\\n+\t\tif ((nr) > alloc) { \\\n+\t\t\tif (alloc_nr(alloc) < (nr)) \\\n+\t\t\t\talloc = (nr); \\\n+\t\t\telse \\\n+\t\t\t\talloc = alloc_nr(alloc); \\\n+\t\t\tif (srealloc((void **)&(x), alloc)) \\\n+\t\t\t\treturn MEMORY_ERROR; \\\n+\t\t} \\\n+\t} while (0)\n+\n+enum safe_result sstrbuf_grow(struct strbuf *sb, size_t extra)\n+{\n+\tint new_buf = !sb->alloc;\n+\tsize_t new_len = st_add3(sb->len, extra, 1);\n+\tif (new_buf)\n+\t\tsb->buf = NULL;\n+\n+\tSAFE_ALLOC_GROW(sb->buf, new_len, sb->alloc);\n+\n+\tif (new_buf)\n+\t\tsb->buf[0] = '\\0';\n+\n+\treturn SUCCESS;\n+}\ndiff --git a/strbuf-safe.h b/strbuf-safe.h\nindex 3cf14545bb..f6adf7434b 100644\n--- a/strbuf-safe.h\n+++ b/strbuf-safe.h\n@@ -85,4 +85,11 @@ struct strbuf {\n extern char strbuf_slopbuf[];\n #define STRBUF_INIT { .buf = strbuf_slopbuf }\n \n+enum safe_result {\n+\tSUCCESS = 0,\n+\tMEMORY_ERROR,\n+};\n+\n+enum safe_result sstrbuf_grow(struct strbuf *sb, size_t extra);\n+\n #endif /* STRBUF_SAFE_H */\ndiff --git a/strbuf.c b/strbuf.c\nindex 44955669e8..d005666a07 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -8,6 +8,8 @@\n #include \"utf8.h\"\n #include \"date.h\"\n \n+#define STRBUF_DIE(f) die(_(\"unexpected error during string manipulation: %s\"), f)\n+\n bool starts_with(const char *str, const char *prefix)\n {\n \tfor (; ; str++, prefix++)\n@@ -105,13 +107,8 @@ void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc)\n \n void strbuf_grow(struct strbuf *sb, size_t extra)\n {\n-\tint new_buf = !sb->alloc;\n-\tsize_t new_len = st_add3(sb->len, extra, 1);\n-\tif (new_buf)\n-\t\tsb->buf = NULL;\n-\tALLOC_GROW(sb->buf, new_len, sb->alloc);\n-\tif (new_buf)\n-\t\tsb->buf[0] = '\\0';\n+\tif (sstrbuf_grow(sb, extra))\n+\t\tSTRBUF_DIE(\"strbuf_grow\");\n }\n \n void strbuf_trim(struct strbuf *sb)\ndiff --git a/wrapper.c b/wrapper.c\nindex 97a29bda75..69ff9a8ff6 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -144,20 +144,28 @@ int xstrncmpz(const char *s, const char *t, size_t len)\n \treturn s[len] == '\\0' ? 0 : 1;\n }\n \n-void *xrealloc(void *ptr, size_t size)\n+int srealloc(void **ptr, size_t size)\n {\n-\tvoid *ret;\n-\n \tif (!size) {\n-\t\tfree(ptr);\n-\t\treturn xmalloc(0);\n+\t\tfree(*ptr);\n+\t\tif ((*ptr = malloc(1)))\n+\t\t\treturn 0;\n+\t\treturn -1;\n \t}\n \n-\tmemory_limit_check(size, 0);\n-\tret = realloc(ptr, size);\n-\tif (!ret)\n+\tif (safe_memory_limit_check(size, 0))\n+\t\treturn -1;\n+\tif ((*ptr = realloc(*ptr, size)))\n+\t\treturn 0;\n+\n+\treturn -1;\n+}\n+\n+void *xrealloc(void *ptr, size_t size)\n+{\n+\tif (srealloc(&ptr, size))\n \t\tdie(\"Out of memory, realloc failed\");\n-\treturn ret;\n+\treturn ptr;\n }\n \n void *xcalloc(size_t nmemb, size_t size)\ndiff --git a/wrapper.h b/wrapper.h\nindex 69df68ee7a..956de2c534 100644\n--- a/wrapper.h\n+++ b/wrapper.h\n@@ -27,6 +27,9 @@ char *xgetcwd(void);\n FILE *fopen_for_writing(const char *path);\n FILE *fopen_or_warn(const char *path, const char *mode);\n \n+/* safe versions of helpers above. */\n+int srealloc(void **ptr, size_t size);\n+\n /*\n * Like strncmp, but only return zero if s is NUL-terminated and exactly len\n * characters long. If it is not, consider it greater than t.\n-- \ngitgitgadget\n\n"},{"id":"552862","messageId":"6e654dcbac2ac1b6c4d5311e64ce516d0bded0fb.1789736540.git.gitgitgadget@gmail.com","threadId":"66346","inReplyTo":"pull.2230.git.1789736540.gitgitgadget@gmail.com","subject":"[PATCH 5/6] json-writer: include strbuf-safe.h","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-18T13:02:19Z","receivedAt":"2026-09-18T13:02:32Z","isPatch":true,"body":"From: Derrick Stolee \n\nWe will use json-writer.c as our first 'safe' API, after removing some\nunsafe methods that reach die(), especially the strbuf API. However, the\nfact that json-writer.h includes strbuf.h causes someheadaches here.\n\nNormally, we would only declare 'struct strbuf;' as a way to anonymously\ndefine a struct and have implementations include the full header as needed.\nHowever, 'struct json_writer' needs the full struct info and\nJSON_WRITER_INIT needs access to STRBUF_INIT.\n\nThankfully, both are included in strbuf-safe.h, so we can have the header\ninclude the safe API and move an include of strbuf.h to json-writer.c.\n\nHowever, this has some implications to the trace2 API that includes\njson-writer.h and implicitly depends on that includes of strbuf.h. Have\nthose files include strbuf.h directly to compensate.\n\nSigned-off-by: Derrick Stolee \n---\n json-writer.c | 1 +\n json-writer.h | 2 +-\n trace2/tr2_tgt_event.c | 1 +\n trace2/tr2_tgt_perf.c | 1 +\n 4 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/json-writer.c b/json-writer.c\nindex 34577dc25f..e7fc5775da 100644\n--- a/json-writer.c\n+++ b/json-writer.c\n@@ -2,6 +2,7 @@\n \n #include \"git-compat-util.h\"\n #include \"json-writer.h\"\n+#include \"strbuf.h\"\n \n void jw_init(struct json_writer *jw)\n {\ndiff --git a/json-writer.h b/json-writer.h\nindex 8f845d4d29..fa8cf02253 100644\n--- a/json-writer.h\n+++ b/json-writer.h\n@@ -70,7 +70,7 @@\n * of the given strings.\n */\n \n-#include \"strbuf.h\"\n+#include \"strbuf-safe.h\"\n \n struct json_writer\n {\ndiff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c\nindex 36a746cc10..b25fa0fb30 100644\n--- a/trace2/tr2_tgt_event.c\n+++ b/trace2/tr2_tgt_event.c\n@@ -5,6 +5,7 @@\n #include \"json-writer.h\"\n #include \"repository.h\"\n #include \"run-command.h\"\n+#include \"strbuf.h\"\n #include \"version.h\"\n #include \"trace2/tr2_dst.h\"\n #include \"trace2/tr2_tbuf.h\"\ndiff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c\nindex 96a5bc7f10..5554081c3c 100644\n--- a/trace2/tr2_tgt_perf.c\n+++ b/trace2/tr2_tgt_perf.c\n@@ -7,6 +7,7 @@\n #include \"quote.h\"\n #include \"version.h\"\n #include \"json-writer.h\"\n+#include \"strbuf.h\"\n #include \"trace2/tr2_dst.h\"\n #include \"trace2/tr2_sid.h\"\n #include \"trace2/tr2_sysenv.h\"\n-- \ngitgitgadget\n\n"},{"id":"552863","messageId":"dea925f31647e7c08f3fa467b8058351b463f593.1789736540.git.gitgitgadget@gmail.com","threadId":"66346","inReplyTo":"pull.2230.git.1789736540.gitgitgadget@gmail.com","subject":"[PATCH 6/6] strbuf-safe: add init and release methods","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-18T13:02:20Z","receivedAt":"2026-09-18T13:02:33Z","isPatch":true,"body":"From: Derrick Stolee \n\nContinue extending the strbuf-safe API by adding these safe versions of the\ninitialize and release methods:\n\n* sstrbuf_init()\n* sstrbuf_release()\n\nThese both depend on sstrbuf_grow() that was introduced in the previous\nchange.\n\nAs we are working to make json-writer.c a safe API, adapt its use of\nstrbuf_release() to the safe version. To properly handle the responses of\nthe safe versions, some methods are converted to return their own error\ncodes. However, callers of those methods are not adapted at this time and\nwill be adapted in future changes. This leaves a window where json-writer\nconsumers may continue running after an error occurs, potentially leading to\na different error in the future.\n\nSigned-off-by: Derrick Stolee \n---\n json-writer.c | 31 +++++++++++++++++++------------\n json-writer.h | 5 +++--\n strbuf-safe.c | 18 ++++++++++++++++++\n strbuf-safe.h | 2 ++\n strbuf.c | 12 ++++--------\n 5 files changed, 46 insertions(+), 22 deletions(-)\n\ndiff --git a/json-writer.c b/json-writer.c\nindex e7fc5775da..38351f3439 100644\n--- a/json-writer.c\n+++ b/json-writer.c\n@@ -3,6 +3,8 @@\n #include \"git-compat-util.h\"\n #include \"json-writer.h\"\n #include \"strbuf.h\"\n+/* banned-die must be last. */\n+#include \"banned-die.h\"\n \n void jw_init(struct json_writer *jw)\n {\n@@ -10,10 +12,15 @@ void jw_init(struct json_writer *jw)\n \tmemcpy(jw, &blank, sizeof(*jw));;\n }\n \n-void jw_release(struct json_writer *jw)\n+int jw_release(struct json_writer *jw)\n {\n-\tstrbuf_release(&jw->json);\n-\tstrbuf_release(&jw->open_stack);\n+\tenum safe_result result = SUCCESS;\n+\n+\t/* attempt both removals without short-circuiting. */\n+\tresult = sstrbuf_release(&jw->json) || result;\n+\tresult = sstrbuf_release(&jw->open_stack) || result;\n+\n+\treturn result;\n }\n \n /*\n@@ -99,16 +106,17 @@ static void maybe_add_comma(struct json_writer *jw)\n \t\tjw->need_comma = 1;\n }\n \n-static void fmt_double(struct json_writer *jw, int precision,\n-\t\t\t double value)\n+static int fmt_double(struct json_writer *jw, int precision,\n+\t\t double value)\n {\n \tif (precision < 0) {\n \t\tstrbuf_addf(&jw->json, \"%f\", value);\n+\t\treturn 0;\n \t} else {\n \t\tstruct strbuf fmt = STRBUF_INIT;\n \t\tstrbuf_addf(&fmt, \"%%.%df\", precision);\n \t\tstrbuf_addf(&jw->json, fmt.buf, value);\n-\t\tstrbuf_release(&fmt);\n+\t\treturn sstrbuf_release(&fmt);\n \t}\n }\n \n@@ -235,8 +243,8 @@ static void kill_indent(struct strbuf *sb,\n \t}\n }\n \n-static void append_sub_jw(struct json_writer *jw,\n-\t\t\t const struct json_writer *value)\n+static int append_sub_jw(struct json_writer *jw,\n+\t\t\t const struct json_writer *value)\n {\n \t/*\n \t * If both are pretty, increase the indentation of the sub_jw\n@@ -255,18 +263,17 @@ static void append_sub_jw(struct json_writer *jw,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tincrease_indent(&sb, value, jw->open_stack.len * 2);\n \t\tstrbuf_addbuf(&jw->json, &sb);\n-\t\tstrbuf_release(&sb);\n-\t\treturn;\n+\t\treturn sstrbuf_release(&sb);\n \t}\n \tif (!jw->pretty && value->pretty) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tkill_indent(&sb, value);\n \t\tstrbuf_addbuf(&jw->json, &sb);\n-\t\tstrbuf_release(&sb);\n-\t\treturn;\n+\t\treturn sstrbuf_release(&sb);\n \t}\n \n \tstrbuf_addbuf(&jw->json, &value->json);\n+\treturn 0;\n }\n \n void jw_object_sub_jw(struct json_writer *jw, const char *key,\ndiff --git a/json-writer.h b/json-writer.h\nindex fa8cf02253..72277d9839 100644\n--- a/json-writer.h\n+++ b/json-writer.h\n@@ -103,9 +103,10 @@ struct json_writer\n void jw_init(struct json_writer *jw);\n \n /*\n- * Release the internal buffers of a json_writer.\n+ * Release the internal buffers of a json_writer. Returns nonzero on\n+ * failure.\n */\n-void jw_release(struct json_writer *jw);\n+int jw_release(struct json_writer *jw);\n \n /*\n * Begin the json_writer using an object as the top-level data structure. If\ndiff --git a/strbuf-safe.c b/strbuf-safe.c\nindex e4a0707d63..7a8701e827 100644\n--- a/strbuf-safe.c\n+++ b/strbuf-safe.c\n@@ -32,3 +32,21 @@ enum safe_result sstrbuf_grow(struct strbuf *sb, size_t extra)\n \n \treturn SUCCESS;\n }\n+\n+enum safe_result sstrbuf_init(struct strbuf *sb, size_t hint)\n+{\n+\tstruct strbuf blank = STRBUF_INIT;\n+\tmemcpy(sb, &blank, sizeof(*sb));\n+\tif (!hint)\n+\t\treturn 0;\n+\treturn sstrbuf_grow(sb, hint);\n+}\n+\n+enum safe_result sstrbuf_release(struct strbuf *sb)\n+{\n+\tif (sb->alloc) {\n+\t\tfree(sb->buf);\n+\t\treturn sstrbuf_init(sb, 0);\n+\t}\n+\treturn 0;\n+}\ndiff --git a/strbuf-safe.h b/strbuf-safe.h\nindex f6adf7434b..fe04d9cf62 100644\n--- a/strbuf-safe.h\n+++ b/strbuf-safe.h\n@@ -91,5 +91,7 @@ enum safe_result {\n };\n \n enum safe_result sstrbuf_grow(struct strbuf *sb, size_t extra);\n+enum safe_result sstrbuf_init(struct strbuf *sb, size_t hint);\n+enum safe_result sstrbuf_release(struct strbuf *sb);\n \n #endif /* STRBUF_SAFE_H */\ndiff --git a/strbuf.c b/strbuf.c\nindex d005666a07..835238dc64 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -70,18 +70,14 @@ char strbuf_slopbuf[1];\n \n void strbuf_init(struct strbuf *sb, size_t hint)\n {\n-\tstruct strbuf blank = STRBUF_INIT;\n-\tmemcpy(sb, &blank, sizeof(*sb));\n-\tif (hint)\n-\t\tstrbuf_grow(sb, hint);\n+\tif (sstrbuf_init(sb, hint))\n+\t\tSTRBUF_DIE(\"strbuf_init\");\n }\n \n void strbuf_release(struct strbuf *sb)\n {\n-\tif (sb->alloc) {\n-\t\tfree(sb->buf);\n-\t\tstrbuf_init(sb, 0);\n-\t}\n+\tif (sstrbuf_release(sb))\n+\t\tSTRBUF_DIE(\"strbuf_release\");\n }\n \n char *strbuf_detach(struct strbuf *sb, size_t *sz)\n-- \ngitgitgadget\n"},{"id":"552885","messageId":"9adb1d94-c72b-43f2-aa02-004e3e476eb1@gmail.com","threadId":"66346","inReplyTo":"pull.2230.git.1789736540.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/6] [RFC] Create a 'safe' strbuf API","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-19T15:23:41Z","receivedAt":"2026-09-19T15:23:45Z","isPatch":true,"body":"Hi Stolee\n\nOn 18/09/2026 14:02, Derrick Stolee via GitGitGadget wrote:\n> \n> The goal of this short RFC, such as it is, is to get some feedback on\n> whether this is a worthwhile direction to pursue or if I should abandon this\n> idea of having this definition of \"safe\" for some APIs. This decision may\n> also determine if we should abandon ds/trace2-tolerate-failed-timestamps or\n> leave the existing behavior as-is.\n\nI think having APIs that return errors rather than dying on allocation \nfailures or overflow is reasonable. The xdiff and reftable code already \nhave something similar. I'm not sure \"safe\" is a good description for \nthose APIs though as it does not describe how they differ from the \nexisting APIs. Instead of talking about safety I'd rather the \ndocumentation talked about returning errors on failure and the function \nnaming somehow reflected that.\n\nFor the strbuf API having to check for failure on every function call \ndoes not sound attractive, I think having a sticky error bit like the \nstdio functions so that one can build a string and check there have been \nno failures once just before using it would be a nicer approach.\n\n> I had discussed earlier that what we'd really need is a guarantee that we\n> can't transitively reach die() from any \"safe\" API. The eventual goal would\n> be to include json-writer.c and the trace2 code files into the \"safe\"\n> bucket, but for now I'm making sure that strbuf-safe.c satisfies this CodeQL\n> query:\n\nI don't know enough about CodeQL to comment on this beyond noting that \nthe implementation of sstrbuf_grow() in patch 4 contains a call to \nst_add3() which dies on overflow (it should be using st_add_overflows() \ninstead) so something isn't right with these checks.\n\nThanks\n\nPhillip\n\n> import cpp\n> \n> class SafeFunction extends Function {\n> SafeFunction() {\n> getFile().getRelativePath() = \"strbuf-safe.c\"\n> }\n> }\n> \n> predicate directlyCalls(Function caller, Function callee) {\n> exists(FunctionCall call |\n> call.getEnclosingFunction() = caller and\n> call.getTarget() = callee\n> )\n> }\n> \n> \n> from SafeFunction source, Function sink\n> where\n> (sink.getName() = \"die\" or sink.getName() = \"exit\") and\n> directlyCalls+(source, sink)\n> select source,\n> \"This safe function can transitively reach \" + sink.getName() + \"().\"\n> \n> \n> If we went with this approach, then I'd explore how to make this a\n> build-time requirement during CI.\n> \n> In regards to the structure of this RFC:\n> \n> 1. The safe API needs the same structures, but shouldn't import more than\n> necessary. Some movement of structs across headers is done before\n> anything else.\n> 2. In order to make even the smallest safe method work, we first need to\n> figure out how to handle GIT_ALLOC_LIMIT, which is an undocumented\n> environment variable. I explain that I think this should be\n> GIT_TEST_ALLOC_LIMIT, but maybe the ship has sailed due to Hyrum's Law.\n> So I make an effort to document it but also to initialize it proactively\n> within the process startup instead of implicitly at the lowest level.\n> This allows us to avoid a die() when checking the environment variable.\n> 3. Thus, we get a 'safe' version of a memory allocation size check. This is\n> our first example of creating a safe version that is then called by the\n> non-safe version to prevent repeated code.\n> 4. We can then create our first safe strbuf method: sstrbuf_grow(). I\n> explain why I prepend with s instead of appending _gently in the commit.\n> 5. Some trace2 code implicitly depends on strbuf.h through json-writer.h,\n> so we drop that in favor of strbuf-safe.h to keep the dependence on the\n> full struct definition without forever having the non-safe methods\n> reachable. The goal eventually is to drop the strbuf.h include from\n> json-writer.c, but that isn't accomplished in this RFC.\n> 6. Finally, create safe init and release methods and use them in\n> json-writer.c. This does show some of the \"transition risk\" where some\n> json-writer methods become \"safe\" but I haven't done the hard work to\n> make sure the callers of those methods respond to the new return values.\n> If we proceed with the RFC, then I'd split this into a creation of the\n> safe strbuf methods and then the refactoring required to respond\n> correctly to errors in json-writer.c\n> \n> Thanks in advance for your thoughts!\n> \n> Thanks, -Stolee\n> \n> Derrick Stolee (6):\n> strbuf: add header for 'safe' API\n> wrapper: initialize GIT_ALLOC_LIMIT proactively\n> wrapper: create safe_memory_limit_check()\n> strbuf-safe: add sstrbuf_grow()\n> json-writer: include strbuf-safe.h\n> strbuf-safe: add init and release methods\n> \n> Documentation/git.adoc | 6 +++\n> Makefile | 1 +\n> common-init.c | 2 +\n> environment.h | 1 +\n> json-writer.c | 32 ++++++++------\n> json-writer.h | 7 +--\n> meson.build | 1 +\n> strbuf-safe.c | 52 ++++++++++++++++++++++\n> strbuf-safe.h | 97 ++++++++++++++++++++++++++++++++++++++++++\n> strbuf.c | 23 ++++------\n> strbuf.h | 74 ++------------------------------\n> trace2/tr2_tgt_event.c | 1 +\n> trace2/tr2_tgt_perf.c | 1 +\n> wrapper.c | 67 ++++++++++++++++++++---------\n> wrapper.h | 9 ++++\n> 15 files changed, 253 insertions(+), 121 deletions(-)\n> create mode 100644 strbuf-safe.c\n> create mode 100644 strbuf-safe.h\n> \n> \n> base-commit: a80c36bda0e5aff1c9945d08f43079a6aa85ccad\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2230%2Fderrickstolee%2Fstrbuf-safe-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2230/derrickstolee/strbuf-safe-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/2230\n\n"},{"id":"552949","messageId":"xmqq4ifimhfr.fsf@gitster.g","threadId":"66346","inReplyTo":"b1779709120adc9c1df40c7210481d6bed9791c5.1789736540.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/6] strbuf: add header for 'safe' API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-21T21:18:48Z","receivedAt":"2026-09-21T21:18:50Z","isPatch":true,"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> +/*\n> + * NOTE FOR STRBUF DEVELOPERS\n> + *\n> + * strbuf is a low-level primitive; as such it should interact only\n> + * with other low-level primitives. Do not introduce new functions\n> + * which interact with higher-level APIs.\n> + *\n> + * This header file specifically conatins the \"safe\" API surface for\n> + * working with strbufs. The implementations of these methods avoid\n> + * using die() and other exits. Thus, these methods are appropriate\n> + * for use within lower-level APIs such as trace2.\n> + */\n\nI have to wonder if this is somewhat backwards, in that the longer\nterm goal for us should be to make most of the service routines like\nstrbuf, string_list, csum_file, etc., free of die() and be \"safe\".\n\nA recent trend under the label \"libification\" is to make the use of\nthe_repository more explicit and pass a \"struct repository *\" as a\nparameter instead more widely throughout the code flow, but it would\nbe equally if not more useful change to expand the \"safe\" API surface\nso that callers of more service routines take responsibility to act\non errors.\n\nAnd picking strbuf as the first instance of such generic service\nlibrary certainly is a good idea. Its interface is well defined.\n\nWe may want to rename functions that _happen_ to use a strbuf to\nreturn their results but otherwise has nothing to do with strbuf\naway from strbuf_ prefix (strbuf_realpath() etc. in abspath.h are\nprime examples) as part of this first step, though.\n"},{"id":"552950","messageId":"xmqqzexal2lv.fsf@gitster.g","threadId":"66346","inReplyTo":"3b3c67243d200a42aa105981b64228e2cbb35a6c.1789736540.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/6] wrapper: create safe_memory_limit_check()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-21T21:24:28Z","receivedAt":"2026-09-21T21:24:30Z","isPatch":true,"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> +static int safe_memory_limit_check(size_t size, int verbose)\n> {\n> +\tsize_t limit = git_alloc_limit ? git_alloc_limit : SIZE_MAX;\n> +\tif (size > limit) {\n> +\t\tif (verbose)\n> \t\t\terror(\"attempting to allocate %\"PRIuMAX\" over limit %\"PRIuMAX,\n> \t\t\t (uintmax_t)size, (uintmax_t)git_alloc_limit);\n> +\t\treturn -1;\n> \t}\n> \treturn 0;\n> }\n\nThe code is prepared for a case where git_alloc_limit is set to 0,\nin which case SIZE_MAX is used as a stand-in value. When the check\ndetects a request with overly large 'size', the error message tells\nus that 'size' is over 'git_alloc_limit', the latter is zero and any\nconcrete value of 'size' certainly would be over that. Which may be a\nbit confusing.\n\nShouldn't we be giving the local \"limit\" instead in the message?\n"},{"id":"552951","messageId":"xmqqv77yl2cq.fsf@gitster.g","threadId":"66346","inReplyTo":"ebd91b95209d778727dca1bfcce17dcb76b3151f.1789736540.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/6] strbuf-safe: add sstrbuf_grow()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-21T21:29:57Z","receivedAt":"2026-09-21T21:29:59Z","isPatch":true,"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> +int srealloc(void **ptr, size_t size)\n> {\n> \tif (!size) {\n> +\t\tfree(*ptr);\n> +\t\tif ((*ptr = malloc(1)))\n> +\t\t\treturn 0;\n> +\t\treturn -1;\n> \t}\n> \n> +\tif (safe_memory_limit_check(size, 0))\n> +\t\treturn -1;\n> +\tif ((*ptr = realloc(*ptr, size)))\n> +\t\treturn 0;\n> +\n> +\treturn -1;\n> +}\n\nThis overrites *ptr with whatever realloc() returns, and then checks\nif we had an error, thereby losing whatever pointer *ptr originally\nhad. When realloc() does fail, we have already clobbered *ptr, and\nvery likely have robbed our caller the pointer it had to the region\nof memory. Aren't we leaking that piece of memory as the result?\n"},{"id":"552952","messageId":"xmqqld8ul1ny.fsf@gitster.g","threadId":"66346","inReplyTo":"dea925f31647e7c08f3fa467b8058351b463f593.1789736540.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 6/6] strbuf-safe: add init and release methods","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-21T21:44:49Z","receivedAt":"2026-09-21T21:44:51Z","isPatch":true,"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> +int jw_release(struct json_writer *jw)\n> {\n> -\tstrbuf_release(&jw->json);\n> -\tstrbuf_release(&jw->open_stack);\n> +\tenum safe_result result = SUCCESS;\n> +\n> +\t/* attempt both removals without short-circuiting. */\n> +\tresult = sstrbuf_release(&jw->json) || result;\n> +\tresult = sstrbuf_release(&jw->open_stack) || result;\n> +\n> +\treturn result;\n> }\n\nThis is puzzling in a few ways.\n\n\"enum safe_result\" so far has been SUCCESS==0 and MEMORY_ERROR==1.\nPresumably in some future we would gain other kind of error symbols,\nbut when that happens is this meant to act as an enumeration of\ndifferent kinds errors? Or an enumeration of bitmasks that can\nsignal different kinds of errors?\n\nIf we mean \"enum safe_result\" is an enumeration of different kinds\nof errors, then the \"result\" variable and the returned value from\nhere would be able to report a *single* kind of error, and it may\nbe common to report the first error we encounter, in which case\n\n enum safe_result result = SUCCESS;\n enum safe_result res;\n\n res = sstrbuf_release(&jw->json);\n if (!result && res)\n\tresult = res;\n res = sstrbuf_release(&jw->open_stack);\n if (!result && res)\n\tresult = res;\n return result;\n\nwould be slightly longer, far easier to reason about, and is a lot\nmore futureproof. What you wrote, with \"||\", does not really allow\nanything other than \"is it still zero, or coalesce any non-zero\nvalue to 1\".\n\nOn the other hand, if we mean \"enum safe_result\" is an enumeration\nof bitmasks, each bit representing different kind of error, then\n\n enum safe_result result = 0;\n\n result |= sstrbuf_release(&jw->json);\n result |= sstrbuf_release(&jw->open_stack);\n return result;\n\nwould probably be what you want. That way you can add different\nfunctions that returns different bit to signal a different kind of\nerror and or it in.\n\n result |= some_function();\n"},{"id":"552957","messageId":"xmqq1pamkzcs.fsf@gitster.g","threadId":"66346","inReplyTo":"xmqqld8ul1ny.fsf@gitster.g","subject":"Re: [PATCH 6/6] strbuf-safe: add init and release methods","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-21T22:34:43Z","receivedAt":"2026-09-21T22:34:46Z","isPatch":true,"body":"Junio C Hamano writes:\n\n> \"Derrick Stolee via GitGitGadget\" writes:\n>\n>> +int jw_release(struct json_writer *jw)\n>> {\n>> -\tstrbuf_release(&jw->json);\n>> -\tstrbuf_release(&jw->open_stack);\n>> +\tenum safe_result result = SUCCESS;\n>> +\n>> +\t/* attempt both removals without short-circuiting. */\n>> +\tresult = sstrbuf_release(&jw->json) || result;\n>> +\tresult = sstrbuf_release(&jw->open_stack) || result;\n>> +\n>> +\treturn result;\n>> }\n>\n> This is puzzling in a few ways.\n> If we mean \"enum safe_result\" is an enumeration of different kinds\n> of errors, then the \"result\" variable and the returned value from\n> ...\n> On the other hand, if we mean \"enum safe_result\" is an enumeration\n> of bitmasks, each bit representing different kind of error, then\n> ...\n\nI forgot the third possibility. Regardless of which interpretation\nof \"enum safe_result\" we use, if jw_release() is designed to say \"0\nfor success, non-zero for failure\", then almost as written but\ndeclaring \"result\" as a plain \"int\"\n\n int result = 0;\n\n result = sstrbuf_release(&jw->json) || result;\n result = sstrbuf_release(&jw->open_stack) || result;\n\n return result;\n\nwould probably make sense, even though the \"|| result\" construct is\na bit unusual in C.\n\nThanks.\n\n"},{"id":"553102","messageId":"DLMXXKPGU78J.2PBDFYJTPFTA4@fastmail.com","threadId":"66346","inReplyTo":"b1779709120adc9c1df40c7210481d6bed9791c5.1789736540.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/6] strbuf: add header for 'safe' API","fromName":"Mark C. Chu-Carroll","fromEmail":"markchucarroll@fastmail.com","sentAt":"2026-09-23T19:25:23Z","receivedAt":"2026-09-23T19:26:43Z","isPatch":true,"body":"General comment: I really like the idea of this. While I haven't\nencountered this specific issue with git, I've dealt with similar issues\nin other systems, and even if the cascading error case is rare, it's\nincredibly frustrating to deal with the loss of error details because\nthey used unsafe operations to generate their messages!\n\nI'm not really qualified to comment much on the code yet, but there's a \ncouple of small writing style things that I'll nitpick for\nclarity/readibilty. Feel free to ignore these if you disagree.\n\nOn Fri Sep 18, 2026 at 9:02 AM EDT, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n>\n> The strbuf library is an important API used all over the Git codebase.\n> Contributors use it in nearly any string-manipulating action. However, the\n> implementation uses other helping functions that die() on failure instead of\n> returning an error code. Thus, the strbuf API isn't _safe_.\n>\n> In particular, we cannot include 'banned-die.h' in 'strbuf.c'.\n\nI think we prefer to avoid \"we\" in these comments; and \nthe \"in particular\" here feels a little abrupt - maybe \"In order to\nensure that strbuf functions can't call die, strbuf.c should not include ...\"\n\n> To start the creation of a safe strbuf API, move the struct definition into\n> a new 'strbuf-safe.h' header file. All consumers of 'strbuf.h' will consume\n> that header transitively.\n>\n> In the future, we will hope to have consumers that need a 'safe' API will\n> include 'strbuf-safe.h' instead of 'strbuf.h'.\n\nAgain, avoiding we; and I don't think the quotes belong there. \n\nMaybe \"In the future, consumers that need a safe API will include ...\"\n\n\n> We will see in future changes the inclusion of new implementations that\n> return an error code instead of halting.\n\nThe structure of this sentence is confusing. I had to read it a couple\nof times to figure out how to parse it. Better something like: \n\"Future changes will include new implementations that return an error\ncode instead of halting.\"\n\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> strbuf-safe.h | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++\n> strbuf.h | 74 +++----------------------------------------\n> 2 files changed, 92 insertions(+), 70 deletions(-)\n> create mode 100644 strbuf-safe.h\n>\n> diff --git a/strbuf-safe.h b/strbuf-safe.h\n> new file mode 100644\n> index 0000000000..3cf14545bb\n> --- /dev/null\n> +++ b/strbuf-safe.h\n> @@ -0,0 +1,88 @@\n> +#ifndef STRBUF_SAFE_H\n> +#define STRBUF_SAFE_H\n> +\n> +/*\n> + * NOTE FOR STRBUF DEVELOPERS\n> + *\n> + * strbuf is a low-level primitive; as such it should interact only\n> + * with other low-level primitives. Do not introduce new functions\n> + * which interact with higher-level APIs.\n> + *\n> + * This header file specifically conatins the \"safe\" API surface for\n> + * working with strbufs. The implementations of these methods avoid\n> + * using die() and other exits. Thus, these methods are appropriate\n> + * for use within lower-level APIs such as trace2.\n> + */ \n\nAs with the prose comments above, safe shouldn't be in quotes.\n\n> +\n> +struct string_list;\n> +\n> +/**\n> + * strbufs are meant to be used with all the usual C string and memory\n> + * APIs. Given that the length of the buffer is known, it's often better to\n> + * use the mem* functions than a str* one (e.g., memchr vs. strchr).\n> + * Though, one has to be careful about the fact that str* functions often\n> + * stop on NULs and that strbufs may have embedded NULs.\n> + *\n> + * A strbuf is NUL terminated for convenience, but no function in the\n> + * strbuf API actually relies on the string being free of NULs.\n> + *\n> + * strbufs have some invariants that are very important to keep in mind:\n\nI think this should be stronger - they shouldn't just be kept in mind, they \nshould be strictly enforced: \"strbufs have some invariants that _must_\nbe maintained\".\n\n> + *\n> + * - The `buf` member is never NULL, so it can be used in any usual C\n> + * string operations safely. strbufs _have_ to be initialized either by\n> + * `strbuf_init()` or by `= STRBUF_INIT` before the invariants, though.\n\nI think \"must\" is better that \"have to\" here; I know some non-native\nenglish speakers get confused by that.\n\n> + *\n> + * Do *not* assume anything on what `buf` really is (e.g. if it is\n> + * allocated memory or not), use `strbuf_detach()` to unwrap a memory\n> + * buffer from its strbuf shell in a safe way. That is the sole supported\n> + * way. This will give you a malloced buffer that you can later `free()`.\n> + *\n> + * However, it is totally safe to modify anything in the string pointed by\n> + * the `buf` member, between the indices `0` and `len-1` (inclusive).\n> + *\n> + * - The `buf` member is a byte array that has at least `len + 1` bytes\n> + * allocated. The extra byte is used to store a `'\\0'`, allowing the\n> + * `buf` member to be a valid C-string. All strbuf functions ensure this\n> + * invariant is preserved.\n\nI think there should be a \"must\" before ensure\".\n\n> + *\n> + * NOTE: It is OK to \"play\" with the buffer directly if you work it this\n> + * way:\n\nI don't think \"play\" is good, and in any case it shouldn't be in quotes. \nMaybe \"It is OK to manipulate the buffer directly...\"\n\n> + *\n> + * strbuf_grow(sb, SOME_SIZE); <1>\n> + * strbuf_setlen(sb, sb->len + SOME_OTHER_SIZE);\n> + *\n> + * <1> Here, the memory array starting at `sb->buf`, and of length\n> + * `strbuf_avail(sb)` is all yours, and you can be sure that\n> + * `strbuf_avail(sb)` is at least `SOME_SIZE`.\n> + *\n> + * NOTE: `SOME_OTHER_SIZE` must be smaller or equal to `strbuf_avail(sb)`.\n> + *\n> + * Doing so is safe, though if it has to be done in many places, adding the\n> + * missing API to the strbuf module is the way to go.\n> + *\n> + * WARNING: Do _not_ assume that the area that is yours is of size `alloc\n> + * - 1` even if it's true in the current implementation. Alloc is somehow a\n> + * \"private\" member that should not be messed with. Use `strbuf_avail()`\n> + * instead.\n\nAgain, the quote around private. (I had an undergrad advisor who was a\nstickler about the right way to use quotes, and he pounded into me so that\nnow I die a little bit every time I see them used as emphasis or as a marker \nof \"not really\".)\n\n> +*/\n> +\n> +/**\n> + * Data Structures\n> + * ---------------\n> + */\n> +\n> +/**\n> + * This is the string buffer structure. The `len` member can be used to\n> + * determine the current length of the string, and `buf` member provides\n> + * access to the string itself.\n> + */\n> +struct strbuf {\n> +\tsize_t alloc;\n> +\tsize_t len;\n> +\tchar *buf;\n> +};\n> +\n> +extern char strbuf_slopbuf[];\n> +#define STRBUF_INIT { .buf = strbuf_slopbuf }\n> +\n> +#endif /* STRBUF_SAFE_H */ \n\nRepeat comments above for the repetitions in the other file.\n\n\n-- \nMark Craig Chu-Carroll (@MarkChuCarroll at gitlab)\n*** Software Tools/Math Geek - Software Engineer at Gitlab\n*** Work Email: mcarroll@gitlab.com / markchucarroll@fastmail.com\n*** Personal Blog: http://goodmath.org/blog / Personal email: markcc@gmail.com\n\n"},{"id":"553104","messageId":"20260923194628.GA44327@coredump.intra.peff.net","threadId":"66346","inReplyTo":"9adb1d94-c72b-43f2-aa02-004e3e476eb1@gmail.com","subject":"Re: [PATCH 0/6] [RFC] Create a 'safe' strbuf API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-23T19:46:28Z","receivedAt":"2026-09-23T19:46:32Z","isPatch":true,"body":"On Sat, Sep 19, 2026 at 04:23:41PM +0100, Phillip Wood wrote:\n\n> > The goal of this short RFC, such as it is, is to get some feedback on\n> > whether this is a worthwhile direction to pursue or if I should abandon this\n> > idea of having this definition of \"safe\" for some APIs. This decision may\n> > also determine if we should abandon ds/trace2-tolerate-failed-timestamps or\n> > leave the existing behavior as-is.\n> \n> I think having APIs that return errors rather than dying on allocation\n> failures or overflow is reasonable. The xdiff and reftable code already have\n> something similar. I'm not sure \"safe\" is a good description for those APIs\n> though as it does not describe how they differ from the existing APIs.\n> Instead of talking about safety I'd rather the documentation talked about\n> returning errors on failure and the function naming somehow reflected that.\n\nYeah, I don't love the term \"safe\" here for two reasons:\n\n 1. It implies the normal strbuf functions aren't safe. The flaw here\n is the \"malloc failure is fatal\" scheme, but that's just fine and\n shared with most of the rest of our code. I think we usually\n reserve safe/unsafe for things that you should be extra careful of\n using (like refs_resolve_ref_unsafe, or the unsafe hash algos).\n\n 2. There are many types of safety, and this is just implementing one\n of them. ;) In particular, for the use case in the trace code we're\n looking at, I'd expect that we would want to avoid calling malloc()\n at all, because it relies on locks. So this new code would not be\n safe to call from a signal handler, for example.\n\nSo what I had imagined when seeing the initial subject lines was not\nstrbufs who report malloc errors, but rather strbuf-like functions that\noperate on a fixed-size buffer (with either a static max-size like 4k,\nor perhaps a per-variable max-size recorded in the struct).\n\nYou can still run into errors, of course; we might run out of room in\nthe buffer. But we'd see those cases deterministically for a given\ninput, rather than occasionally when races or other external factors\ncause malloc to unexpectedly fail.\n\n> For the strbuf API having to check for failure on every function call does\n> not sound attractive, I think having a sticky error bit like the stdio\n> functions so that one can build a string and check there have been no\n> failures once just before using it would be a nicer approach.\n\nAgreed. I think that is a good approach for the static_strbuf idea\nabove, too.\n\n-Peff\n"},{"id":"553107","messageId":"xmqqzex7913p.fsf@gitster.g","threadId":"66346","inReplyTo":"DLMXXKPGU78J.2PBDFYJTPFTA4@fastmail.com","subject":"Re: [PATCH 1/6] strbuf: add header for 'safe' API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-23T20:14:34Z","receivedAt":"2026-09-23T20:14:41Z","isPatch":true,"body":"\"Mark C. Chu-Carroll\" writes:\n\n> General comment: I really like the idea of this. While I haven't\n> encountered this specific issue with git, I've dealt with similar issues\n> in other systems, and even if the cascading error case is rare, it's\n> incredibly frustrating to deal with the loss of error details because\n> they used unsafe operations to generate their messages!\n\nIf I understand correctly what this topic aims at, you'll see the\n\"loss of error details\" either way. Either we ran out of memory\ninside strbuf call and die, or we fail to allocate memory to format\nthe details and end up not showing it.\n\n> On Fri Sep 18, 2026 at 9:02 AM EDT, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee \n>>\n>> In particular, we cannot include 'banned-die.h' in 'strbuf.c'.\n>\n> I think we prefer to avoid \"we\" in these comments; and \n\nThe third word of your comment should not be \"we\" but \"I\", if that\n\"we\" intends to include me and others who wrote many commit log\nmessages ;-)\n"}]}