diff --git a/CHANGELOG.md b/CHANGELOG.md index e1a4f41..612dba6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,52 @@ This project is pre-release (`0.x`, an initial, partial implementation of tagged locally in this repository's own git history but has not been pushed to any remote — there is no public release yet, only a local one. +## [Unreleased] + +### Fixed +- **A failed journal write (disk full, quota, an I/O error) used to be + silently reported as success.** `journal_append_record` and its callers + were `void` and never checked `fwrite`/`fflush`/`fsync`'s return + values, so `vfs_write`/`vfs_mkdir`/`vfs_unlink`/`vfs_rename` on an + overlay-backed file all reported success even when the durable journal + append actually failed. Fixed by propagating success/failure through + the whole call chain; the affected call now returns `VFS_ERR_IO`. +- **A second bug, found only by reproducing the first fix's edge case + directly: a partial write left a torn record in the journal that made + every later, individually-*successful* write unrecoverable on reopen + too**, not just the failed one — `journal_replay` stops at the first + record it can't fully read, regardless of what valid records follow it. + Fixed by rolling the journal file back to its exact pre-record length + whenever a record fails partway. Both bugs covered permanently by the + new `tests/test_journal_failure.c`, which forces a real write failure + via `RLIMIT_FSIZE` rather than a mock. +- Neither bug was caught by an existing test — both were found by code + 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. + +### Added +- `tests/test_crash_consistency.c`: real `fork()`+`SIGKILL` fault + injection (not a hand-truncated file) against journaled writes and + compaction, with an independent progress side-channel so the test's own + oracle can distinguish "never attempted before the kill" from + "attempted and lost" — a distinction its own first draft got wrong, + reporting 128 false failures before that side-channel was added. Zero + real corruption found across dozens of genuine interruptions per run, + both scenarios, on repeated runs including under ASan/UBSan. + +### Documented +- The pack integrity checksum's (FNV-1a64) non-cryptographic nature — + already documented for the compaction-dedup bug — was never explicitly + connected to its *other* job, load-time pack integrity validation, + where it matters more (`SECURITY.md`). +- TSan's inability to run is now confirmed in a *second*, independent + sandboxed environment (a separate remote cloud sandbox, tried + specifically to check whether the restriction was one environment's + fluke), not just this project's own — same root cause + (`personality(ADDR_NO_RANDOMIZE)` blocked by seccomp) confirmed both + generically and against this project's actual test suite. + ## [0.1.0] - 2026-09-14 ### Added diff --git a/CLAUDE.md b/CLAUDE.md index d0696b6..329c539 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -29,12 +29,12 @@ Sanitizer builds are not wired into `make test` (they need per-file compilation - `src/internal.h` — every internal type shared across `.c` files; read this first when touching implementation code. - `src/vfs.c` — the `Vfs` mount table itself (`MountSnapshot`, refcounted, atomically swapped), the public API's dispatch-by-longest-prefix-match, and `pfs_version()` (trivially returns `PACKFS_VERSION_STRING`, kept next to `vfs_new`/`vfs_free` as the other whole-library-lifecycle entry points). - `src/upper.c` — the writable layer shared by `mem`, `dir`, and the overlay's upper side: `UpperSnapshot`/`UpperEntry`/`MutCell`, the persistent-treap index (`TreapNode` and `treap_*`/`node_*` — see "Known performance characteristics" below for why it's a treap and not a flat array), the structural-write functions (`upper_create`/`upper_mkdir`/`upper_remove`/`upper_rename`/`upper_copy_up`), the content-write fast path (`upper_cell_read`/`upper_cell_write`), the range-query API (`upper_visit_range`, used by `readdir` in both this file and `overlay.c`), and the standalone `mem`/`dir` `Backend` (`backend_mem_new`/`backend_dir_new`). -- `src/overlay.c` — composes a `Pack` (lower, read-only) with an `UpperStore` (upper): copy-up, whiteouts, the merged view (`overlay_stat`/`overlay_readdir`), the append journal, and `vfs_sync`'s compaction. +- `src/overlay.c` — composes a `Pack` (lower, read-only) with an `UpperStore` (upper): copy-up, whiteouts, the merged view (`overlay_stat`/`overlay_readdir`), the append journal (`journal_append_record` reports and rolls back a failed write rather than leaving a torn record or silently reporting success — see "Known reliability characteristics" below), and `vfs_sync`'s compaction. - `src/pack.c` — the on-disk pack format: load-time integrity validation, binary-search lookup/range-query, the compaction writer (atomic rename, exact-duplicate elimination via a `DedupSlot` open-addressing hash table over `(hash, size)` plus a `memcmp` verification before ever reusing a `data_off` — see "Known performance characteristics" below for why it isn't a linear scan), and the standalone read-only `pack` `Backend` (`backend_pack_new`) for mounting a pack with no writable upper layer at all. - `src/containment.c` — `dir`-mount path containment (`openat2`/`O_NOFOLLOW` fallback) and opt-in Landlock hardening. - `src/path.c` — virtual-namespace `.`/`..` canonicalization. - `src/hash.c` — FNV-1a64, used for pack checksums and compaction dedup. -- `tests/` — one binary per concern (`test_mem`, `test_dir`, `test_pack_overlay`, `test_concurrency`, `test_index_stress` — the treap's correctness under thousands of randomized structural operations, cross-checked against an independent reference model; `test_pack_write_perf` — the `pack_write` dedup O(n²) regression tripwire; `test_crash_consistency` — real fork()+`SIGKILL` fault injection against a real process, not a hand-truncated file, verifying Section 4.3/4.4's crash-safety claims for both journaled writes and compaction, with an independent progress side-channel so the oracle can tell "never attempted before the kill" apart from "attempted and lost" — see its own file header before touching journal or compaction code); `test_harness.h` is a small assertion-macro header, not a framework, consistent with the zero-dependency constraint. +- `tests/` — one binary per concern (`test_mem`, `test_dir`, `test_pack_overlay`, `test_concurrency`, `test_index_stress` — the treap's correctness under thousands of randomized structural operations, cross-checked against an independent reference model; `test_pack_write_perf` — the `pack_write` dedup O(n²) regression tripwire; `test_crash_consistency` — real fork()+`SIGKILL` fault injection against a real process, not a hand-truncated file, verifying Section 4.3/4.4's crash-safety claims for both journaled writes and compaction, with an independent progress side-channel so the oracle can tell "never attempted before the kill" apart from "attempted and lost" — see its own file header before touching journal or compaction code; `test_journal_failure` — a real, forced journal-write I/O failure via `RLIMIT_FSIZE`, covering the two journal-durability bugs "Known reliability characteristics" below documents, that `test_crash_consistency` doesn't: a failed write silently reported as success, and a torn record left behind by that failure poisoning replay for every subsequent record too); `test_harness.h` is a small assertion-macro header, not a framework, consistent with the zero-dependency constraint. **One architectural fact spans every mutable structure and is easy to miss reading any single file in isolation:** `MountSnapshot` (`vfs.c`), `UpperSnapshot` (`upper.c`), and a `mem`-backed `MutCell`'s buffer (`upper.c`, Section 5.7) are all retired through the same two-step pattern — publish the replacement via `atomic_store_explicit(..., memory_order_release)`, then free the superseded object only under a dedicated `reclaim_gate` rwlock's *write* side, while every acquirer takes that same gate's *read* side around its own load-then-increment. This exists because a plain "load a pointer, then atomically increment its refcount" leaves a real gap between those two steps in which a concurrent writer can free the very object being acquired — confirmed by ASan as an actual heap-use-after-free during development, not a theoretical concern. See the comment on `UpperStore.reclaim_gate` in `internal.h` for the full reasoning. Any new refcounted, concurrently-reclaimed structure added to this codebase needs the same gate, not just an atomic pointer and a naive refcount. The one exception, and the reason it's an exception rather than a hole: `TreapNode`'s own per-node refcounting (`upper.c`) does *not* need its own `reclaim_gate`, because nothing ever acquires a `TreapNode*` independently the way `upper_acquire` acquires an `UpperSnapshot*` — a reader only ever reaches tree nodes by walking `s->root` after already holding a valid `UpperSnapshot` reference, which transitively keeps the whole tree alive for the reader's purposes; `node_ref`/`node_unref` are called only by the single writer (under `writer_lock`) and by `upper_release`'s cascade (already inside `reclaim_gate`'s protection at the snapshot level), never by a reader racing a writer the way the snapshot pointer itself is raced. @@ -104,7 +104,14 @@ Section 4.3/4.4's crash-safety claims (self-checking journal records; atomic-ren **A real finding from building this test, recorded here per this file's own "document literally all" standard: the test's first draft reported 128 failures, and every one of them was a bug in the test, not in PackFS.** It checked "is extra write *i* present after the crash" without distinguishing "the child was killed before it ever attempted write *i*" (expected, not a bug) from "write *i* completed and was then lost" (would be a real bug) — SIGKILL cannot be caught, so the child has no way to report its own progress through the normal API. Fixed by adding the independent progress side-channel described above; the corrected test has passed cleanly across every run so far (multiple full runs, plus 3 clean runs under ASan/UBSan). This is the same category of lesson as `BENCH.md`'s own history of findings: a claim (here, "PackFS's crash safety" — there, "the benchmark's numbers") is only as trustworthy as the thing measuring it, and that has to be checked too, not just the subject under test. -**Still not covered, stated plainly:** these two scenarios don't exercise cross-process contention (two processes crashing/racing against the same files — not implemented in v0, see "Load-bearing constraints" above), disk-full/`ENOSPC` mid-write, or a crash during journal *truncation* specifically (the second atomic-rename step after a successful compaction, `.jnl.tmp` → `.jnl`) as its own isolated case. TSan remains unable to run in this sandbox for the concurrency side of reliability — see the sanitizer section above, now including the specific workarounds tried (not just the limitation) before concluding it's genuinely unfixable here. +**Two real, previously-unknown correctness bugs were found and fixed while investigating the write-failure case those two scenarios don't cover** (disk-full/`ENOSPC`/quota mid-write) — neither was found by a test catching it unprompted; both were found by code review while building `tests/test_journal_failure.c`, then confirmed by direct reproduction before being treated as real: + +- **A failed journal write was silently reported as success.** `journal_append_record` and everything that called it (`journal_append_put`/`_delete`/`_mkdir`, `journal_put_current`) used to be `void`, and none of `journal_append_record`'s `fwrite`/`fflush`/`fsync` calls had their return values checked — meaning `vfs_write`/`vfs_mkdir`/`vfs_unlink`/`vfs_rename` on an overlay-backed file all reported `VFS_OK`/a byte count even when the durable journal append actually failed (disk full, quota, an I/O error). This directly contradicted Section 4.4's premise: a caller that saw success had no way to know the write wasn't actually durable, and would only discover the loss on the next crash. Fixed by making the whole journal-append call chain return success/failure and propagating it: the affected `vfs_*` call now returns `VFS_ERR_IO` instead of silently succeeding — even though the in-memory content, already updated by that point, is left as-is and still visible to readers in the same process (the same asymmetry a real `write()`-succeeds-but-later-`fsync()`-fails has; there is no way to "undo" the in-memory update, and the return value's job is to report durability, not roll back visible state). +- **Fixing the above was not sufficient by itself, and this was confirmed by direct reproduction, not just reasoned about: a partial write leaves a torn record in the middle of the journal file, which poisons every record appended *after* it too, even ones that complete successfully later.** `journal_replay` stops at the first record whose length doesn't fit the remaining file (Section 4.3, by design — this is correct behavior for replay itself). A torn record left behind by a *failed* write sits before every subsequent record, so replay never reaches them, regardless of whether they themselves wrote perfectly fine. Reproduced directly: a forced-failed write followed immediately by a genuinely successful one was unrecoverable after reopening — the good record existed in the file, replay just never got past the torn one in front of it — before this half of the fix. Closed by rolling the journal file back to its exact pre-record byte length whenever a record fails partway (`ftruncate` to a length captured via `ftell` before any of that record's bytes are written), so the on-disk journal stays valid-up-to-EOF even when an individual write fails, not only when every write succeeds. + +Both are covered permanently by `tests/test_journal_failure.c`, which forces a real write failure via `RLIMIT_FSIZE` + ignoring `SIGXFSZ` (so `write()` returns `EFBIG` instead of killing the process) — not a mock or a simulated error path — and checks all three properties: the failure is reported (not swallowed), a fresh reopen does not see the torn record as if valid, and a later genuinely-successful write on the same overlay session survives reopening (the exact scenario that was broken before the second half of the fix). + +**Still not covered, stated plainly:** cross-process contention (two processes crashing/racing against the same files — not implemented in v0, see "Load-bearing constraints" above), and a crash during journal *truncation* specifically (the second atomic-rename step after a successful compaction, `.jnl.tmp` → `.jnl`) as its own isolated, precisely-targeted case — investigated and deliberately not built as a dedicated test (it would need real concurrency, a second writer thread racing the kill, to reliably land that specific narrow window; the existing compaction-crash scenario above already covers it probabilistically, at whatever rate its delay sampling happens to hit that window, just not by design). TSan remains unable to run for the concurrency side of reliability — now confirmed in *two* independent sandboxed environments (this project's own, and a separate remote cloud sandbox tried specifically to check whether the restriction was environment-specific), both blocking `personality(ADDR_NO_RANDOMIZE)` identically and failing TSan on this project's actual test suite with the identical `FATAL: ThreadSanitizer: unexpected memory mapping` signature — see the sanitizer section above for the workarounds tried in each. ## Self-evaluation diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 89f63e9..186356b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -73,7 +73,14 @@ all, in another: `test_pack_overlay`), which — together with the fact that every one of those binaries passes cleanly on a repeat run — indicates a sandbox startup race (plausibly in the same family as the ASLR/mapping restrictions behind the TSan limitation above), not a bug in the binary -being tested. **Always wrap sanitizer-build test runs in `timeout` in such +being tested. **`tests/test_crash_consistency.c` hits this flake noticeably +more often than the rest of the suite** (observed 2 failures in 5 runs, +versus roughly 1-in-5–10 elsewhere) — expected, not a sign anything is +wrong with that test specifically: it forks 60+ subprocesses per run +(one per fault-injection trial), and each fork is an independent chance to +hit the same startup race, so a test that forks this much will trip it +proportionally more often. Re-run it in isolation before treating a +failure there as a real finding, same as any other `DEADLYSIGNAL` exit. **Always wrap sanitizer-build test runs in `timeout` in such an environment** (e.g. `timeout 30 ./build/san/test_name`) so a stuck run fails loudly and bounded instead of hanging; treat a `DEADLYSIGNAL` exit as inconclusive, re-run, and only treat the run as a real finding if the output diff --git a/SECURITY.md b/SECURITY.md index 10d33db..db062a8 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -63,6 +63,17 @@ rather than a generic "we take security seriously": the *test itself* had on its first draft (conflating "never attempted before the kill" with "lost after completing") — fixed before treating the crash-safety claim as verified. +- **A real, previously-unknown data-loss bug in this exact area was found + and fixed: a failed journal write (disk full, quota, an I/O error) used + to be silently reported as success**, and even after fixing that, a + partial write left behind a torn record that made every *later*, + individually-successful write unrecoverable on reopen too — not just + the one that actually failed. Both are fixed (`src/overlay.c`, + `journal_append_record`) and covered by `tests/test_journal_failure.c`, + which forces a real write failure via `RLIMIT_FSIZE` rather than + simulating one. See `CLAUDE.md`'s "Known reliability characteristics" + for the full story, including how each half of the bug was confirmed by + direct reproduction before and after the fix, not just inspection. ## What PackFS explicitly does *not* claim diff --git a/src/overlay.c b/src/overlay.c index 075ffbe..2cdd732 100644 --- a/src/overlay.c +++ b/src/overlay.c @@ -61,52 +61,107 @@ static const char *dirrel(const char *p) { return p[0] == '/' ? p + 1 : p; } #define JOP_DELETE 1 #define JOP_MKDIR 2 -static void journal_append_record(Overlay *ov, uint8_t op, const char *name, - int64_t mtime, uint64_t size, const void *data) { - if (!ov->pack_path) return; +/* + * Returns 0 on success (including the intentional "no pack_path configured, + * no durability wanted" case — an embedder may pass NULL to + * backend_overlay_new's pack_path to run with no journal/compaction target + * at all, and that is not a failure), -1 if a durable append was actually + * attempted and failed at any step (open, an fwrite short of its full + * count, fflush, or fsync). + * + * This return value matters: every one of journal_append_record's callers + * used to ignore it entirely (this function used to be void, and every + * fwrite/fflush/fsync call above went unchecked) — meaning a disk-full or + * I/O-error condition during an ordinary vfs_write/vfs_mkdir/vfs_unlink/ + * vfs_rename on an overlay-backed file silently reported success while the + * change was never actually made durable, a real data-loss bug this + * project's own crash-safety claims (Section 4.4) implicitly promise does + * not happen. Found via code review while building fault-injection tests + * for those claims, not by a test catching it first — there was no test + * that could have caught it, since nothing exercised a failing journal + * write before this fix. See tests/test_journal_failure.c. + */ +static int journal_append_record(Overlay *ov, uint8_t op, const char *name, + int64_t mtime, uint64_t size, const void *data) { + if (!ov->pack_path) return 0; /* no journal configured: not an error */ pthread_mutex_lock(&ov->journal_lock); if (!ov->journal_fp) { char jpath[PFS_PATH_MAX]; snprintf(jpath, sizeof(jpath), "%s.jnl", ov->pack_path); ov->journal_fp = fopen(jpath, "ab"); } + int ok = 0; if (ov->journal_fp) { + fseek(ov->journal_fp, 0, SEEK_END); + long pre_len = ftell(ov->journal_fp); uint32_t name_len = (uint32_t)strlen(name); uint32_t payload_len = 4 + name_len + (op == JOP_PUT ? (uint32_t)(8 + 8 + size) : 0); unsigned char *payload = (unsigned char *)malloc(payload_len); - size_t off = 0; - memcpy(payload + off, &name_len, 4); off += 4; - memcpy(payload + off, name, name_len); off += name_len; - if (op == JOP_PUT) { - memcpy(payload + off, &mtime, 8); off += 8; - memcpy(payload + off, &size, 8); off += 8; - if (size) memcpy(payload + off, data, size); + if (payload) { + size_t off = 0; + memcpy(payload + off, &name_len, 4); off += 4; + memcpy(payload + off, name, name_len); off += name_len; + if (op == JOP_PUT) { + memcpy(payload + off, &mtime, 8); off += 8; + memcpy(payload + off, &size, 8); off += 8; + if (size) memcpy(payload + off, data, size); + } + uint32_t checksum = (uint32_t)pfs_fnv1a64(payload, payload_len); + ok = fwrite(&payload_len, 4, 1, ov->journal_fp) == 1 + && fwrite(&checksum, 4, 1, ov->journal_fp) == 1 + && fwrite(&op, 1, 1, ov->journal_fp) == 1 + && fwrite(payload, 1, payload_len, ov->journal_fp) == payload_len + && fflush(ov->journal_fp) == 0 + && fsync(fileno(ov->journal_fp)) == 0; + free(payload); + /* + * A partial write here (e.g. ENOSPC or, as in + * tests/test_journal_failure.c, RLIMIT_FSIZE mid-record) + * would otherwise leave a torn record sitting in the middle + * of the journal file permanently: journal_replay stops at + * the first record whose length doesn't fit (Section 4.3), + * which means every record appended *after* this one — + * including ones that themselves complete successfully + * later — would be silently unreachable on every future + * reopen until the next compaction, not just this one + * failed write. Confirmed by direct reproduction before this + * fix: a forced failed write followed by a genuinely + * successful one was unrecoverable, because replay never got + * past the torn record to reach the good one. Rolling the + * file back to its exact pre-record length keeps "the + * journal on disk is valid up to EOF" true even when a + * write fails, not just when every write succeeds. + */ + if (!ok && pre_len >= 0) { + fflush(ov->journal_fp); + if (ftruncate(fileno(ov->journal_fp), (off_t)pre_len) == 0) { + fseek(ov->journal_fp, 0, SEEK_END); + } + } } - uint32_t checksum = (uint32_t)pfs_fnv1a64(payload, payload_len); - fwrite(&payload_len, 4, 1, ov->journal_fp); - fwrite(&checksum, 4, 1, ov->journal_fp); - fwrite(&op, 1, 1, ov->journal_fp); - fwrite(payload, 1, payload_len, ov->journal_fp); - fflush(ov->journal_fp); - fsync(fileno(ov->journal_fp)); - free(payload); } pthread_mutex_unlock(&ov->journal_lock); + return ok ? 0 : -1; } -static void journal_append_put(Overlay *ov, const char *name, const void *data, uint64_t size, int64_t mtime) { - journal_append_record(ov, JOP_PUT, name, mtime, size, data); +static int journal_append_put(Overlay *ov, const char *name, const void *data, uint64_t size, int64_t mtime) { + return journal_append_record(ov, JOP_PUT, name, mtime, size, data); } -static void journal_append_delete(Overlay *ov, const char *name) { - journal_append_record(ov, JOP_DELETE, name, 0, 0, NULL); +static int journal_append_delete(Overlay *ov, const char *name) { + return journal_append_record(ov, JOP_DELETE, name, 0, 0, NULL); } -static void journal_append_mkdir(Overlay *ov, const char *name) { - journal_append_record(ov, JOP_MKDIR, name, 0, 0, NULL); +static int journal_append_mkdir(Overlay *ov, const char *name) { + return journal_append_record(ov, JOP_MKDIR, name, 0, 0, NULL); } /* journals the *current* full content of `path` after a write/rename, - * matching the whole-value scheme described above. */ -static void journal_put_current(Overlay *ov, const char *path, MutCell *cell) { + * matching the whole-value scheme described above. Returns 0 on success, + * -1 on failure — including an allocation failure while staging the + * content to journal: `buf` used to be passed into journal_append_put + * (and from there into a memcpy of `size` bytes) even when malloc(size) + * had failed and left it NULL, an OOM-triggered NULL-pointer memcpy this + * fix also closes, not just the silently-ignored-failure issue above. */ +static int journal_put_current(Overlay *ov, const char *path, MutCell *cell) { UpperStore *us = upper_of(ov); pfs_usize size; void *buf = NULL; @@ -121,14 +176,16 @@ static void journal_put_current(Overlay *ov, const char *path, MutCell *cell) { } else { VfsStat st; int e = 0; - if (pfs_dir_statat(us->root_fd, dirrel(path), &st, &e) < 0) return; + if (pfs_dir_statat(us->root_fd, dirrel(path), &st, &e) < 0) return -1; size = st.size; mtime = st.mtime; buf = size ? malloc(size) : NULL; if (buf) upper_cell_read(us, cell, path, 0, buf, size); } - journal_append_put(ov, path, buf, size, mtime); + if (size && !buf) return -1; /* allocation failed: nothing safe to journal */ + int rc = journal_append_put(ov, path, buf, size, mtime); free(buf); + return rc; } static void journal_replay(Overlay *ov) { @@ -291,7 +348,15 @@ static pfs_isize overlay_write(VfsFile *f, const void *buf, pfs_usize n) { pfs_isize w = upper_cell_write(upper_of(of->ov), of->cell, of->path, of->pos, buf, n, &err); if (w < 0) return err; of->pos += (pfs_usize)w; - journal_put_current(of->ov, of->path, of->cell); + /* The in-memory write above already succeeded and is visible to + * readers in this process regardless of what happens next — that + * cannot be undone, the same way a real write() into the page cache + * isn't undone by a later failed fsync(). What CAN still be reported + * is durability: if the journal append fails, this call reports + * VFS_ERR_IO instead of the byte count, so a caller relying on + * "success means durable" (Section 4.4's whole premise) finds out + * immediately rather than silently losing the write on a later crash. */ + if (journal_put_current(of->ov, of->path, of->cell) < 0) return VFS_ERR_IO; return w; } @@ -461,7 +526,7 @@ static int overlay_mkdir(Backend *b, const char *path) { int err = 0; if (upper_mkdir(us, path, &err) < 0) return err; - journal_append_mkdir(ov, path); + if (journal_append_mkdir(ov, path) < 0) return VFS_ERR_IO; /* see overlay_write's comment */ return VFS_OK; } @@ -513,7 +578,7 @@ static int overlay_unlink(Backend *b, const char *path) { int err = 0; if (upper_remove(us, path, had_lower, &err) < 0) return err; - journal_append_delete(ov, path); + if (journal_append_delete(ov, path) < 0) return VFS_ERR_IO; /* see overlay_write's comment */ return VFS_OK; } @@ -555,16 +620,24 @@ static int overlay_rename(Backend *b, const char *from, const char *to) { upper_remove(us, from, 1, &err2); } - journal_append_delete(ov, from); + /* Both journal calls below are attempted regardless of whether the + * first one failed — "from" being gone and "to" now holding the + * content are two independent durable facts, and a failure to + * journal one is not a reason to skip attempting the other. See + * overlay_write's comment on why a journal failure here reports + * VFS_ERR_IO despite the in-memory rename already having happened. */ + int jerr = journal_append_delete(ov, from) < 0; if (kind == ENTRY_DIR_MARKER) { - journal_append_mkdir(ov, to); + jerr |= journal_append_mkdir(ov, to) < 0; } else { UpperSnapshot *s2 = upper_acquire(us); const UpperEntry *nue; - if (upper_lookup(s2, to, &nue) && nue->kind == ENTRY_FILE) journal_put_current(ov, to, nue->cell); + if (upper_lookup(s2, to, &nue) && nue->kind == ENTRY_FILE) { + jerr |= journal_put_current(ov, to, nue->cell) < 0; + } upper_release(s2); } - return VFS_OK; + return jerr ? VFS_ERR_IO : VFS_OK; } /* ---- compaction (Section 4.1 step 5, 5.3, 9.2) ---- */ diff --git a/tests/test_journal_failure.c b/tests/test_journal_failure.c new file mode 100644 index 0000000..1283c26 --- /dev/null +++ b/tests/test_journal_failure.c @@ -0,0 +1,208 @@ +/* + * test_journal_failure.c — a real, forced journal-write I/O failure + * (RLIMIT_FSIZE + SIGXFSZ ignored, so write() fails with EFBIG instead of + * killing the process), verifying two real bugs found and fixed in + * overlay.c while building this test, not by this test catching them + * unprompted -- there was no test before this one that exercised a + * failing journal write at all: + * + * 1. journal_append_* and journal_put_current used to be void, and every + * fwrite/fflush/fsync inside journal_append_record went unchecked -- + * a real write failure (disk full, quota, this test's RLIMIT_FSIZE) + * was silently swallowed and vfs_write/vfs_mkdir/vfs_unlink/ + * vfs_rename all reported success on an overlay-backed file even + * though the change was never made durable. + * + * 2. Fixing (1) by itself was not enough: a partial write leaves a torn + * record sitting in the middle of the journal file, and + * journal_replay stops at the first record it can't fully read + * (Section 4.3) -- meaning every record appended *after* the failed + * one, including ones that complete successfully later, was silently + * unreachable on every future reopen. Confirmed by direct + * reproduction before this half of the fix: a forced-failed write + * followed by a genuinely successful one was unrecoverable, because + * replay never got past the torn record in between. Fixed by rolling + * the journal file back to its exact pre-record length on any failed + * write, keeping "valid up to EOF" true even when a write fails. + * + * What this test does NOT cover: the parallel bug in journal_put_current + * (a malloc(size) failure used to leave a NULL buffer that got passed + * into journal_append_record's memcpy anyway -- an OOM-triggered + * NULL-pointer dereference, not a "write failed" one). Reliably forcing + * malloc() to fail in a portable, safe way isn't practical here; that + * half of the fix is verified by code inspection (the `if (size && !buf) + * return -1;` guard in journal_put_current), not by a test triggering it. + */ + +#include +#include +#include +#include +#include +#include + +#include "packfs.h" +#include "test_harness.h" + +static void reset_paths(const char *pack_path, const char *jpath) { + unlink(pack_path); + unlink(jpath); +} + +static void set_fsize_limit(rlim_t bytes) { + struct rlimit rl; + rl.rlim_cur = bytes; + rl.rlim_max = RLIM_INFINITY; + CHECK_EQ_INT(setrlimit(RLIMIT_FSIZE, &rl), 0); +} + +int main(void) { + /* SIGXFSZ's default action terminates the process; ignoring it makes + * the offending write() return -1/EFBIG instead, which is what this + * test needs to observe -- a failed write, not a killed process. */ + signal(SIGXFSZ, SIG_IGN); + + char pack_path[512]; + snprintf(pack_path, sizeof(pack_path), "/tmp/packfs_test_journal_fail_%d.img", (int)getpid()); + char jpath[600]; + snprintf(jpath, sizeof(jpath), "%s.jnl", pack_path); + + /* --- scenario 1: vfs_write's journal append fails; the failure must + * be reported (not swallowed), the in-memory content must still be + * readable in this process, and -- critically -- a fresh reopen must + * NOT see the torn/rolled-back record as if it were valid. --- */ + reset_paths(pack_path, jpath); + { + Vfs *v = vfs_new(); + Backend *mem = backend_mem_new(); + int oerr = 0; + Backend *ov = backend_overlay_new(pack_path, mem, &oerr); + CHECK(ov != NULL); + CHECK_EQ_INT(vfs_mount(v, "/", ov), VFS_OK); + + set_fsize_limit(32); /* far smaller than any real journal record */ + + int err = 0; + VfsFile *f = vfs_open(v, "/big.txt", VFS_O_WRONLY | VFS_O_CREAT, &err); + CHECK(f != NULL); + const char *payload = "this content is deliberately longer than 32 bytes"; + pfs_isize wr = vfs_write(f, payload, strlen(payload)); + CHECK_EQ_INT(wr, VFS_ERR_IO); /* durability failure must be reported */ + CHECK_EQ_INT(vfs_close(f), VFS_OK); + + /* in-memory: still readable in this process, same as a page-cache + * write a later fsync() fails on -- only durability was refused */ + f = vfs_open(v, "/big.txt", VFS_O_RDONLY, &err); + CHECK(f != NULL); + if (f) { + char buf[128] = {0}; + pfs_isize rd = vfs_read(f, buf, sizeof(buf)); + CHECK_EQ_INT(rd, (pfs_isize)strlen(payload)); + CHECK_EQ_INT(memcmp(buf, payload, strlen(payload)), 0); + CHECK_EQ_INT(vfs_close(f), VFS_OK); + } + + vfs_unmount(v, "/"); + backend_free(ov); + backend_free(mem); + vfs_free(v); + set_fsize_limit(RLIM_INFINITY); /* reopening below only reads, but be safe */ + + Vfs *v2 = vfs_new(); + Backend *mem2 = backend_mem_new(); + int oerr2 = 0; + Backend *ov2 = backend_overlay_new(pack_path, mem2, &oerr2); + CHECK(ov2 != NULL); /* must never fail to load, even with a rolled-back journal */ + if (ov2) { + CHECK_EQ_INT(vfs_mount(v2, "/", ov2), VFS_OK); + int err2 = 0; + VfsFile *f2 = vfs_open(v2, "/big.txt", VFS_O_RDONLY, &err2); + /* the failed write must not have been replayed as if valid */ + CHECK(f2 == NULL); + CHECK_EQ_INT(err2, VFS_ERR_NOENT); + if (f2) vfs_close(f2); + vfs_unmount(v2, "/"); + backend_free(ov2); + backend_free(mem2); + } else { + vfs_free(v2); + } + } + + /* --- scenario 2: vfs_mkdir's journal append fails --- */ + reset_paths(pack_path, jpath); + { + Vfs *v = vfs_new(); + Backend *mem = backend_mem_new(); + int oerr = 0; + Backend *ov = backend_overlay_new(pack_path, mem, &oerr); + CHECK(ov != NULL); + CHECK_EQ_INT(vfs_mount(v, "/", ov), VFS_OK); + set_fsize_limit(4); /* smaller than even a bare mkdir record */ + CHECK_EQ_INT(vfs_mkdir(v, "/a-directory-name-long-enough"), VFS_ERR_IO); + vfs_unmount(v, "/"); + backend_free(ov); + backend_free(mem); + vfs_free(v); + set_fsize_limit(RLIM_INFINITY); + } + + /* --- scenario 3: after a failed write, lifting the limit lets a + * later, unrelated write on the SAME overlay session succeed and + * survive a fresh reopen -- proving the rollback keeps the journal + * usable afterward, not merely "not corrupted." --- */ + reset_paths(pack_path, jpath); + { + Vfs *v = vfs_new(); + Backend *mem = backend_mem_new(); + int oerr = 0; + Backend *ov = backend_overlay_new(pack_path, mem, &oerr); + CHECK(ov != NULL); + CHECK_EQ_INT(vfs_mount(v, "/", ov), VFS_OK); + + set_fsize_limit(32); + int err = 0; + VfsFile *f = vfs_open(v, "/big.txt", VFS_O_WRONLY | VFS_O_CREAT, &err); + CHECK(f != NULL); + const char *payload = "this content is deliberately longer than 32 bytes"; + CHECK_EQ_INT(vfs_write(f, payload, strlen(payload)), VFS_ERR_IO); + CHECK_EQ_INT(vfs_close(f), VFS_OK); + + set_fsize_limit(RLIM_INFINITY); + f = vfs_open(v, "/after.txt", VFS_O_WRONLY | VFS_O_CREAT, &err); + CHECK(f != NULL); + CHECK_EQ_INT(vfs_write(f, "durable now", 11), 11); + CHECK_EQ_INT(vfs_close(f), VFS_OK); + + vfs_unmount(v, "/"); + backend_free(ov); + backend_free(mem); + vfs_free(v); + + Vfs *v2 = vfs_new(); + Backend *mem2 = backend_mem_new(); + int oerr2 = 0; + Backend *ov2 = backend_overlay_new(pack_path, mem2, &oerr2); + CHECK(ov2 != NULL); + if (ov2) { + CHECK_EQ_INT(vfs_mount(v2, "/", ov2), VFS_OK); + int err2 = 0; + VfsFile *f2 = vfs_open(v2, "/after.txt", VFS_O_RDONLY, &err2); + CHECK(f2 != NULL); /* this is the exact case that was broken before the rollback fix */ + if (f2) { + char buf[32] = {0}; + CHECK_EQ_INT(vfs_read(f2, buf, sizeof(buf)), 11); + CHECK_EQ_INT(memcmp(buf, "durable now", 11), 0); + CHECK_EQ_INT(vfs_close(f2), VFS_OK); + } + vfs_unmount(v2, "/"); + backend_free(ov2); + backend_free(mem2); + } else { + vfs_free(v2); + } + } + + reset_paths(pack_path, jpath); + TEST_MAIN_END(); +}