fix(discover): make cbm_gitignore_merge atomic on allocation failure

cbm_gitignore_merge copied src patterns into dst one strdup at a time and
returned void on failure, leaving dst with a partial set of patterns and no
signal to the caller. If the dropped pattern was the critical exclude (the
.git/info/exclude entry that prevents an OOM walk), the bug could silently
recur with some patterns applied and others dropped.

Make the merge atomic: on any allocation failure roll back the partial copies
so dst is left exactly as it was, and return bool so callers can tell. The
.gitignore caller now degrades gracefully (exclude patterns skipped, as if the
file were absent) instead of ending up with a half-merged matcher.

Adds a reproduce-first test that injects a strdup failure mid-merge via a test
seam and asserts dst is unchanged; it is red without the rollback.

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
This commit is contained in:
Martin Vogel committed 2026-06-22 23:51:49 +02:00
1 parent acb7eaaa05
commit 5ea5579d2b
4 files changed
+78 -9

No files matched your search

+4 -1
View File
@@ -563,7 +563,10 @@ int cbm_discover_ex(const char *repo_path, const cbm_discover_opts_t *opts, cbm_
if (!gitignore) {
gitignore = git_exclude;
} else {
cbm_gitignore_merge(gitignore, git_exclude);
/* On allocation failure the merge is atomic (dst unchanged), so
* the .gitignore patterns still apply; the exclude patterns are
* simply skipped — same as if .git/info/exclude were absent. */
(void)cbm_gitignore_merge(gitignore, git_exclude);
cbm_gitignore_free(git_exclude);
}
}
+5 -2
View File
@@ -60,8 +60,11 @@ void cbm_gitignore_free(cbm_gitignore_t *gi);
/* Append all patterns from src into dst. dst takes ownership of deep copies
* of each src pattern; src is unchanged and must still be freed by the caller.
* NULL-safe on either argument. */
void cbm_gitignore_merge(cbm_gitignore_t *dst, const cbm_gitignore_t *src);
* NULL-safe on either argument.
* Returns true on success (or when there is nothing to merge). Returns false on
* allocation failure, in which case dst is left exactly as it was (atomic) — no
* partial merge — so a failed merge degrades to "as if src was absent". */
bool cbm_gitignore_merge(cbm_gitignore_t *dst, const cbm_gitignore_t *src);
/* ── Directory skip / suffix filters ─────────────────────────────── */
+24 -6
View File
@@ -380,23 +380,40 @@ void cbm_gitignore_free(cbm_gitignore_t *gi) {
free(gi);
}
void cbm_gitignore_merge(cbm_gitignore_t *dst, const cbm_gitignore_t *src) {
if (!dst || !src || src->count == 0) {
return;
/* Test seam: lets a unit test simulate strdup() failure mid-merge so the
* atomic-rollback path can be exercised without real OOM. NULL = use strdup. */
char *(*cbm_gitignore_merge_dup_hook_for_test)(const char *) = NULL;
bool cbm_gitignore_merge(cbm_gitignore_t *dst, const cbm_gitignore_t *src) {
if (!dst) {
return false;
}
if (!src || src->count == 0) {
return true; /* nothing to merge */
}
int needed = dst->count + src->count;
if (needed > dst->capacity) {
gi_pattern_t *grown = realloc(dst->patterns, (size_t)needed * sizeof(gi_pattern_t));
if (!grown) {
return;
return false; /* dst left unchanged */
}
dst->patterns = grown;
dst->capacity = needed;
}
int start_count = dst->count;
for (int i = 0; i < src->count; i++) {
char *pat = strdup(src->patterns[i].pattern);
char *pat = cbm_gitignore_merge_dup_hook_for_test
? cbm_gitignore_merge_dup_hook_for_test(src->patterns[i].pattern)
: strdup(src->patterns[i].pattern);
if (!pat) {
return;
/* Roll back partial copies so dst is unchanged on failure (atomic
* merge). A silent partial merge could drop the very exclude
* pattern the caller relied on while keeping others. */
for (int j = start_count; j < dst->count; j++) {
free(dst->patterns[j].pattern);
}
dst->count = start_count;
return false;
}
dst->patterns[dst->count].pattern = pat;
dst->patterns[dst->count].negated = src->patterns[i].negated;
@@ -404,4 +421,5 @@ void cbm_gitignore_merge(cbm_gitignore_t *dst, const cbm_gitignore_t *src) {
dst->patterns[dst->count].rooted = src->patterns[i].rooted;
dst->count++;
}
return true;
}
+45
View File
@@ -6,6 +6,7 @@
#include "../src/foundation/compat.h"
#include "test_framework.h"
#include "discover/discover.h"
#include <string.h> /* strdup (test seam) */
/* ── Basic pattern matching ────────────────────────────────────── */
@@ -230,6 +231,49 @@ TEST(gi_merge_null_safe) {
PASS();
}
/* Reproduce-first guard for #493: when an allocation fails mid-merge,
* cbm_gitignore_merge must leave dst exactly as it was (atomic) and signal
* failure — never a partial merge that keeps some src patterns and drops
* others. The seam below injects a strdup failure on the 2nd src pattern.
* Without the atomic rollback this test is RED: the first src pattern ("a/")
* leaks into dst, so `matches(dst, "a", true)` is true. */
extern char *(*cbm_gitignore_merge_dup_hook_for_test)(const char *);
static int gi_dup_calls;
static int gi_dup_fail_at;
static char *gi_failing_dup(const char *s) {
if (++gi_dup_calls > gi_dup_fail_at) {
return NULL;
}
return strdup(s);
}
TEST(gi_merge_atomic_on_alloc_failure) {
cbm_gitignore_t *dst = cbm_gitignore_parse("*.log\n"); /* 1 pattern */
cbm_gitignore_t *src = cbm_gitignore_parse("a/\nb/\nc/\n"); /* 3 patterns */
ASSERT_NOT_NULL(dst);
ASSERT_NOT_NULL(src);
gi_dup_calls = 0;
gi_dup_fail_at = 1; /* first src pattern copies; second fails */
cbm_gitignore_merge_dup_hook_for_test = gi_failing_dup;
bool ok = cbm_gitignore_merge(dst, src);
cbm_gitignore_merge_dup_hook_for_test = NULL; /* restore for other tests */
ASSERT_FALSE(ok); /* failure is signalled, not silent */
/* dst unchanged: its own pattern still matches, and the partially copied
* src patterns were rolled back. */
ASSERT_TRUE(cbm_gitignore_matches(dst, "x.log", false));
ASSERT_FALSE(cbm_gitignore_matches(dst, "a", true));
ASSERT_FALSE(cbm_gitignore_matches(dst, "b", true));
cbm_gitignore_free(src);
cbm_gitignore_free(dst);
PASS();
}
/* ── Suite ─────────────────────────────────────────────────────── */
SUITE(gitignore) {
@@ -253,4 +297,5 @@ SUITE(gitignore) {
RUN_TEST(gi_merge_patterns);
RUN_TEST(gi_merge_into_empty);
RUN_TEST(gi_merge_null_safe);
RUN_TEST(gi_merge_atomic_on_alloc_failure);
}