Skip to content

fix: four BUG-severity findings from the 2026-07-02 deep review (I143–I146) - #89

Merged
Xof merged 1 commit into
mainfrom
fix/deepdive-20260702-bugs
Jul 2, 2026
Merged

fix: four BUG-severity findings from the 2026-07-02 deep review (I143–I146)#89
Xof merged 1 commit into
mainfrom
fix/deepdive-20260702-bugs

Conversation

@Xof

@Xof Xof commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes the four BUG-severity findings from the fresh-eyes deep review (2026-07-02), each verified against the code by an independent adversarial pass. Tracked as I143–I146 in ISSUES.md (added in the companion review PR).

Fixes

I143 — TOCTOU create-vs-open race (src/lib.rs)
Chisel::open chose create_new vs open_existing from an unlocked path.exists() stat taken before the flock. Two processes racing open() could have B stat an empty/absent file, A concurrently create + commit + release the lock, then B run create_new over A's committed data. Now the create/open decision uses io.page_count() — the count cached from the file length observed after the lock is held. The pre-lock stat is retained only for the create_if_missing gate (which must stay pre-lock so a refused open never materializes an empty file).

I144 — unvalidated stride → panic / DoS on open (src/transaction/recovery.rs)
The encrypted crypto-header's stride (plaintext, guarded only by the forgeable XXH3 checksum) was passed verbatim to set_stride, which computes file_len / stride. A forged stride = 0 was a guaranteed division-by-zero panic (violating the poison-not-panic model); a huge value drove multi-GiB read allocations. Now stride must equal ENC_PAGE_SIZE before set_stride; a mismatch returns CorruptSuperblock. This also makes the crypto_header.rs "validated by the engine" comment true. Regression test: corrupt_crypto_header_stride_errors_not_panic forges the on-disk stride to 0, re-stamps the checksum, and asserts the reopen returns CorruptSuperblock rather than panicking.

I145 — reclaim_freemap_orphans not poison-wrapped (src/transaction/freemap.rs)
Every other TransactionManager entry point routes fatal errors through poison_on_fatal; the orphan sweep did not. A fatal error mid-sweep returned un-poisoned, and reclaim_orphans writes the partially-advanced freemap root back into current_roots even on its error path — leaving a usable manager holding an indeterminate freemap. Now wrapped in poison_on_fatal like the rest (inner cache borrow scoped so it's released before poison_on_fatal takes &self).

I146 — README contradicted the code on a durability contract (README.md)
The format-compatibility section said the newer-minor write-refusal gate was "not yet wired up." It shipped as I29 (recovery.rs, forces the handle read-only so mutations return ReadOnlyMode), and THEORY.md already documented it as live. Corrected the README to match the code and its sibling doc.

Not in this PR

The 13 DESIGN findings and 45 SMELL/NIT from the same review are recorded in the companion review PR (review file + ISSUES.md triage) for separate triage, not fixed here.

Verification

cargo build, full cargo test (incl. the new forge test), cargo clippy --workspace -- -D warnings, and cargo fmt --check all clean. No behavior change on the plaintext path; the I143 change is guarded by every existing reopen test (data preserved ⇒ open_existing chosen).

…–I146)

I143 — TOCTOU create-vs-open race (src/lib.rs): the create_new vs
open_existing decision was made from an unlocked path.exists() stat taken
BEFORE the flock. A process racing open() could stat an empty file while
another creates + commits + releases the lock, then run create_new over the
committed data. Decide from io.page_count() (cached from the post-lock file
length) instead; the pre-lock stat is kept only for the create_if_missing
gate, which must stay pre-lock so a refused open never materializes a file.

I144 — unvalidated crypto-header stride (src/transaction/recovery.rs): the
plaintext stride (guarded only by the forgeable XXH3 checksum) went verbatim
into set_stride, which computes file_len / stride. A forged stride=0 was a
division-by-zero panic (violating poison-not-panic); a huge value drove
multi-GiB allocations. Require stride == ENC_PAGE_SIZE before set_stride;
mismatch returns CorruptSuperblock. Regression test forges the on-disk stride
to 0, re-stamps the checksum, and asserts the reopen errors, not panics.

I145 — reclaim_freemap_orphans not poison-wrapped (src/transaction/freemap.rs):
a fatal error mid-sweep returned un-poisoned, and reclaim_orphans writes the
partially-advanced freemap root back into current_roots even on its error path.
Wrap the sweep in poison_on_fatal like every other TM entry point.

I146 — README contradicted the code (README.md): the format-compatibility
section said the newer-minor write-refusal gate was "not yet wired up"; it
shipped as I29 and THEORY.md documents it as live. Corrected to match.

Full test suite (incl. the new forge test), clippy --workspace -D warnings,
and fmt --check all clean.
@Xof
Xof merged commit 04534c0 into main Jul 2, 2026
10 checks passed
@Xof
Xof deleted the fix/deepdive-20260702-bugs branch July 2, 2026 01:28
Xof added a commit that referenced this pull request Jul 2, 2026
…ES.md (#90)

Adds the fresh-eyes review report (docs/reviews/review-20260702-001902.md)
and triages its verified findings as I143–I160 under a new "Deep review
2026-07-02" section in ISSUES.md.

- 18 findings confirmed by adversarial verification (0 refuted): 4 BUG,
  13 DESIGN, 1 SMELL. Prior-review delta: 0 regressions, 71 resolved.
- I143–I146 (the BUGs) are marked FIXED (PR #89).
- I147–I160 (DESIGN/SMELL) recorded OPEN for triage — corrupt-page
  hardening asymmetry, KDF-param DoS, per-page replay, the encrypted-minor
  write-gate off-by-series, Python finished-transaction aliasing, and the
  encrypted on-disk-spillway test gap are the notable ones.

Docs only; no code change.
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

🚦 Bench results: PR vs main

⚠️ 4 regression(s) detected across 3 scenario/mode pair(s)

Scenario Mode Δ throughput Worst Δ
ycsb-b redb-strict +0.0% p50 +12.3% ⚠️
ycsb-a chisel-mem -5.8% p50 +6.3% ⚠️
ycsb-b chisel-strict +4.2% p50 +5.5% ⚠️
document-store chisel-mem -2.4%
document-store chisel-strict +3.3%
document-store redb-strict +3.3%
document-store sqlite-strict +0.3%
mutation-log chisel-mem +1.7%
mutation-log chisel-strict +2.5%
mutation-log redb-strict +0.1%
mutation-log sqlite-strict +2.8%
ycsb-a chisel-strict +4.3%
ycsb-a redb-strict +0.0%
ycsb-a sqlite-strict -1.1%
ycsb-b chisel-mem -2.0%
ycsb-b sqlite-strict -1.0%
Per-scenario detail (4 metrics × cells)

document-store

Mode Throughput p50 p95 p99
chisel-mem 34076 ops/s → 33249 ops/s (-2.4%) 5.0 µs → 5.1 µs (+0.2%) 65.8 µs → 66.6 µs (+1.2%) 292.5 µs → 295.5 µs (+1.0%)
chisel-strict 2807 ops/s → 2901 ops/s (+3.3%) 12.0 µs → 12.1 µs (+0.3%) 1.07 ms → 1.02 ms (-4.2%) 2.56 ms → 2.58 ms (+0.7%)
redb-strict 3995 ops/s → 4127 ops/s (+3.3%) 18.0 µs → 18.2 µs (+1.0%) 646.0 µs → 598.9 µs (-7.3%) 1.65 ms → 1.58 ms (-4.0%)
sqlite-strict 5109 ops/s → 5126 ops/s (+0.3%) 20.0 µs → 20.4 µs (+2.3%) 419.8 µs → 438.9 µs (+4.6%) 1.36 ms → 1.46 ms (+7.3%)

mutation-log

Mode Throughput p50 p95 p99
chisel-mem 54149 ops/s → 55050 ops/s (+1.7%) 23.2 µs → 22.9 µs (-1.6%) 26.2 µs → 25.7 µs (-1.7%) 35.7 µs → 35.1 µs (-1.5%)
chisel-strict 1659 ops/s → 1701 ops/s (+2.5%) 434.0 µs → 418.0 µs (-3.7%) 928.6 µs → 848.5 µs (-8.6%) 9.66 ms → 10.13 ms (+4.9%)
redb-strict 1805 ops/s → 1807 ops/s (+0.1%) 215.1 µs → 212.9 µs (-1.0%) 330.6 µs → 308.6 µs (-6.7%) 29.04 ms → 29.63 ms (+2.0%)
sqlite-strict 4721 ops/s → 4851 ops/s (+2.8%) 125.4 µs → 123.4 µs (-1.6%) 346.3 µs → 335.4 µs (-3.1%) 539.2 µs → 471.5 µs (-12.6%)

ycsb-a

Mode Throughput p50 p95 p99
chisel-mem 43424 ops/s → 40913 ops/s (-5.8%) ⚠️ 38.7 µs → 41.1 µs (+6.3%) ⚠️ 53.1 µs → 56.5 µs (+6.4%) 69.8 µs → 72.3 µs (+3.6%)
chisel-strict 2123 ops/s → 2214 ops/s (+4.3%) 338.2 µs → 341.0 µs (+0.8%) 973.1 µs → 931.5 µs (-4.3%) 3.09 ms → 2.63 ms (-15.0%)
redb-strict 2681 ops/s → 2682 ops/s (+0.0%) 188.8 µs → 187.8 µs (-0.6%) 275.2 µs → 271.5 µs (-1.3%) 591.3 µs → 519.4 µs (-12.2%)
sqlite-strict 150764 ops/s → 149061 ops/s (-1.1%) 6.8 µs → 6.8 µs (-0.2%) 8.6 µs → 8.7 µs (+1.9%) 10.5 µs → 10.4 µs (-1.5%)

ycsb-b

Mode Throughput p50 p95 p99
chisel-mem 293926 ops/s → 288081 ops/s (-2.0%) 1.4 µs → 1.4 µs (+3.7%) 40.1 µs → 40.9 µs (+1.9%) 43.7 µs → 44.4 µs (+1.6%)
chisel-strict 19985 ops/s → 20833 ops/s (+4.2%) 2.5 µs → 2.7 µs (+5.5%) ⚠️ 350.2 µs → 345.1 µs (-1.5%) 902.3 µs → 849.7 µs (-5.8%)
redb-strict 27099 ops/s → 27104 ops/s (+0.0%) 2.5 µs → 2.8 µs (+12.3%) ⚠️ 183.3 µs → 187.7 µs (+2.4%) 245.7 µs → 257.0 µs (+4.6%)
sqlite-strict 184382 ops/s → 182523 ops/s (-1.0%) 5.4 µs → 5.5 µs (+0.8%) 7.3 µs → 7.4 µs (+1.1%) 9.5 µs → 9.6 µs (+1.8%)
Generated by chisel-bench-diff at 2026-07-02T01:39:41Z. Compares PR HEAD against main. Never blocks merge — signal, not gate. Thresholds: throughput 5%, p50 5%, p95 10%, p99 10%.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant