ci(miri): check the unsafe decode path under the interpreter - #1162
Conversation
The `Yokeable`/`StableDeref` impls behind `OwnedNodeRef` transmute lifetimes over borrowed decode output, and `zlib_pool` extends a Vec's length over inflate's uninitialized spare capacity. Neither clippy nor a native test observes an aliasing violation or an uninit read there — the first symptom would be a miscompile. Adds a Miri workflow over wacore-binary (both the portable-SIMD and the scalar decode paths), wacore-appstate and wacore-noise: the pure crates that carry the unsafe and drive it over real protocol payloads. The tokio/SQLite crates stay out; Miri has no epoll or FFI. The zlib fixtures only reach window refill and buffer growth at hundreds of KB to MB, which the interpreter cannot finish in CI time, so they are ignored under `cfg(miri)` and a 1 KB twin keeps `set_len` covered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1mXhXv7K1bxPnCL3cYF2b
📝 WalkthroughWalkthroughAdds a GitHub Actions workflow for selected Rust crates under Miri with pinned tooling and caching. Adds focused ChangesMiri validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| .github/workflows/miri.yml | Introduces a three-leg Miri workflow with isolated cache keys, explicit sysroot setup, and interpreter-oriented configuration. |
| AGENTS.md | Documents when and how contributors should use Miri for changes to unsafe binary-codec paths. |
| wacore/binary/src/node.rs | Adds focused tests for the yoked ownership, lifetime-transform, and zero-copy slicing invariants. |
| wacore/binary/src/token.rs | Excludes a computation-heavy safe lookup test from Miri while leaving native coverage intact. |
| wacore/binary/src/zlib_pool.rs | Adds a small hand-built zlib fixture that reaches pooled inflate under Miri and excludes oversized fixtures only from interpreter runs. |
Reviews (6): Last reviewed commit: "test(binary): interpret the hand-written..." | Re-trigger Greptile
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/miri.yml:
- Line 62: Update the actions/checkout@v6 step in the Miri workflow to set
persist-credentials to false, while preserving the existing read-only
permissions and all other job behavior.
In `@AGENTS.md`:
- Line 23: Update the local Miri guidance in AGENTS.md to match the CI matrix by
documenting runs for wacore-binary with and without default features,
wacore-appstate, and wacore-noise; alternatively, explicitly label the existing
command as a minimal binary-only smoke test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 679aac57-3165-43f3-ac75-933f228778b8
📒 Files selected for processing (3)
.github/workflows/miri.ymlAGENTS.mdwacore/binary/src/zlib_pool.rs
|
First run in, and the gate found something on its first outing — just not in our code.
|
|
Tip For best results, initiate chat on the files or code changes.
Shipping the Thanks for taking ✏️ Learnings added
|
|
Both points land well. The consistency argument on SHA pinning is correct — introducing a second convention mid-repo without a repo-wide rule or Dependabot keeping the pins current would be net-negative. A dedicated PR that covers all six workflows at once is the right call.
On the Miri findings: the |
The first run found real UB, in `inout` 0.2.2 rather than in this workspace: `PaddedInOutBuf::into_out` builds a shared slice from a raw pointer that invalidates the `&mut [u8]` still protected as `cbc::encrypt_padded`'s argument, reached from `RustCryptoProvider::aes_256_cbc_encrypt`. That is the AES-CBC path every appstate record takes, so the leg cannot be green until the dependency is, and per-test `cfg_attr(miri, ignore)` would delete the coverage while appearing to keep it. The blocker is named in the workflow so the criterion for restoring the leg is written down. wacore-noise stays: it passed, AES-GCM not going through the padded path. Also stops the checkout persisting its token, as signal-durability-nightly.yml already does — this job never pushes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1mXhXv7K1bxPnCL3cYF2b
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31fad9e2af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`OwnedNodeRef::new` was constructed in exactly one place: a test inside `#[cfg(feature = "serde")]`. Neither Miri leg enables that feature, so the `Yokeable` transmutes and the `StableDeref` impl — two of the three unsafe sites the gate exists for — were never interpreted. The gate would have reported green over an aliasing regression it was supposed to catch. Both tests are worth having on their own: nothing previously moved an `OwnedNodeRef` after construction, which is the entire `StableDeref` claim, and `slice_bytes`' pointer arithmetic had no coverage at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1mXhXv7K1bxPnCL3cYF2b
`lookup_matches_reference_under_byte_mutation` walks every byte position of every token against all 256 values — ~3M probes, a second natively and minutes under Miri, which is what left the wacore-binary leg still running after twelve. `token.rs` holds no `unsafe` and no raw pointers, so the interpreter has nothing to find there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1mXhXv7K1bxPnCL3cYF2b
📦 Binary size report
.text per crate
Baseline: |
zlib-rs 0.6.6's deflate state frees its buffers from `deflate::end` while a `&mut` into them is still protected, which Miri rejects — so the one zlib test left running under the interpreter died in `flate2`'s Drop, inside the helper that built its fixture rather than in anything this crate runs. Compression is not a path wacore-binary has; inflate is. The fixture is now a hand-built stored deflate block plus its adler32, which keeps `inflate_into_spare`'s `set_len` covered and drops the compressor from the interpreted set entirely. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1mXhXv7K1bxPnCL3cYF2b
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f2e229b73
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #[test] | ||
| fn borrowed_payloads_survive_moving_the_cart() { |
There was a problem hiding this comment.
Exercise the mutable Yokeable transmute under Miri
This test constructs, moves, and reads the yoke, which reaches Yokeable::make and the StableDeref implementation, but it never calls Yoke::with_mut; a repo-wide search finds no other with_mut call. Consequently AttrsRef::transform_mut—the second lifetime transmute advertised by the workflow—remains entirely uninterpreted, so a regression there would still leave every Miri leg green. Add a small mutation-path assertion through owned.inner.with_mut(...) in this module.
AGENTS.md reference: AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
| # nothing in this workspace can make it so. | ||
| - name: wacore-noise | ||
| cache-key: noise | ||
| args: -p wacore-noise --lib |
There was a problem hiding this comment.
Keep the large Noise fixtures off Miri
This command runs every wacore-noise library unit test under the interpreter, including several 200 KiB framing fixtures and the 16 MiB allocations in test_build_edge_routing_preintro_too_large, test_build_handshake_header_with_oversized_routing, and test_encode_frame_too_large; none has #[cfg_attr(miri, ignore)], and these safe size-boundary tests do not cover the unsafe crypto backends that justify this leg. Thus every PR pays Miri's interpretation cost for the same large-fixture class deliberately excluded from the binary legs; ignore these tests under Miri and retain small crypto-path twins where needed.
AGENTS.md reference: AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
`Yoke::with_mut` is called nowhere in the workspace, so `transform_mut` was uninterpreted — and yoking a `NodeRef` would not have reached the `AttrsRef` impl anyway: the derive transmutes the whole struct in one go and never calls a field's methods. The hand-written impl is there to satisfy that bound, which leaves its three transmutes reachable only through a yoke of `AttrsRef` itself. That yoke is what this test builds, borrowing the attribute key and value straight out of the cart so `make`, `transform` and `transform_mut` all carry a real lifetime across. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1mXhXv7K1bxPnCL3cYF2b
|
Two findings from @chatgpt-codex-connector, one taken and one declined on measurement. Taken: the mutable transmute was uninterpreted — and worse than reportedCorrect that
Its compile-time half was already covered: Declined: the large Noise fixturesThe cost argument is reasonable a priori and does not survive measurement. The mechanism explains it: The three zlib-style ignores in this PR were each paid for by a measurement (hundreds of KB to MB of interpreted inflate, and a ~3M-probe table sweep that had a leg still running at twelve minutes). Trimming a four-minute leg on a prediction would be churn in someone else's tests without one. Generated by Claude Code |
Summary
Testing
|
Nothing in CI currently observes the workspace's
unsafe. Clippy does not model aliasing, and a native test only fails if the miscompile has already happened — which is the point at which the bug is hardest to attribute.What there is to check
The workspace's load-bearing
unsafeis concentrated inwacore-binary, on the hot decode path:node.rs—unsafe impl Yokeable for AttrsRef<'static>,make/transform/transform_mutnode.rs—unsafe impl StableDeref for BytesCartBytesderef target stays put while the wrapper moves, which is what letsOwnedNodeRefself-referencezlib_pool.rs—out.set_len(out.len() + produced)producedbytes intospare_capacity_mut(), so that prefix is initializedThe rest of the tree is clean by construction:
wacore-libsignalis#[deny(unsafe_code)], and the only otherunsafein the workspace iscfg(target_arch = "wasm32")Send/Syncimpls plus a counting allocator in an example.Scope
Three legs,
fail-fast: falseso each reports its own verdict. Measured on the final commit:wacore-binary --libwacore-binary --no-default-features --lib--no-default-featuresis the only way to reach the latterwacore-noise --libunsafeof its own, but it drives the zero-copy decode over real Noise frames and pullscurve25519/sha2/AES-GCM, whose unsafe backends this exercisesThey run in parallel, so the gate costs about four minutes of wall clock.
wacore-appstatewas tried and dropped. Miri rejectsinout0.2.2'sPaddedInOutBuf::into_out, which builds a shared slice from a raw pointer that invalidates the&mut [u8]still held protected ascbc::encrypt_padded's argument — reached fromwacore-libsignal'saes_256_cbc_encryptthrough the documented API used the documented way, with two RustCrypto layers in between. That is the AES-CBC path every appstate record takes, so the leg cannot go green until the dependency does, and#[cfg_attr(miri, ignore)]-ing the ~20 tests that reach it would be deleting the leg while pretending to keep it. The blocker is named in the workflow so the criterion for re-adding it is written down. Full trace in this comment.Also out: everything Tokio-backed (Miri has no epoll),
e2e-tests, and--testtargets. The last is not only a cost decision —wacore/binary/tests/*_alloc.rsassert exact allocation counts under a counting global allocator, which is a statement about optimized codegen, not about the interpreter; androundtrip_proptest.rsis fixed at 2048 cases (ProptestConfig::with_caseswins overPROPTEST_CASES) and writes regression files, which isolation forbids. Running that proptest under Miri at a reduced case count would be a genuinely good UB fuzzer for the decoder, but it needs the case count to come from the environment first — worth a follow-up, not worth weakening this gate to get.Decisions in the workflow that are not obvious
components: miri, rust-src.cargo miri setupcompiles the interpreted sysroot fromrust-src; installingmiridoes not pull it in. Both are published for the pinnednightly-2026-06-16onx86_64-unknown-linux-gnu— checked againststatic.rust-lang.org's channel manifest rather than assumed, since Miri is dropped from a nightly whenever it fails to build.cargo-miriworks by installing itself asRUSTC_WRAPPER; sccache wants the same slot.~/.cache/mirigoes throughSwatinem/rust-cache'scache-directoriesinstead — the sysroot restores in ~18s rather than being rebuilt — and each leg gets its own cache key because they interpret different feature sets intotarget/miri.RUSTFLAGS: ""..cargo/config.tomlsets lld,--icf=alland-Zshare-genericsfor this target. Miri never links, so those are dead weight; a set-but-emptyRUSTFLAGSoverrides config rustflags, the same levermain.yml's stable job already pulls.MIRIFLAGS— Stacked Borrows, isolation on. Notably not-Zmiri-strict-provenance:bytesreconstructs its taggedSharedpointer out of an integer, andslice_bytesdoes its own pointer→integer arithmetic, so that flag would fail on sight, on a class of finding this gate is not looking for. Isolation stays on: nothing in these crates reads a clock or the filesystem, and Miri suppliesgetrandomitself.push/pull_requestrather than a nightly schedule. These crates change on ordinary protocol PRs, which is exactly when the aliasing claims get invalidated; a nightly result arrives after the PR merged. At four minutes it is cheaper than most jobs already in the matrix.What the gate found before it went green
Three aliasing violations, all in dependencies, none in this workspace:
inout0.2.2, above — production AES-CBC path. Cost thewacore-appstateleg.zlib-rs0.6.6 deflate —deflate::endfrees its buffers while a&mutinto them is still protected. Reached only by the test helper that built a fixture withflate2; compression is not a path this crate has, inflate is. Fixed by hand-building the fixture as a stored deflate block plus its adler32, which drops the compressor from the interpreted set entirely rather than dropping the test.Tests
Two of the three unsafe sites were not reachable from any Miri leg as first written, and the second gap was subtler than the first. Both caught in review by @chatgpt-codex-connector.
OwnedNodeRefwas constructed in exactly one test, inside#[cfg(feature = "serde")], which neither leg enables. And even fixing that would not have reachedAttrsRef's hand-written impl: yoke's derive transmutes the whole struct in one go and never calls a field type's methods, so all three of its methods are reachable only through a yoke ofAttrsRefitself, which nothing built. A gate that reports green over the regression it exists to catch is worse than no gate.yoked_attrs_ref_survives_make_transform_and_mutation(new)AttrsRefborrowing its key and value from the cart, somake,transformandtransform_muteach carry a real lifetime across — our transmutes rather than yoke's generated onesborrowed_payloads_survive_moving_the_cart(new)OwnedNodeRefmoved through aBoxand into aVecstill reads its tag, attrs and content back — the wholeStableDerefclaim, which nothing previously exercisedslice_bytes_views_the_backing_buffer_without_copying(new)slice_bytesreturns a view at the same address, not a copy — its pointer arithmetic had no coverage at allpooled_roundtrip_small_input(new)inflate_into_spare'sset_lenunder the interpreter, on a compressor-free fixtureThe nine
zlib_poolfixtures are 100 KB–4 MB, and not gratuitously — window refill, the ratio-projected growth path and shrink-on-return only happen at that scale — so they are#[cfg_attr(miri, ignore)]behind one comment explaining the pattern.lookup_matches_reference_under_byte_mutationjoins them: ~3M table probes, one second natively, and it is pure safe code with no raw pointers. It was what left thewacore-binaryleg still running after twelve minutes; that leg now finishes in 37 seconds.Each ignore was paid for by a measurement. A proposal to trim the Noise crate's large fixtures too was declined for want of one — the whole leg is 3m16s, and
vec[0u8; 16 MiB]lowers to__rust_alloc_zeroed, which Miri services as a single operation rather than 16 M writes.Verification
All three legs green on
89757cd, at the times in the table above.cargo fmt --allandcargo clippy -p wacore-binary --all-targets -- -D warningsclean;cargo test -p wacore-binary --liband--no-default-features --libboth pass 117 tests, so theignoreattributes did not quietly take anything out of the native run.Follow-ups, deliberately not smuggled in here
inoutandzlib-rsaliasing violations upstream to RustCrypto and trifectatechfoundation.provider.rsshould use the buffer-to-buffer cipher API instead. Changing production crypto to satisfy an interpreter is a trade-off worth its own discussion, not a detail of adding CI.main.yml,wasm.yml,supply-chain.yml,e2e.ymlanddocker.ymlall use@v6/@master; pinning one new file leaves two conventions and no rule, and the supply-chain surface is unchanged while the other five resolve the same mutable refs. Its own PR, ideally with Dependabot'sgithub-actionsecosystem pointed at the pins so they do not rot.