Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 98 additions & 0 deletions .github/workflows/miri.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
permissions:
contents: read
name: Miri

on:
push:
branches: [main]
pull_request:
branches: [main]
workflow_call:

env:
CARGO_TERM_COLOR: always
# Keep in sync with PROTOC_VERSION in main.yml — env is per-workflow, so the
# value cannot be shared across files without a repo-level Actions variable.
PROTOC_VERSION: '3.25.3'
CARGO_INCREMENTAL: "0"
# Miri interprets MIR and never links, so the workspace's lld/ICF and
# -Zshare-generics rustflags buy nothing here; a set-but-empty RUSTFLAGS takes
# precedence over .cargo/config.toml (same lever main.yml's stable job pulls).
RUSTFLAGS: ""

jobs:
miri:
name: Miri (${{ matrix.name }})
runs-on: ubuntu-latest
# Interpretation costs two orders of magnitude over native. The fixtures the
# gate covers are small by construction (the MB-scale zlib ones are
# `#[cfg_attr(miri, ignore)]`), but a cold sysroot build alone is minutes.
timeout-minutes: 30
strategy:
# Each leg is an independent UB question; one failing should not hide the
# verdict on the others.
fail-fast: false
matrix:
include:
# wacore-binary owns the workspace's only load-bearing `unsafe`: the
# `Yokeable`/`StableDeref` impls behind `OwnedNodeRef` (two lifetime
# transmutes over borrowed decode output) and the `set_len` over
# inflate's uninitialized spare capacity in `zlib_pool`. Both are
# invisible to clippy and to native tests — nothing observes the
# aliasing violation or the uninit read until it miscompiles.
- name: wacore-binary
cache-key: binary-simd
args: -p wacore-binary --lib
Comment thread
jlucaso1 marked this conversation as resolved.
# The portable-SIMD scanners in the decoder/encoder and their scalar
# fallbacks are separate code paths, and `--no-default-features` is the
# only way to reach the latter.
- name: wacore-binary (no simd)
cache-key: binary-scalar
args: -p wacore-binary --no-default-features --lib
# No `unsafe` of its own, but it drives wacore-binary's zero-copy
# decode over real Noise frames and pulls the crypto stack
# (aes/sha2/curve25519), whose unsafe backends this exercises.
#
# wacore-appstate is deliberately absent: `inout` 0.2.2's
# `PaddedInOutBuf::into_out` invalidates the `&mut [u8]` that `cbc`'s
# `encrypt_padded` still holds protected, which Miri rejects under
# Stacked Borrows. That is on the AES-CBC path every appstate record
# takes, so the leg cannot be green until the dependency is fixed —
# nothing in this workspace can make it so.
- name: wacore-noise
cache-key: noise
args: -p wacore-noise --lib

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

steps:
- uses: actions/checkout@v6
Comment thread
coderabbitai[bot] marked this conversation as resolved.
with:
persist-credentials: false
- uses: dtolnay/rust-toolchain@master
Comment thread
greptile-apps[bot] marked this conversation as resolved.
with:
toolchain: nightly-2026-06-16
# rust-src is what `cargo miri setup` compiles the interpreter's
# sysroot from; installing `miri` does not pull it in.
components: miri, rust-src
# waproto is in the appstate/noise graphs, and its build script needs protoc.
- name: Install protoc
uses: taiki-e/install-action@v2
with:
tool: protoc@${{ env.PROTOC_VERSION }}
# No sccache here: cargo-miri drives the build through its own
# RUSTC_WRAPPER and the two cannot share that slot.
- name: Cache Rust build (registry + target + Miri sysroot)
uses: Swatinem/rust-cache@v2
with:
cache-targets: "true"
# `cargo miri setup` builds the interpreted sysroot here; without it
# every run recompiles core/std from rust-src.
cache-directories: ~/.cache/miri
# Each leg interprets a different feature set into target/miri.
key: ${{ matrix.cache-key }}
- name: Build Miri sysroot
run: cargo miri setup
# Default flags: Stacked Borrows, isolation on. Deliberately no
# -Zmiri-strict-provenance — `bytes` rebuilds its tagged `Shared` pointer
# out of an integer, which strict provenance rejects on sight and which is
# not the class of bug this gate is looking for.
- name: Run tests under Miri
run: cargo miri test ${{ matrix.args }}
2 changes: 2 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ cargo clippy --workspace --all-targets -- -D warnings # what CI enforces

Workspace clippy takes minutes — pushing and letting CI parallelize the matrix is usually faster. E2E tests (`cargo test -p e2e-tests`) need the mock server running; see `agent_docs/e2e_testing.md`.

Touching `unsafe` — the `Yokeable`/`StableDeref` impls in `wacore-binary`'s `node.rs`, the `set_len` in `zlib_pool.rs` — means CI's Miri gate (`.github/workflows/miri.yml`) is what proves it, since neither clippy nor a native test observes an aliasing violation or an uninit read. Locally: `rustup component add miri rust-src && cargo miri test -p wacore-binary --lib`. Interpretation is ~100× native, so a fixture that only makes sense at hundreds of KB (zlib window refill, buffer growth) belongs behind `#[cfg_attr(miri, ignore)]` with a small twin that keeps the `unsafe` covered.
Comment thread
jlucaso1 marked this conversation as resolved.

## Gotchas

Things that look correct and are not:
Expand Down
51 changes: 51 additions & 0 deletions wacore/binary/src/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1008,6 +1008,57 @@ impl OwnedNodeRef {
}
}

#[cfg(test)]
mod owned_node_ref_tests {
use super::*;

/// Raw binary-protocol bytes, as `OwnedNodeRef::new` wants them: `marshal`
/// writes a leading format byte that `unmarshal_ref` does not expect.
fn encoded(node: &Node) -> Bytes {
let bytes = crate::marshal::marshal(node).unwrap();
Bytes::from(bytes[1..].to_vec())
}

fn sample() -> Node {
Node::new(
"iq",
Attrs(vec![(Cow::Borrowed("id"), NodeValue::String("abc".into()))].into()),
Some(NodeContent::Bytes(b"payload".to_vec())),
)
}

#[test]
fn borrowed_payloads_survive_moving_the_cart() {
Comment on lines +1030 to +1031

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

let node = sample();
let owned = OwnedNodeRef::new(encoded(&node)).unwrap();

// Move the value twice — through a Box and into a Vec — before reading
// anything back. That is the whole `StableDeref` claim: the yoked
// `NodeRef` keeps pointing at live bytes even though the wrapper it
// borrows from has moved. Nothing but an interpreter notices when it
// stops being true, which is why this test exists separately from the
// serde one it used to be a side effect of.
let mut moved = vec![*Box::new(owned)];
let owned = moved.pop().unwrap();

assert_eq!(owned.tag(), "iq");
assert!(owned.get_attr("id").unwrap() == "abc");
assert_eq!(owned.content_bytes(), Some(&b"payload"[..]));
assert_eq!(owned.to_owned_node(), node);
}

#[test]
fn slice_bytes_views_the_backing_buffer_without_copying() {
let owned = OwnedNodeRef::new(encoded(&sample())).unwrap();
let content = owned.content_bytes().unwrap();

let view = owned.slice_bytes(content);

assert_eq!(view.as_ref(), b"payload");
assert_eq!(view.as_ptr(), content.as_ptr(), "slice_bytes copied");
}
}

#[cfg(feature = "serde")]
impl serde::Serialize for OwnedNodeRef {
fn serialize<S: serde::Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
Expand Down
6 changes: 6 additions & 0 deletions wacore/binary/src/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,13 @@ mod tests {
/// `None`). Probes the length-bucketed lookup for any discriminator collision
/// that would silently map a non-token (a JID/id) onto a token and corrupt the
/// wire, and stays durable against future token-set edits.
///
/// Ignored under Miri: ~3M lookups (every byte position of every token,
/// times 256) is minutes of interpretation for a table probe that holds no
/// `unsafe` and no raw pointers, so there is nothing there for the
/// interpreter to find.
#[test]
#[cfg_attr(miri, ignore)]
fn lookup_matches_reference_under_byte_mutation() {
use std::collections::HashMap;

Expand Down
54 changes: 54 additions & 0 deletions wacore/binary/src/zlib_pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,53 @@ mod tests {
.collect()
}

/// A zlib stream carrying `data` verbatim in one stored (uncompressed)
/// deflate block, hand-built so the fixture costs no compressor.
///
/// `zlib()` above cannot be used under Miri: 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. That is the compression half, which this
/// crate never runs — inflate is the whole production path — so the fixture
/// side steps around it rather than the test being dropped.
fn stored_zlib(data: &[u8]) -> Vec<u8> {
assert!(data.len() <= u16::MAX as usize, "one stored block only");
// 0x78 0x01: deflate, 32 KB window, and (0x78 << 8 | 0x01) % 31 == 0 as
// the header check requires.
let mut out = vec![0x78, 0x01];
let len = data.len() as u16;
// BFINAL=1, BTYPE=00 (stored), then the byte-aligned LEN/!LEN pair.
out.push(0x01);
out.extend_from_slice(&len.to_le_bytes());
out.extend_from_slice(&(!len).to_le_bytes());
out.extend_from_slice(data);

let (mut a, mut b) = (1u32, 0u32);
for &byte in data {
a = (a + byte as u32) % 65521;
b = (b + a) % 65521;
}
out.extend_from_slice(&(((b << 16) | a).to_be_bytes()));
out
}

// Every other test here is sized in hundreds of KB to MB — the only way to
// reach window refill, the growth projection and shrink-on-return — which
// puts a full inflate cycle hours out of reach of Miri's interpreter, so
// they are `#[cfg_attr(miri, ignore)]`. This one keeps the `set_len` in
// `inflate_into_spare` under Miri on a fixture it can finish.
#[test]
fn pooled_roundtrip_small_input() {
let original = varied(1024);
let compressed = stored_zlib(&original);
assert_eq!(
decompress_zlib_pooled(&compressed, 64 * 1024).unwrap(),
original
);
assert_eq!(drain_reader(&compressed, original.len()), original);
}

#[test]
#[cfg_attr(miri, ignore)]
fn inflate_reader_roundtrip_across_chunks() {
// >128 KB so the stream spans multiple 64 KB decompress windows, and read
// it back in tiny odd steps to exercise refill + compaction.
Expand All @@ -408,6 +454,7 @@ mod tests {
}

#[test]
#[cfg_attr(miri, ignore)]
fn inflate_reader_ensure_larger_than_chunk() {
// A single record bigger than the 64 KB window must be fully buffered.
let original: Vec<u8> = (0..150 * 1024).map(|i| (i % 256) as u8).collect();
Expand All @@ -418,6 +465,7 @@ mod tests {
}

#[test]
#[cfg_attr(miri, ignore)]
fn inflate_reader_keeps_one_window_for_smaller_records() {
INFLATE_POOL.with(|p| p.borrow_mut().clear());
const RECORD: usize = 30 * 1024;
Expand All @@ -440,6 +488,7 @@ mod tests {
}

#[test]
#[cfg_attr(miri, ignore)]
fn inflate_reader_enforces_max() {
let original = vec![0u8; 1024 * 1024];
let compressed = zlib(&original);
Expand All @@ -448,6 +497,7 @@ mod tests {
}

#[test]
#[cfg_attr(miri, ignore)]
fn pooled_high_ratio_stream_roundtrips() {
// ~50x expansion: the 2x up-front guess undershoots badly, so this
// exercises the ratio-projected growth path end to end.
Expand All @@ -470,6 +520,7 @@ mod tests {
}

#[test]
#[cfg_attr(miri, ignore)]
fn pooled_oneshot_matches_streaming() {
let original = varied(100_000);
let compressed = zlib(&original);
Expand All @@ -490,6 +541,7 @@ mod tests {
}

#[test]
#[cfg_attr(miri, ignore)]
fn inflate_reader_reuses_pool_state_correctly() {
// Back-to-back readers each checkout the pooled Decompress and reset it, so
// no state may carry over between streams. Verify several sizes in sequence.
Expand All @@ -500,6 +552,7 @@ mod tests {
}

#[test]
#[cfg_attr(miri, ignore)]
fn inflate_reader_reuse_after_error() {
// A reader aborted mid-stream (max exceeded) returns partial zlib state to
// the pool; the next checkout must reset it and decompress a full stream.
Expand All @@ -513,6 +566,7 @@ mod tests {
}

#[test]
#[cfg_attr(miri, ignore)]
fn drop_shrinks_oversized_buffer_before_pooling() {
// Buffering a large record grows `buf` to many MB; on return to the pool it
// must be shrunk back toward the bounded steady-state capacity, not parked
Expand Down
Loading