test(net): fuzz the wire codecs - #3198
Conversation
Decoding an IETF SUBSCRIBE (or PUBLISH_OK) keeps the filter type but drops the Location an absolute filter carries after it, so the struct cannot express what encoding it would need to write. Draft-14 encode guarded that with a `debug_assert!`, which is reachable from a peer's own message: it aborts under debug assertions, and in release it emits a truncated message whose next field the peer reads as the missing Location. Return the new `EncodeError::Unsupported` instead. Found by the fuzz target added in the following commit; the crash input is committed there as a regression. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Coverage-guided fuzzing (libFuzzer via cargo-fuzz) of the bytes a relay parses from an untrusted peer: 21 moq-lite types across 6 versions, 37 IETF types across 6 drafts, both varint codecs, and the `Path` invariants. `just rs fuzz <target>` runs it. The target bodies live in moq-net's hidden `fuzz` module rather than in `fuzz/fuzz_targets/`, for two reasons. `lite` and `ietf` are private modules, so an outside crate cannot reach a single decoder. And sharing the bodies lets the crate's own tests replay the generated corpus, plus every crash input committed under `fuzz/regressions/`, on the pinned stable toolchain as part of `just test` -- so a crash found by fuzzing becomes a regression test CI runs without anyone installing cargo-fuzz. libFuzzer needs `-Zsanitizer`, so the fuzzer itself stays out of `check` and `test` the way `loom` does. Beyond "does not panic", each target asserts that what we encode we decode back consuming every byte, and that encoding is byte-stable. The seed corpus is generated from the dispatch rather than committed, and `seeds_reach_every_arm` fails when nothing decodes for an arm, so a target cannot silently cover less than it lists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. |
|
Warning Review limit reachedNext included review available in 14 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (18)
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 |
Summary
Pathinvariants.just rs fuzz <target>runs it; targets arelite,ietf,varint,path.fuzzmodule (src/fuzz.rs), not infuzz/fuzz_targets/.liteandietfare private modules, so an outside crate cannot reach a single decoder; and sharing the bodies lets the crate's own tests replay the corpus, plus every crash input underfuzz/regressions/, on the pinned stable toolchain as part ofjust test. A crash found by fuzzing becomes a regression test CI runs without anyone installing cargo-fuzz. The fuzzer itself stays out ofcheck/testbecause libFuzzer needs-Zsanitizer, the same wayloomdoes.seeds_reach_every_armfails when no seed decodes for an arm, so a target cannot silently cover less than it lists.The first run found a real bug, fixed in the first commit. Decoding an IETF SUBSCRIBE (and PUBLISH_OK) keeps the filter type but drops the
Locationan absolute filter carries after it, so the struct cannot express what encoding it would need to write. Draft-14 encode guarded that with adebug_assert!, which is reachable from a peer's own message: it aborts under debug assertions, and in release it emits a truncated message whose next field the peer reads as the missingLocation. It now returnsEncodeError::Unsupported. The crash input is committed atfuzz/regressions/ietf/subscribe-absolute-filter, and fails the suite without the fix.Two things the targets deliberately do not assert, both documented in the code:
HashMap, so byte-stability is unasserted for moq-lite SETUP and drafts 14/15 (16+ sort by key first). The nondeterministic wire output is real but out of scope here.u64, aboveVarInt::MAX's documented 62-bit ceiling. The existing tests already call this out.No CI wiring: fuzzing is a local recipe, and what gates PRs is the stable replay in
just test.Public API changes
moq_net::EncodeError::Unsupported: new variant. Additive, and the enum is#[non_exhaustive], so this targetsmain.moq-netgains an off-by-defaultfuzzfeature enabling a#[doc(hidden)] pub mod fuzz. Nothing but the harness depends on it, and it is absent from a default build.drafts/update: the fix refuses to emit a message that was never encodable correctly in the first place. Nothing to mirror injs/neteither, since the change is a Rust-side error return.Test plan
cargo clippy --locked --workspace --all-targets -- -D warnings, plus-p moq-net --features fuzz(whichchecknever compiles, since it builds default features only).RUSTDOCFLAGS="-D warnings" cargo docwith and without thefuzzfeature. The feature build is whatjust rs featurescompiles nightly, and it caught a private intra-doc link.cargo nextest run --locked --workspace --all-targets: 3266 passed.cargo fmt --all --check,cargo sort --workspace --check,cargo shear,taplo format --check,bun remark --frail,just --fmt --check.just rs fuzz: lite 5.4M execs, ietf 8.7M, varint 6.4M, path 2.6M, all clean after the fix.fuzz::tests::regressionsfails with thedebug_assert!restored and passes with the fix.(Written by Claude Opus 5)