Repository navigation
test(wire): assert one attribute per type in debug encoders - #2981
Merged
Merged
Conversation
Debug builds now panic when the shared path-attribute encoder or an MRT RIB entry would emit the same type code twice (RFC 4271 section 5). This catches an emitter that copies a stored attribute and also synthesizes one of the same type, the class behind the duplicate NEXT_HOP in MRT, warm checkpoints and BMP Loc-RIB. Release builds skip the check.
The encoder is public, and a debug assertion there would panic on caller-supplied attributes in an embedder's debug build. The check now compiles only with the non-default strict-encode-invariants feature, which the workspace enables through dev-dependencies for its own tests. Default, release and bench builds are unchanged.
There was a problem hiding this comment.
🟡 Changes recommended
The MRT guard can falsely panic on oversized attributes before existing length-error handling runs.
1 open finding
What changed in this PR
Adds duplicate path-attribute checks to catch encoder regressions like #2977, while preserving default wire-crate and release behavior.
Changes:
- Adds an opt-in wire encoder invariant enabled by workspace test dependencies.
- Adds debug-only MRT entry checks.
- Adds regression tests and documents the feature.
| File | Description |
|---|---|
| crates/wire/src/attribute.rs | Checks emitted attribute types and tests duplicate rejection. |
| crates/wire/CHANGELOG.md | Documents the opt-in invariant. |
| crates/wire/Cargo.toml | Declares the non-default feature. |
| crates/transport/Cargo.toml | Enables checks for transport tests. |
| crates/rib/Cargo.toml | Enables checks for RIB tests. |
| crates/mrt/src/codec.rs | Checks complete MRT entries and tests extended-length parsing. |
| crates/mrt/Cargo.toml | Enables wire checks for MRT tests. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
An attribute payload over u16::MAX has its length narrowed by the wire encoder, so the debug walk misread the payload as further headers and panicked on a false duplicate before the caller could return FieldTooLarge. The walk now skips any block it cannot read faithfully.
lance0
marked this pull request as ready for review
October 8, 2026 03:12
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Workspace test builds now panic when one encoded attribute list would carry the same path-attribute type twice (RFC 4271 §5). Default, release and bench builds, and embedders of
rustbgpd-wire, are unchanged.Why
#2977 fixed a duplicate
NEXT_HOPin MRT dumps, warm checkpoints and BMP Loc-RIB. Each encoder copied the route's stored attributes and also synthesized an attribute of a type already in storage. No test caught it, because the encoder tests built routes by hand.An audit of every emitter that copies stored attributes and also adds one found no other reachable duplicate on current main. This PR adds a guard so the next one fails in tests instead of reaching the wire.
Change
rustbgpd-wireencoder (encode_path_attributes_with_scratch): a[bool; 256]records each emitted type, including the synthesized AS4_PATH/AS4_AGGREGATOR compatibility attributes. This one path covers export UPDATEs (per-session and shared update-group), Adj-RIB-Out BMP (the same bytes) and BMP Loc-RIB synthesis.cfg(all(debug_assertions, any(test, feature = "strict-encode-invariants"))). The feature is off by default, so embedders never panic on caller-supplied input.[dev-dependencies]only. Under resolver 2 a releasecargo build -p rustbgpdcompiles wire with no features (checked against the rustc command line), andcargo test --workspaceturns it on for every package, root tests included.cliamong root dev-dependencies.debug_assertionsis off in the bench profile, so measurements are unaffected.publish = false): MRT encodes one attribute per wire-encoder call, so the wire check cannot see across them. A debug-only walk of each encoded RIB entry runs in the unicast, EVPN and generic entry writers.Evidence
mrt_next_hopregressions panic in the new guards (duplicate path attribute type 3 ...). The IPv6 regression still passed.NEXT_HOPpresence check inprepare_unicast_attributesmade 58 existing transport tests fail, each panicking in the wire guard.unique_attribute_guard_walks_extended_length_attributesred.mrt_next_hopfailures and 58 transport failures.cargo fmt --check,just gate-ribandjust gateall pass (10,511 passed, 0 failed); rustc/clippy 1.99.0.No daemon behavior change, so there is no root changelog fragment. The wire crate changelog records the new non-default feature.