Conversation
c407b74 to
2d381d0
Compare
daniel-noland
left a comment
There was a problem hiding this comment.
notes for agent
There was a problem hiding this comment.
Pull request overview
Adds protocol-aware “packet pattern” matching over Headers, using type-level layer adjacency (Within<T>) to enforce valid layer transitions at compile time while performing strict runtime gap checks for VLANs (and optionally IPv6 extensions).
Changes:
- Moved
Within<T>and all layer-ordering/conformance impls into a newheaders::withinmodule. - Added
headers::patproviding immutable/mutable matchers (pat()/pat_mut()) plus embedded ICMP payload matching. - Adjusted visibility of
EmbeddedHeadersfields to support matcher implementation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| net/src/headers/within.rs | Centralizes Within<T> trait + ordering/conformance graph (incl. IPv6 extensions + embedded headers). |
| net/src/headers/pat.rs | Introduces the pattern-matching API, including mutable + embedded matchers and tests. |
| net/src/headers/mod.rs | Wires in within and exposes the new pat module. |
| net/src/headers/embedded.rs | Widens EmbeddedHeaders field visibility for matcher access. |
| net/src/headers/builder.rs | Removes local Within definition/impls and re-exports the moved trait. |
48cda1d to
8b1c7d6
Compare
8b1c7d6 to
4764ea7
Compare
4764ea7 to
47c5b14
Compare
47c5b14 to
5ed0ad4
Compare
5ed0ad4 to
3b2ab0e
Compare
| /// been consumed (`net_ext.len() == ext_cursor`). | ||
| /// | ||
| /// This is a sealed implementation detail -- all impls are provided by | ||
| /// this module and users never need to reference this trait directly. |
There was a problem hiding this comment.
confidence: 8
tags: [docs]ExtGapCheck is documented as a sealed implementation detail that users “never need to reference”, but it is declared pub inside the public headers::pat module, making it part of the crate’s public API surface. To keep the public API minimal (and align docs with reality), consider reducing its visibility (e.g., pub(crate)/pub(super)) and relying on the existing #![allow(private_bounds)], or keep it pub but add #[doc(hidden)] to avoid advertising it as public API.
| /// this module and users never need to reference this trait directly. | |
| /// this module and users never need to reference this trait directly. | |
| #[doc(hidden)] |
b650ad5 to
fa8f1ae
Compare
94dbfe4 to
138589d
Compare
Move the Within<T> trait and all layer-ordering impls out of the
feature-gated builder module into headers::within (always compiled).
Also:
- Add Within impls for Net/Transport enums (no-op conform) for
enum-level pattern matching
- Add Within impls for EmbeddedHeaders (after Icmp4/Icmp6) and
truncated transport types for embedded ICMP payload matching
- Add EmbeddedStart marker for embedded matcher entry position
- Re-export EmbeddedStart publicly from headers module
- Make EmbeddedHeaders.{net, net_ext, transport} pub(super) for
the pattern matcher (narrowed from pub(crate))
The enum-level Within impls use no-op conform because the concrete
variant is unknown statically. The builder is safe from these:
HeaderStack::stack additionally requires Blank, which enums do not
implement -- stacking Net or Transport directly is a compile error.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Adds Matcher/MatcherMut builders for type-safe, adjacency-checked packet header pattern matching, plus EmbeddedMatcher/EmbeddedMatcherMut for mutable access to ICMP error payloads (required for NAT per RFC 5508). 48 tests total: bolero fuzz tests for all positive/negative matching, plus adversarial tests using HeadersBuilder to verify graceful behavior on structurally broken headers (mismatched IP/ICMP versions, transport without net, VLANs without net, embedded without ICMP, empty headers). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
138589d to
747e10e
Compare
mvachhar
left a comment
There was a problem hiding this comment.
Wow, there is a lot of tedious stuff in here. It looks reasonable, though I am relying on the fact that the tests pass rather than a detailed analysis of every case in the implementation. I'm unhappy about the tuple expansion stuff, but without variadic templates, there isn't much alternative. In the C++ world, this type of thing did pay off for Boost which had very ergonomic APIs. Eventually C++ added variadic templates and alot of the ugliness went away. I hope the same happens in Rust.
| impl_tuple_append!(A, B, C, D, E, F, G, H, I, J, K, L, M); | ||
| impl_tuple_append!(A, B, C, D, E, F, G, H, I, J, K, L, M, N); | ||
| impl_tuple_append!(A, B, C, D, E, F, G, H, I, J, K, L, M, N, O); | ||
| impl_tuple_append!(A, B, C, D, E, F, G, H, I, J, K, L, M, N, O, P); |
There was a problem hiding this comment.
I thought I was done with this crap. Rust needs variadic generics!
Match on the shape of a packet!
Adds
headers::pat, a protocol-aware pattern matching API forHeaders. The matcher chain usesWithin<T>bounds (from #1445) to enforce valid layer ordering at compile time and runtime gap checks to catch silently skipped layers (e.g. an unaccounted-for VLAN between Ethernet and IPv4).pat()) and mutable (pat_mut()) variants are provided, with mutable access via split borrows andsplit_first_mutno unsafe code is used (directly)..embedded()with a nested sub-tuple, supporting the NAT rewrite path required by RFC 5508. The abstraction is zero-cost:cargo-show-asmconfirms the generated assembly is identical to hand-written code.opt_*) distinguish "genuinely absent" from "wrong variant present": a wrong variant is a miss, not absence..when(),.inspect(), and.otherwise()are available at any chain position