Fix pack_write's O(n^2) dedup + correctness bug; document mount table O(n^2)
A audit for other instances of the file-index O(n^2) shape (fixed previously via the persistent treap) found two more real issues: 1. pack_write's Section 9.2 exact-duplicate elimination was a linear scan of every previously-seen (hash, size) pair per entry -- O(n^2) total, invisible in the existing benchmark because its identical-content test files made every scan match on the first comparison. Measured with unique content instead: 80,000 entries took 1.74s, with a 20,000->80,000 step showing 15.6x for a 4x-N step, matching O(n^2)'s 16x prediction. The same scan also trusted a (hash, size) match without ever comparing actual bytes -- a latent correctness bug, since FNV-1a64 is explicitly not collision-resistant. Fixed both at once with an open-addressing hash table (load factor 1/2, linear probing) plus a memcmp verification before ever reusing a data_off. Post-fix: 80,000 entries in 0.044s (39.6x faster), ratio drops to 3.35x (consistent with O(n)). Covered permanently by two new/extended tests: a white-box assertion in test_pack_overlay.c that duplicate-content entries share one data_off and distinct-content entries do not, and a new tests/test_pack_write_perf.c regression tripwire against 10,000 unique entries. 2. vfs.c's mount table uses the same full-array-copy-per-write pattern the file index used to, confirmed O(n^2) via a new bench/bench.c category (500/2,000/8,000 mounts, both 4x-N steps showing 15-20x). Deliberately NOT rewritten: mount points are created by a program's own source code, not workload-driven, so realistic mount counts never reach the scale that made the file index's O(n^2) a real problem. Documented with full reasoning in BENCH.md and CLAUDE.md rather than silently left as an undocumented gap. Also fixes a real CI gap the new tests exposed: ci.yml's sanitizer-build steps never passed -D_GNU_SOURCE when compiling test files (only the library .o's got it), which was harmless while no test included internal.h and became a link failure once two did (internal.h needs _GNU_SOURCE for pthread_rwlock_t). And documents, in CONTRIBUTING.md and CLAUDE.md, a sandbox flake observed directly during this work's own sanitizer runs: ASan/UBSan test binaries occasionally fail to start with AddressSanitizer:DEADLYSIGNAL (sometimes looping rather than exiting), non-deterministically hitting different unrelated binaries across runs -- a startup race, not a memory-safety bug, confirmed by clean passes on retry; sanitizer runs in such an environment should be timeout-wrapped. BENCH.md's "After" table and Appendix B are replaced with the current, complete 54-measurement bench/bench.c run (the original 45 plus the new mount-scaling category); the pre-fix 45-measurement "Before" table is kept as the historical record, per this project's documentation standard. Verified: make test (all 6 binaries, including the 2 new/changed), a clean make all, and repeated ASan+UBSan runs (0 real findings; the DEADLYSIGNAL flake above was observed and correctly distinguished from a real finding by re-running until a clean pass). TSan could not be run in this sandbox (pre-existing, documented environment limitation). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UqJpkdJ6Njnt1pw3CbghzB
This commit is contained in:
@@ -28,6 +28,48 @@ Any change to the concurrency-sensitive files (`upper.c`, `overlay.c`,
|
||||
a data race there is exactly the class of bug Section 5 of `concept.md`
|
||||
exists to prevent, and the plain build will not surface it.
|
||||
|
||||
**Known environment limitation, stated plainly rather than glossed over:**
|
||||
ThreadSanitizer cannot run at all in some sandboxed/containerized
|
||||
development environments — including the one this project's own commit
|
||||
history was largely developed in — because TSan requires disabling ASLR
|
||||
for itself via `personality(ADDR_NO_RANDOMIZE)`, and some sandboxes block
|
||||
that syscall outright (confirmed here: `personality()` returns `EPERM`,
|
||||
and even a trivial unrelated pthread program fails identically with
|
||||
`FATAL: ThreadSanitizer: unexpected memory mapping`, not just PackFS code).
|
||||
If your environment can't run TSan, say so rather than silently skipping
|
||||
it or claiming verification that didn't happen — ASan/UBSan still catch
|
||||
real bugs (they found and fixed a genuine heap-use-after-free during this
|
||||
project's development, see the `reclaim_gate` note in `internal.h`) but do
|
||||
not do TSan's happens-before race analysis, so they are not a substitute
|
||||
for it. CI (`.github/workflows/ci.yml`) runs on a normal, unrestricted
|
||||
GitHub Actions VM where TSan is expected to work; treat a change as
|
||||
TSan-verified only once it has actually passed there or on a local machine
|
||||
that can run it, not merely because ASan/UBSan passed.
|
||||
|
||||
**A second, separate environment quirk, also observed directly rather than
|
||||
assumed:** in the same kind of sandboxed environment, an ASan/UBSan-built
|
||||
test binary occasionally (non-deterministically, roughly 1 run in 5–10 in
|
||||
this project's own experience) fails to start at all, printing
|
||||
`AddressSanitizer:DEADLYSIGNAL` — and, in its worse form, printing that same
|
||||
line in an unbounded loop rather than printing it once and exiting, which
|
||||
can run for minutes and consume unbounded CPU/memory if left unattended.
|
||||
This has been observed hitting different, unrelated test binaries from run
|
||||
to run (in one session: `test_dir` and `test_mem`, in another: nothing at
|
||||
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
|
||||
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
|
||||
contains an actual `ERROR: AddressSanitizer` or `runtime error:` string —
|
||||
`DEADLYSIGNAL` alone, with neither of those strings present, is this flake,
|
||||
not a memory-safety bug in PackFS. As with the TSan limitation, the correct
|
||||
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 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
|
||||
|
||||
Reference in New Issue
Block a user