From b1779709120adc9c1df40c7210481d6bed9791c5 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 16 Sep 2026 18:57:54 -0400 Subject: [PATCH 1/6] strbuf: add header for 'safe' API 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. Signed-off-by: Derrick Stolee --- strbuf-safe.h | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++ strbuf.h | 74 +++---------------------------------------- 2 files changed, 92 insertions(+), 70 deletions(-) create mode 100644 strbuf-safe.h diff --git a/strbuf-safe.h b/strbuf-safe.h new file mode 100644 index 00000000000000..3cf14545bb7048 --- /dev/null +++ b/strbuf-safe.h @@ -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 */ diff --git a/strbuf.h b/strbuf.h index 1089ae687bda95..b41f8ef901ee58 100644 --- a/strbuf.h +++ b/strbuf.h @@ -1,85 +1,19 @@ #ifndef STRBUF_H #define STRBUF_H +#include "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. - */ - -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 - * --------------- + * Also see strbuf-safe.h for the struct definitions and safe versions + * of some methods declared in this header file. */ -/** - * 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 } - struct object_id; /** From 8d30730feac37d2cd42c969dca3f33c007c2065e Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 16 Sep 2026 19:39:39 -0400 Subject: [PATCH 2/6] wrapper: initialize GIT_ALLOC_LIMIT proactively 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. Signed-off-by: Derrick Stolee --- Documentation/git.adoc | 6 ++++++ common-init.c | 2 ++ environment.h | 1 + wrapper.c | 26 +++++++++++++++++--------- wrapper.h | 6 ++++++ 5 files changed, 32 insertions(+), 9 deletions(-) diff --git a/Documentation/git.adoc b/Documentation/git.adoc index 8a5cdd3b3d22c5..07da5c4f124cea 100644 --- a/Documentation/git.adoc +++ b/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. diff --git a/common-init.c b/common-init.c index d26c9c1f20239e..bf73c754b4ff43 100644 --- a/common-init.c +++ b/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); diff --git a/environment.h b/environment.h index e7ec5b0437342d..86b67da877f2b1 100644 --- a/environment.h +++ b/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" diff --git a/wrapper.c b/wrapper.c index 561f9ee9c99fc1..3de6b21cc2106d 100644 --- a/wrapper.c +++ b/wrapper.c @@ -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; } diff --git a/wrapper.h b/wrapper.h index a6287d7f4d11be..69df68ee7aeef9 100644 --- a/wrapper.h +++ b/wrapper.h @@ -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 */ From 3b3c67243d200a42aa105981b64228e2cbb35a6c Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 16 Sep 2026 19:44:51 -0400 Subject: [PATCH 3/6] wrapper: create safe_memory_limit_check() 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. Signed-off-by: Derrick Stolee --- wrapper.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/wrapper.c b/wrapper.c index 3de6b21cc2106d..97a29bda75fb38 100644 --- a/wrapper.c +++ b/wrapper.c @@ -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); From ebd91b95209d778727dca1bfcce17dcb76b3151f Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 16 Sep 2026 19:30:43 -0400 Subject: [PATCH 4/6] strbuf-safe: add sstrbuf_grow() 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. Signed-off-by: Derrick Stolee --- 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 diff --git a/Makefile b/Makefile index d4b775953d3842..594385321986a6 100644 --- a/Makefile +++ b/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 diff --git a/meson.build b/meson.build index d86f2acd2b2a46..368fdd00d5ee82 100644 --- a/meson.build +++ b/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', diff --git a/strbuf-safe.c b/strbuf-safe.c new file mode 100644 index 00000000000000..e4a0707d636c75 --- /dev/null +++ b/strbuf-safe.c @@ -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; +} diff --git a/strbuf-safe.h b/strbuf-safe.h index 3cf14545bb7048..f6adf7434bc400 100644 --- a/strbuf-safe.h +++ b/strbuf-safe.h @@ -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 */ diff --git a/strbuf.c b/strbuf.c index 44955669e8c504..d005666a077ae1 100644 --- a/strbuf.c +++ b/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++) @@ -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) diff --git a/wrapper.c b/wrapper.c index 97a29bda75fb38..69ff9a8ff6568c 100644 --- a/wrapper.c +++ b/wrapper.c @@ -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) diff --git a/wrapper.h b/wrapper.h index 69df68ee7aeef9..956de2c534c732 100644 --- a/wrapper.h +++ b/wrapper.h @@ -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. From 6e654dcbac2ac1b6c4d5311e64ce516d0bded0fb Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 16 Sep 2026 20:41:51 -0400 Subject: [PATCH 5/6] json-writer: include strbuf-safe.h 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. Signed-off-by: Derrick Stolee --- 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(-) diff --git a/json-writer.c b/json-writer.c index 34577dc25f887c..e7fc5775da18be 100644 --- a/json-writer.c +++ b/json-writer.c @@ -2,6 +2,7 @@ #include "git-compat-util.h" #include "json-writer.h" +#include "strbuf.h" void jw_init(struct json_writer *jw) { diff --git a/json-writer.h b/json-writer.h index 8f845d4d294d8d..fa8cf022533916 100644 --- a/json-writer.h +++ b/json-writer.h @@ -70,7 +70,7 @@ * of the given strings. */ -#include "strbuf.h" +#include "strbuf-safe.h" struct json_writer { diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c index 36a746cc108e09..b25fa0fb307dbd 100644 --- a/trace2/tr2_tgt_event.c +++ b/trace2/tr2_tgt_event.c @@ -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" diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c index 96a5bc7f10a097..5554081c3ccb58 100644 --- a/trace2/tr2_tgt_perf.c +++ b/trace2/tr2_tgt_perf.c @@ -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" From dea925f31647e7c08f3fa467b8058351b463f593 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 16 Sep 2026 20:51:20 -0400 Subject: [PATCH 6/6] strbuf-safe: add init and release methods 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. Signed-off-by: Derrick Stolee --- 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(-) diff --git a/json-writer.c b/json-writer.c index e7fc5775da18be..38351f3439bc2a 100644 --- a/json-writer.c +++ b/json-writer.c @@ -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, diff --git a/json-writer.h b/json-writer.h index fa8cf022533916..72277d983931ad 100644 --- a/json-writer.h +++ b/json-writer.h @@ -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 diff --git a/strbuf-safe.c b/strbuf-safe.c index e4a0707d636c75..7a8701e827b7ae 100644 --- a/strbuf-safe.c +++ b/strbuf-safe.c @@ -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; +} diff --git a/strbuf-safe.h b/strbuf-safe.h index f6adf7434bc400..fe04d9cf62ae1c 100644 --- a/strbuf-safe.h +++ b/strbuf-safe.h @@ -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 */ diff --git a/strbuf.c b/strbuf.c index d005666a077ae1..835238dc645846 100644 --- a/strbuf.c +++ b/strbuf.c @@ -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)