From 4aad7d6f649db54a3b9441b3a98e2b0f1fcaf245 Mon Sep 17 00:00:00 2001 From: retoor Date: Mon, 14 Sep 2026 20:10:25 +0000 Subject: [PATCH] Fix real memory leak Gitea CI caught, that local testing had been masking Gitea CI flagged two things on the last push: 1. A -Wunused-result warning on an intentionally-ignored write() return value in test_crash_consistency.c's progress side-channel. Fixed with an explicit (void) cast and a comment explaining why ignoring it is safe (a short write there only makes the progress count more conservative, per that function's own existing documented tolerance). 2. A real LeakSanitizer failure -- 256 bytes across 4 allocations from vfs_new/vfs_unmount. Root cause: tests/test_journal_failure.c's two "reopen after the failure, verify recovery" blocks called vfs_unmount/backend_free/backend_free inside their `if (ov2)` branch (the normal, expected path) but vfs_free(v2) only on the `else` branch, which is never actually reached in practice. Fixed by moving vfs_free(v2) to run unconditionally after the if, in both blocks. This bug was invisible locally across many runs because local sanitizer verification had been using ASAN_OPTIONS=detect_leaks=0 -- adopted originally for a real reason (a SIGKILLed forked child in test_crash_consistency.c never runs its own exit-time leak check, so its allocations were never the actual concern) but applied to the whole test run, which also suppressed detection of this real bug in the *parent* process's own code. CI doesn't set that option, so it caught what local runs couldn't. Documented in CONTRIBUTING.md as a real process gap, not just a code bug: a local verification habit that diverges from what CI actually runs can let a real finding through until it reaches CI. Verified: confirmed the leak directly first (reproduced locally by dropping the detect_leaks=0 override, matching CI exactly, before touching any code), then confirmed the fix by re-running the same no-override sweep across all 8 test binaries with zero leaks found, plus a clean make all + make test. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01UqJpkdJ6Njnt1pw3CbghzB --- CHANGELOG.md | 10 ++++++++++ CONTRIBUTING.md | 20 ++++++++++++++++++++ tests/test_crash_consistency.c | 6 +++++- tests/test_journal_failure.c | 6 ++---- 4 files changed, 37 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 612dba6..f24fb79 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,16 @@ pushed to any remote — there is no public release yet, only a local one. review while investigating a question about data-integrity trustworthiness, then confirmed by direct reproduction before and after each fix, not merely inspected and assumed correct. +- **`tests/test_journal_failure.c` itself leaked memory on its normal, + expected code path** (`vfs_free(v2)` was only called on the unreached + `else` branch, in two separate reopen blocks) — invisible locally + because local sanitizer runs had been using + `ASAN_OPTIONS=detect_leaks=0`, caught by Gitea CI, which does not set + that option. Fixed, and the local/CI sanitizer-invocation mismatch that + let it go unnoticed is now called out explicitly in `CONTRIBUTING.md`. + Also fixed a `-Wunused-result` warning on an intentionally-ignored + `write()` return value in `tests/test_crash_consistency.c`'s progress + side-channel. ### Added - `tests/test_crash_consistency.c`: real `fork()`+`SIGKILL` fault diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 186356b..86ae0c2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -91,6 +91,26 @@ response is to say this plainly and re-run until a clean pass is obtained (or hand off to CI, which runs on an unrestricted VM where this has not been observed), not to suppress or silently ignore a `DEADLYSIGNAL` exit. +**A real gap this project's own process had, found the hard way: local +sanitizer runs had been using `ASAN_OPTIONS=detect_leaks=0`, which masked +a genuine memory leak that Gitea CI (which does not set that option) then +caught.** The override wasn't unreasonable on its own terms — a +`SIGKILL`ed forked child (as in `tests/test_crash_consistency.c`) never +runs its own exit-time leak check, so its allocations were never actually +the concern — but setting it for the *entire* test run also suppressed +LeakSanitizer for the parent process's own code, where a real bug +(`tests/test_journal_failure.c` was missing a `vfs_free(v2)` call on its +normal, expected code path, in two separate reopen blocks) went +undetected locally across many runs. **Do not add `detect_leaks=0` (or +any other blanket sanitizer-weakening option) to a local verification +habit without it also being in `.gitea/workflows/ci.yml`** — if CI and a +local run check different things, a real finding can pass locally and +only surface once it reaches CI, which is what happened here. If a fork ++`SIGKILL` test's own allocations genuinely need excluding, scope the +exclusion narrowly (e.g. a leak-suppression file naming the specific +allocation site) rather than disabling leak detection for the whole +binary. + A change to the index structure in `upper.c` (the `TreapNode`/`treap_*`/ `node_*` functions, `snapshot_upsert`, `snapshot_remove`, or the `UpperSnapshot`/`UpperEntry` representation) should be run through diff --git a/tests/test_crash_consistency.c b/tests/test_crash_consistency.c index 13335d6..522892c 100644 --- a/tests/test_crash_consistency.c +++ b/tests/test_crash_consistency.c @@ -221,7 +221,11 @@ static void record_progress(const char *progress_path, int n_done) { if (fd < 0) return; char buf[16]; int len = snprintf(buf, sizeof(buf), "%d", n_done); - write(fd, buf, (size_t)len); + /* Best-effort: a short write here only makes read_progress()'s count + * more conservative (see the imprecision note above), never wrong in + * the unsafe direction, so there is nothing more useful to do with a + * partial-write return value than the explicit (void) already says. */ + (void)write(fd, buf, (size_t)len); fsync(fd); close(fd); } diff --git a/tests/test_journal_failure.c b/tests/test_journal_failure.c index 1283c26..55889d1 100644 --- a/tests/test_journal_failure.c +++ b/tests/test_journal_failure.c @@ -124,9 +124,8 @@ int main(void) { vfs_unmount(v2, "/"); backend_free(ov2); backend_free(mem2); - } else { - vfs_free(v2); } + vfs_free(v2); } /* --- scenario 2: vfs_mkdir's journal append fails --- */ @@ -198,9 +197,8 @@ int main(void) { vfs_unmount(v2, "/"); backend_free(ov2); backend_free(mem2); - } else { - vfs_free(v2); } + vfs_free(v2); } reset_paths(pack_path, jpath);