Repository navigation
perf(aead-frame): write the storage profile three times faster and read it a third faster, with every blob byte-for-byte the same - #559
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
✅ No CRAP threshold violations1520 function(s) analyzed · threshold 30 |
🧬 Mutation testing (cargo-mutants,
|
| caught | missed | unviable | timeout |
|---|---|---|---|
| 95 | 0 | 31 | 0 |
✅ Every mutant in the changed lines was caught by a test.
Code review: standards and specTwo-axis review of c2664d7..6a8791d, run as two separate reviewers so neither masks the other. Line numbers are at 6a8791d. StandardsDocumented-standard breaches
Smells (judgement calls)
Correctness
SpecChecked against PR #534's findings and the agreed design (frozen bytes, Missing or partial
Not asked for
Implemented and checked
Summary. Standards: 3 breaches, 5 smells, 1 correctness defect; worst is the Kani unwind bound. Spec: 1 partial, 4 scope items, 1 unflagged behaviour change; worst is the body's "proofs pass unchanged" claim. |
Zeroize audit of this branch (c2664d7...1a5e35c)Run with the zeroize-audit skill over the branch's diff across vitaminc-protected, vitaminc-aead and vitaminc-aead-frame: source analysis, LLVM IR at O0 to O3, release assembly (AArch64, read by hand), five hand-written proof-of-concept programs run under a watching allocator and independently verified. Headline: every wipe this branch added or changed survives optimisation; one medium finding (ZA-0001) is a documentation guarantee that the Run ID: Configuration:
Executive SummaryVerdict. Every wipe this branch added or changed survives optimization.
By Severity
By Confidence
PoC Validation
Two of the four exploitable PoCs (ZA-0002 and ZA-0003) confirm behaviour the code documents on purpose, so they raise confidence in the observation, not the severity. By Category
By Status
Status is kept in Sensitive Objects Inventory29 objects, SO-5000 to SO-5028, found by reading the diff and its callers.
FindingsMedium SeverityZA-0001: SECRET_COPY, medium (confirmed, open)Location: Evidence:
Confidence: one source signal plus a verified exploitable PoC: confirmed (the MCP downgrade to needs_review was applied first, then the PoC lifted it). Recommended Fix: two options.
Low Severity, ConfirmedZA-0002: SECRET_COPY, low (confirmed, accepted)Location: Evidence:
Recommended Fix: None required. Keep callers to the move-into-a-wiping-type pattern. A clippy ZA-0003: SECRET_COPY, low (confirmed, accepted)Location: Evidence:
Recommended Fix: Optional. Build ZA-0004: SECRET_COPY, low (confirmed, open)Location: Evidence:
Recommended Fix: Make the ZA-0008: STACK_RETENTION, low (confirmed, accepted)Location: Evidence:
Recommended Fix: None needed; accepted category. A complete fix would require wiping compiler-managed temporaries, which Rust source cannot reach. ZA-0009: STACK_RETENTION, low (confirmed, accepted)Location: Evidence:
Recommended Fix: None needed; accepted category. ZA-0010: STACK_RETENTION, low (confirmed, accepted)Location: Evidence:
Recommended Fix: None needed; accepted category (core's sort temporaries cannot be reached from Rust). If tightened, sort indices or Low Severity, LikelyZA-0011: REGISTER_SPILL, low (likely, accepted)Location: Evidence:
Recommended Fix: None; accepted category (STORAGE.md: copies in registers are not wiped). No source-level fix reaches callee-save slots. Low Severity, Needs ReviewZA-0005: SECRET_COPY, low (needs_review, accepted)Location: Evidence:
Recommended Fix: No source change; the repo's position is that wiping the one named copy would overclaim. ZA-0006: SECRET_COPY, low (needs_review, accepted)Location: Evidence:
Recommended Fix: None required. std's ZA-0007: NOT_ON_ALL_PATHS, low (needs_review, open)Location: Evidence:
Recommended Fix: No Rust code change. Note in ZA-0012: NOT_ON_ALL_PATHS, low (needs_review, open; hardening, not a defect)Location: Evidence:
Recommended Fix: Hardening. After Verified CorrectThese are the wipes the branch added or changed that were checked in IR and assembly and found sound. They matter as much as the findings. All IR below is from 1.
|
| Finding | Category | PoC File | Exit Code | Result | Verified | Impact |
|---|---|---|---|---|---|---|
| ZA-0001 | SECRET_COPY | poc/poc_za_0001/src/main.rs |
0 | exploitable | yes | needs_review to confirmed; medium kept |
| ZA-0002 | SECRET_COPY | poc/poc_za_0002/src/main.rs |
0 | exploitable (by design) | yes | needs_review to confirmed; low, accepted |
| ZA-0003 | SECRET_COPY | poc/poc_za_0003/src/main.rs |
0 | exploitable (by design) | yes | needs_review to confirmed; low, accepted |
| ZA-0004 | SECRET_COPY | poc/poc_za_0004/src/main.rs |
0 | exploitable | yes | needs_review to confirmed; low, open |
| ZA-0005 | SECRET_COPY | none | n/a | no_poc | n/a | stack and register copies cannot be observed from Rust without undefined behaviour |
| ZA-0006 | SECRET_COPY | poc/poc_za_0006/src/main.rs |
1 | not_exploitable | yes | severity low (it already was); confidence stays needs_review; finding kept |
| ZA-0007 | NOT_ON_ALL_PATHS | none | n/a | no_poc | n/a | category not supported for Rust PoCs |
| ZA-0008 to ZA-0010 | STACK_RETENTION | none | n/a | no_poc | n/a | already confirmed by IR and assembly |
| ZA-0011 | REGISTER_SPILL | none | n/a | no_poc | n/a | stays likely (assembly only) |
| ZA-0012 | NOT_ON_ALL_PATHS | none | n/a | no_poc | n/a | dead by the capacity invariant; nothing to trigger |
Limits that apply to the exploitable results: each PoC models Vec growth as a moving realloc (GlobalAlloc's default allocate, copy, free). A system allocator that grows a block in place would leave no freed old block, but the caller does not control that. The ZA-0001 PoC exercises update only, and the ZA-0004 PoC exercises write only.
Superseded Findings
No findings were superseded in this run.
Three pairs of analyzer findings were merged into one finding each, because the IR and the assembly finding describe the same function and add up to two independent signals. This is a merge, not a supersession.
| Merged | Into |
|---|---|
| F-RUST-IR-0001 + F-RUST-ASM-0001 | ZA-0008 |
| F-RUST-IR-0002 + F-RUST-ASM-0002 | ZA-0009 |
| F-RUST-IR-0003 + F-RUST-ASM-0003 | ZA-0010 |
Source and analyzer ids map to report ids as follows: F-RUST-SRC-0001 to 0007 are ZA-0001 to ZA-0007, F-RUST-ASM-0004 is ZA-0011, and the Phase 2 observation about the Vec growth fallback is ZA-0012.
Confidence Gate Summary
| Finding | Action | Reason |
|---|---|---|
| ZA-0001 (SECRET_COPY) | Held at needs_review in the interim report, then upgraded to confirmed | MCP unavailable (mcp_mode=off), so SECRET_COPY was downgraded from likely to needs_review. A verified exploitable PoC is a second signal alongside the source-traced mechanism. |
| ZA-0002 (SECRET_COPY) | Downgraded to needs_review, then upgraded to confirmed | Same rule. The PoC confirms documented behaviour; status stays accepted. |
| ZA-0003 (SECRET_COPY) | Downgraded to needs_review, then upgraded to confirmed | Same rule. Status stays accepted. |
| ZA-0004 (SECRET_COPY) | Held at needs_review, then upgraded to confirmed | Analyzer confidence was needs_review; the verified exploitable PoC adds a second signal. Status stays open. |
| ZA-0005 (SECRET_COPY) | Held at needs_review | Source-only signal; the compiler evidence is carried by ZA-0008 to ZA-0011. No PoC possible. |
| ZA-0006 (SECRET_COPY) | Held at needs_review; severity low | The verified PoC returned not exploitable (exit 1), which downgrades severity to low (already low) and keeps the finding. |
| ZA-0007, ZA-0012 (NOT_ON_ALL_PATHS) | Held at needs_review | Heuristic and invariant-based; no PoC is supported for the category. |
| ZA-0008 to ZA-0010 (STACK_RETENTION) | Confirmed | IR (alloca with lifetime.end and no volatile store) plus assembly (secret bytes on the stack at ret): two signals and both hard evidence requirements met. |
| ZA-0011 (REGISTER_SPILL) | Likely | Assembly spill instruction shown (hard requirement met); only one kind of signal. |
The OPTIMIZED_AWAY_ZEROIZE hard-evidence rule (an IR diff showing a wipe at O0 and none at O1 or O2) found nothing: no function loses its volatile stores at any level. No override was rejected.
Analysis Coverage
| Metric | Value |
|---|---|
| Crates analyzed | 3 of 3 in scope (vitaminc-protected, vitaminc-aead, vitaminc-aead-frame with --features serde) |
| TUs in compile DB | n/a (no compile DB; one cargo workspace TU, hash 10e312b4) |
| TUs analyzed | 1 (3 of 3 packages succeeded) |
| TUs with sensitive objects | 1 |
| Agent 1 (MCP resolver) | skipped (mcp_mode=off) |
| Agent 2 (source analyzer) | success: 29 objects, 7 findings, 15 dangerous-API hits (14 false positives, 1 folded into ZA-0002), 0 semantic findings |
| Agent 3 (compiler analyzer) | success: 3 of 3 packages for MIR (O0, O2), IR (O0 to O3), asm (O2, O3), plus an out-of-repo harness at release O0 and O3; 555 raw script findings dismissed with reasons in script-triage.json, 7 hand-verified findings retained |
| Agent 4 (report assembler) | success (interim and final) |
| Agent 5 (PoC generator) | success: 5 PoCs written and built; 7 findings with no supported category |
| PoC verifier | success: 5 of 5 verified, exit codes reproduced |
| Agent 6 (test generator) | skipped (enable_runtime_tests=false) |
What ran: rustdoc JSON for each crate; semantic_audit.py; find_dangerous_apis.py; MIR, IR and assembly emission; check_mir_patterns.py, check_llvm_patterns.py, check_rust_asm.py; the MIR diff; semantic IR analysis of wipe_raw, grow and Drop::drop; the five PoCs.
What was skipped or not established:
validate_rust_toolchain.shwas not run: it fails on the macOS bash 3 (declare -A). The tools and scripts were verified by hand withtest -xandtest -f, andcargo check --workspace --all-targets --features vitaminc-aead-frame/serdesucceeded.- MCP was off, so SECRET_COPY findings carry the downgrade described above.
- CFG analysis was disabled, so no NOT_DOMINATING_EXITS or MISSING_ON_ERROR_PATH finding could be produced; the error-path claims rest on IR inspection of the unwind and
Errblocks instead. - Runtime tests (phase 7) were not generated.
- The emit scripts build with the dev profile and apply
-C opt-level=Nto the audited crate only, so its dependencies are built at dev O0 with debug assertions on. The out-of-repo harness was a true release build (dependencies at O3) and showed the sameBuffercode shape, so no difference is expected, but the three crates' own release-profile IR was not emitted. - All assembly is AArch64 (the host). The AArch64 path is experimental, and every assembly claim in this report was read by hand. x86-64 assembly was not produced.
- LTO builds were not checked. Volatile stores and the fence are preserved by LTO, but this was not observed.
panic=abortbuilds were not compiled; ZA-0007's abort half is true by construction.
Two script bugs found along the way (not in the repository under audit):
check_mir_patterns.pyandcheck_rust_asm.pycrash onsource-analysis/sensitive-objects.json, because it is a dict with anobjectskey and they expect a list. Workaround:secrets-list.json, theobjectsarray on its own.- The emit scripts concatenate two emitted files, so every function appears twice in each
.lland.sfile. Workaround: read the first copy only.
Script noise: 555 raw findings were dismissed. 222 IR "non-volatile memset" hits are zero-initialisation (Zeroed::zeroed, hashbrown control bytes, SipHash state, SeenKeys::default, typed-null padding). 7 IR "volatile stores dropped" hits key on SSA names that change with inlining; a per-function comparison across all four levels found no function that loses its wipe. 37 MIR hits are locals named key or at that are borrowed &str. 289 assembly hits are prologue saves in functions that hold no secret.
Appendix: Evidence Files
Paths are relative to /tmp/zeroize-audit-20261028115220/.
| Finding | Evidence File | Description |
|---|---|---|
| ZA-0001 | poc/poc_za_0001/src/main.rs; source-analysis/source-findings.json (F-RUST-SRC-0001) |
Growth through Controlled::update; source trace |
| ZA-0002 | poc/poc_za_0002/src/main.rs; source-analysis/rust-dangerous-api-findings.vitaminc_protected.json |
risky_unwrap PoC; mem::take hit |
| ZA-0003 | poc/poc_za_0003/src/main.rs |
Passthrough copy PoC |
| ZA-0004 | poc/poc_za_0004/src/main.rs; rust-compiler-analysis/aead.O3.leaf_write_Vec.s |
leaf::write into Vec::new(); the Vec<u8> monomorph |
| ZA-0005 | source-analysis/source-findings.json (F-RUST-SRC-0005) |
Source-level list of stack and register copies |
| ZA-0006 | poc/poc_za_0006/src/main.rs; rust-compiler-analysis/vitaminc-aead.O3.ll |
SeenKeys table PoC; drop_glue::<SeenKeys> |
| ZA-0007 | rust-compiler-analysis/harness.O3.storage_to_vec_u64.ll; rust-compiler-analysis/harness.O3.serde_to_vec.ll |
Unwind and Err paths that drop the Buffer |
| ZA-0008 | rust-compiler-analysis/vitaminc-aead.O3.ll, aead.O3.leaf_write_Buffer.ll, vitaminc-aead.O3.s, aead.O3.leaf_write_Buffer.s, aead.O3.leaf_write_Vec.s, vitaminc-aead.O2.s; ir-findings.json (F-RUST-IR-0001), asm-findings.json (F-RUST-ASM-0001) |
leaf::write IR and assembly |
| ZA-0009 | rust-compiler-analysis/vitaminc-aead-frame.O3.ll, frame.O3.distinct.ll, vitaminc-aead-frame.O3.s, frame.O3.distinct.s, vitaminc-aead-frame.O2.s; ir-findings.json (F-RUST-IR-0002), asm-findings.json (F-RUST-ASM-0002) |
distinct IR and assembly |
| ZA-0010 | rust-compiler-analysis/vitaminc-aead-frame.O3.ll, vitaminc-aead-frame.O3.s; ir-findings.json (F-RUST-IR-0003), asm-findings.json (F-RUST-ASM-0003) |
Sort temporary IR and assembly |
| ZA-0011 | rust-compiler-analysis/vitaminc-aead.O3.s, vitaminc-aead-frame.O3.s, vitaminc-protected.O3.s; asm-findings.json (F-RUST-ASM-0004) |
Callee-saved spill assembly |
| ZA-0012 | rust-compiler-analysis/vitaminc-aead.O3.ll, vitaminc-aead-frame.O3.s, notes.md (Observations) |
Vec growth fallback after grow |
| Verified wipes | rust-compiler-analysis/vitaminc-protected.{O0,O1,O2,O3}.ll, vitaminc-protected.{O2,O3}.s, vitaminc-protected.{O0,O2}.mir, notes.md |
wipe_raw, grow, Drop, zeroize |
| All | report/findings.json, report/finding-metadata.json, report/findings.interim.json, report/merge_poc.py; poc/poc_final_results.json, poc/poc_verification.json, poc/poc_manifest.json; rust-compiler-analysis/script-triage.json; source-analysis/rust-notes.md; preflight.json, merged-config.yaml |
Structured results, triage and run configuration |
Zeroize audit, part two: x86-64 and fat LTOThe audit above ran on the AArch64 host without LTO, on library builds. This pass repeats its wipe checks on Result: every wipe survives on every target and with LTO. No wipe became a
One new finding, low, in the category STORAGE.md already accepts. On x86-64, Also checked: the Limits: the nightly toolchain has no x86-64 darwin std, so the x86 and LTO builds used stable 1.98.1 where the host pass used nightly; Linux x86-64 covered |
7a5a1c4 to
fbe2f5d
Compare
d269464 to
bb5ce22
Compare
cipherstash-bot
left a comment
There was a problem hiding this comment.
Recommendation: 🟡 merge after changes
One item must change before merge: add tests for the two new debug checks on the Vec<u8> sink in leaf.rs, and document the new panic. The other items can wait for a follow-up or are optional: the MapWriter length-hint allocation, the missing storage::to_vec test, and the SeenKeys visibility note. The unsafe code looks sound.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | test-gap | 3 found, 3 posted |
| claude | claude-opus-5-5 | rust | 3 found, 2 posted |
| codex | gpt-5.6-terra | test-gap | 1 found, 1 posted |
| codex | gpt-5.6-terra | rust | 0 found, 0 posted |
Synthesis: claude-sonnet-5-5 merged the findings, removed duplicates and dropped findings it could not confirm in the code. 1 posted finding(s) were raised by two or more models.
Plain language: claude-sonnet-5-5 read every comment as a new reader would. 3 comment(s) had a problem that stopped the reader acting; it rewrote 3. It also rewrote the review body.
Stack: position 1 of 2 (:asterisk: #559, #560). *️⃣ marks this pull request.
Context loaded: the description, 1 linked issue(s) and 6 discussion entries.
Findings not posted:
- Fix in a follow-up:
storage::to_vecsizesMapWriter::keysfrom the length hint of theSerializeimpl, so a wrong hint can panic or abort the process. Already raised in this thread.
… so no blob is read back before it is returned The storage writer read every blob back before returning it, to find a repeated key as a reader would. That pass rebuilt a HashSet for every object and hashed every field name, and was 55% of a 1,000-record write. An object now compares each field name with the ones it has written, held on the stack and spilling to the heap past 16, since the names are the type's own and never data. A map records where each key lies in the blob and checks them for a repeat when it ends, as before, without parsing the rest. A debug build still reads the blob back, so the tests keep checking that the writer and reader agree. On an M4 on mains power this made a 1,000-record write 47% faster, one record 41% and a map of 1,000 keys 29%, and left reads unchanged.
…ob is not wiped a byte at a time zeroize stores one byte per volatile write, so wiping a 64 KiB buffer took 15 µs, and a frame writer that regrows its buffer paid that for every old allocation it wiped. wipe_raw, which the locked regions use, now stores a u128 at a time between a byte-wise head and tail, and falls back to bytes when the pointer cannot be aligned. The stores stay volatile, so the guarantee is unchanged: only the width is. A safe wipe_vec wraps it for a Vec<u8>'s whole allocation, and both frame writers wipe their outgrown buffers with it. The Kani harness is widened to cover the word run, and two tests pin the bounds from every alignment; both pass under Miri's default and strict models. On an M4 on mains this took 17% off a 1,000-record storage write and 10% off the transport write. The drop of the returned buffer still goes through zeroize, which the next change addresses.
…xt they hold is wiped sixteen bytes a store A frame or blob came back as Protected<Vec<u8>>, and the drop of a Vec<u8> goes through zeroize, which stores one byte per volatile write: 15 µs for a 64 KiB value, and a quarter of a 1,000-record write. That wipe cannot be changed from here without specialisation, so the type is. Buffer, in vitaminc-protected, is a Vec<u8> that grows only through push, extend_from_slice and reserve, which copy into a fresh allocation and wipe the old one, and whose zeroize and drop wipe it with wipe_vec. It derefs to a slice for reading and patching, has no Clone, PartialEq or Debug since its bytes are secret, and risky_into_vec is the one way out. to_frame, decrypt_to_frame and both to_vecs return Protected<Buffer> and their writers hold &mut Buffer, so a writer cannot grow its buffer unwiped; the two private reserve copies are gone. leaf::write takes either buffer through an Out trait, so the sealed-leaf path is unchanged, and a passthrough frame converts back to a Vec at its boundary since it crosses in the clear. BREAKING: the four writers return Protected<Buffer>, not Protected<Vec<u8>>; FrameWriter and leaf::write take a Buffer; a caller that borrowed a slice is unchanged, one that took the Vec calls risky_into_vec. Two findings shaped the hot path. Buffer's methods are #[inline], as Vec's are, or structured writes lose up to 24% to a call per byte; but leaf::write stays out of line, since inlined into the storage writer it cost 10 to 15%. And distinct's scratch for integer keys, presized to the key count, is a Buffer too: as a Zeroizing<Vec<[u8; 17]>> it wiped 17 KB of untouched spare capacity a byte at a time for a map of 1,000 text keys, 8 µs of a 38 µs write. On an M4 on mains, against the previous commit: a 1,000-record storage write 205 to 153 µs, a map of 1,000 keys 38.1 to 31.9 µs, one record 0.263 to 0.208 µs, and a 64 KiB value 17 to 1.9 µs. Transport writes gained 10 to 17%. Reads are unchanged.
… read through risky_ref and not a plain slice A Buffer dereferenced to a slice, so a frame written into one could be read without a risky word anywhere, out of step with Protected and Locked in the same crate. Buffer now implements Controlled with a Vec<u8> inside: reads are risky_ref, taking the Vec is risky_unwrap, and a length is filled in through overwrite(at, bytes), which hands out no slice. Its Debug is redacted. The frame and blob writers return a bare Buffer. It already wipes on drop, so the Protected around it only added a second risky layer. BREAKING: to_frame, decrypt_to_frame, serde::to_vec and storage::to_vec return Buffer, not Protected<Buffer>. Buffer no longer implements Deref, DerefMut or AsRef<[u8]>; risky_into_vec is risky_unwrap. Measured on mains, three alternating rounds against the previous commit: every write within 3%, which is the run-to-run spread.
…laced at it, so reading or writing a field allocates nothing The map and enum access held each entry's path segment ready while its value was read, and the transport writer did the same for each struct field and variant part. That segment owns a String copied from the type's own static name, so a struct of seven fields cost seven allocations and frees per read and per write that nothing used unless an error came back. The entry's place is now a Copy value holding the static name, and becomes a path segment only when an error is placed at it. Measured on mains, three alternating rounds against the previous commit: records read 9% to 10% faster in both profiles, a single record 16% to 18%, and the transport write of records 37% faster.
… value is decoded once on read and not twice A string, bytes or map read, and an Option read, first asked what kind of node came next. That answer was found by decoding the whole scalar, slicing its payload and checking its UTF-8, and then thrown away before the same scalar was decoded again for the visitor. The kind is in the tag byte, so it is read from there, and a tag the validator would not have passed is reported as malformed, as the decode reported it. Measured on mains, three alternating rounds against the previous commit: records read 14% faster in both profiles, a 64 KiB text 26%.
…so validating a record allocates no hash set Both validators, the frame's and the storage profile's, built a HashSet per object to refuse a repeated key: an allocation and a SipHash per field for objects that mostly have a handful. SeenKeys keeps the first sixteen keys inline and compares them directly, and hashes only once an object has more, so a host object with thousands of keys is still checked in linear time. The keys are borrowed from the frame, so nothing is copied. Measured on mains, three alternating rounds against the previous commit: records read 18% faster in the storage profile and 23% in the transport; writes unchanged.
…h its 64-byte check loop
…nner_mut reaches its bytes, so neither can be removed unnoticed
…g, bytes or a struct are refused by their kind
…to_vec no longer returns it Protected
…e test helper, so the text reads as the rest of the file
…r and reader changes, rather than promise a re-measure
…r calls it and a public Vec wipe invites unwrapped secrets
…o writer uses them and each is more surface to review
…eSink, so it is not an open extension point named Out
…ince the Controlled methods that lend the Vec can grow it unwiped
… so the guarantee is tested at the type that owns it
…max_len, and show presized wiping sinks in the examples, since a growing Vec frees plaintext unwiped
… under panic=abort leaves a partly written blob unwiped
…allocating growth is compiled out of Buffer's push paths After grow, which the compiler does not look into, Vec's own growth code stayed compiled into push and extend_from_slice in every writer. It could never run, because grow always makes room, but if it ever did it would realloc and free the old allocation unwiped. An assert_unchecked on the spare capacity after reserve lets the compiler drop it: every function that called grow also called RawVec::grow_one or do_reserve_and_handle before (8 of 10 in vitaminc-aead, 6 of 21 in vitaminc-aead-frame) and none does now, with or without fat LTO. A debug_assert of the same condition checks it in debug and Miri builds. Measured: the storage writer is unchanged; the transport map_1000 write is about 6% slower, accepted for the hardening.
…r panic, so removing one fails a test and a caller with an unsized Vec knows why it panics
…ocate the room, since the room it then promises the compiler rests on that panic
…-frame uses it and it should not read as an API to keep stable
bb5ce22 to
efac5e4
Compare
|
Rebased onto main (now at efac5e4), with the review fixes in the threads above. CI is green. On the review summary's other items:
|
… it is shared with vitaminc-aead-frame and is not an API
642ecea to
bd45c10
Compare
Closes #563. Stacked on #534.
Summary
Seven changes to where the storage and transport profiles spend their time, each measured on its own before it was kept. Together they make a 1,000-record storage write 3.1x faster and read 1.6x faster. The storage profile's bytes and version 1 rules are untouched: every blob written before and after is the same.
packages/protected/src/ +├── buffer.rs # Buffer: a growable byte buffer that wipes as it grows and when dropped, read through risky_ref └── locked/mod.rs # wipe_raw stores 16 bytes at a time; a crate-private wipe_vec wraps it for Buffer packages/aead/src/ ├── frame.rs # FrameWriter writes into a Buffer; SeenKeys finds a repeated key with no hash set └── leaf.rs # write() is generic over the sealed ByteSink (Vec<u8> or Buffer), kept out of line by measurement packages/aead-frame/src/ ├── storage.rs # refuses a repeated key as it writes; returns a Buffer; SeenKeys in validate └── serde.rs # returns a Buffer; no allocation per field; classifies a node by its tagWhere the time went, and what each commit does about it:
wipe_raw, the locked regions' Kani-proved wipe, stores sixteen bytes at a time; frames and blobs wipe their old allocation through it as they grow.Vec<u8>impl.Bufferowns the bytes a frame or blob is built in and wipes them word-wise on drop.to_frame,decrypt_to_frame,serde::to_vecandstorage::to_vecreturn it.Bufferis aControlledtype, not aDerefto[u8], so its plaintext is read throughrisky_reflikeLockedandProtected. Measured at no cost.Stringper field to hold an error path it almost never placed. ACopyplaceholder becomes a path segment only when an error lands on it. The transport writer had the same allocation.SeenKeyscompares up to sixteen keys inline and spills to a set past that, so a host object with thousands of keys stays linear.Two inlining experiments (
#[inline]onleaf::max_lenand on theFrameWriter's small methods) were measured and reverted: the first changed nothing and the second made the storage writer 11 to 21% slower.leaf::writecarries#[inline(never)]with its measurement in the doc.Evidence
All numbers are from
cargo bench -p vitaminc-aead-frame --features serde --bench storageon the machine in #534 (Apple M4, macOS 26.5, rustc 1.98.1), on mains power. Each step was measured as an A/B/A/B of two bench binaries built side by side, the commit before it against the commit, three alternating rounds, median of three. Same-binary spread on this machine is 1 to 3%.Before is #534's tip, after is this branch. Both are mains runs; they are from different sessions, so compare them to within a few percent.
Time to write:
Time to read:
Against MessagePack, which wipes nothing: a 1,000-record storage write is 2.1x (was 6.7x), a 64 KiB write 1.7x (was 16x), a 1,000-record read 2.6x (was 4.0x), a 64 KiB text read 1.5x (the second UTF-8 pass, which the two-pass design keeps), a 64 KiB bytes read 1.0x.
What each step moved, on the 1,000-record bench unless noted:
BufferBufferisControlledSeenKeysin both validatorsStep 3's A/B also found that
distinct, the map's key check from step 1, wiped 17 KB of untouched spare capacity a byte at a time on every map write; its scratch is aBuffernow.One behaviour change in the writer:
MapWriter::endnow treats a malformed key identity asunreachable!where the old read-back path returned anUnwritableerror.MapKey::identitycannot fail, so no input reaches it; a writer bug there would now panic rather than error.A review of the branch (two reviewers, standards and spec, posted below) led to nine follow-up commits: the Kani unwind bound for the widened
wipe_rawharness, tests for the mutants CI found surviving (aBuffer's drop wipes its allocation,risky_inner_mutreaches its bytes, a null, bool or float read where a string, bytes or struct is expected is refused by its kind), STORAGE.md and ADR 0003 amended for theBufferreturn,wipe_vecmade crate-private,Buffer::clearandcapacitydropped, and the scalar writer's output trait sealed and namedByteSink. Unifying the writer'sFieldNameswithSeenKeyswas tried and reverted: the map write slowed 3% in an A/B, and the two differ in what they hold.A zeroize audit of the branch (two comments below: source, LLVM IR at O0 to O3 and assembly on AArch64; then x86-64 and fat LTO) found every wipe intact on every target and five open items, now five more commits:
Buffer's doc no longer claims more than its own methods deliver (theControlledmethods that lend theVeccan grow it unwiped, which #496'srisky_updaterenames narrow further); tests that growing aBufferbypushorextend_from_slicefrees the old block wiped; theVec<u8>leaf sink checks in debug builds that it was sized withmax_len, and the leaf doc examples use presized wiping sinks; STORAGE.md records that the wipes run on drop, sopanic=abortleaves a partly written blob unwiped; andreservetells the compiler it made room, soVec's own reallocating growth is compiled out of everyBufferpush path (storage writer unchanged, transport map write about 6% slower, accepted).Checks on every commit:
cargo fmt --check, workspace clippy with-D warningson all targets, the tests ofvitaminc-protected(the buffer and wipe tests also under Miri in both models),vitaminc-aead,vitaminc-aead-frameandvitaminc-aead-napiwith the node addon,cargo +1.88.0 checkas CI's MSRV job runs it, and docs with-D warnings. After the follow-ups:cargo kani -p vitaminc-protectedverifies all 4 harnesses, and cargo-mutants onbuffer.rs(30 caught of 30) and onfoundinserde.rs(11 caught, 1 unviable).storage::to_veckeeps adebug_assert_eq!that validates its blob, so the tests still check that the writer and reader agree.Merge Danger
Door: two-way for the code, one-way for the API
Nothing on disk changes: the bytes every writer produces are byte-for-byte what they were, and the storage profile's conformance corpus and its Kani proofs pass unchanged. The
wipe_rawproof invitaminc-protectedwas widened to a 64-byte buffer so the word-wise run is covered, and passes. The code can be reverted commit by commit; each stands alone.Three commits are breaking (
!), for a major bump through release-plz:to_frame,decrypt_to_frame,serde::to_vecandstorage::to_vecreturnBufferwhere they returnedProtected<Vec<u8>>. A caller that read the bytes throughrisky_refgets a&Vec<u8>as before; one that wants theVeccallsrisky_unwrap.FrameWriter::newandat_depthtake&mut Buffer, not&mut Vec<u8>.leaf::writeis generic over a new sealedByteSinktrait, implemented forVec<u8>andBuffer.New public items:
vitaminc_protected::Bufferandvitaminc_aead::frame::SeenKeys, the last because the storage validator in another crate needs it.Blast Radius: callers of the four writers
Those callers change one line. The aead-napi addon is updated in this branch. The Go reader and writer are untouched. The Performance tables in
packages/aead-frame/README.mdpredate this branch and say so; they are to be re-measured before the release.