Fallible encoding with sink - #315
Merged
Merged
Conversation
EncodingError carries ValueTooLarge {field, actual, max} and Unencodable
{field, reason}. All length-prefixed wire fields will be written through
with_u8_len/with_u16_len (back-patched prefixes) or put_u8/u16_len_slice,
so every capacity check lives in one place and non-power-of-two bounds
are explicit via check_max. Groundwork for fallible encoding (issue bgpkit#313).
Assisted-by: Claude Code
Leaf attribute encoders that can overflow a wire length field now write into a shared BytesMut and return Result<(), EncodingError>: - AS_PATH rejects segments with >255 ASes (was: silent u8 truncation) - Tunnel Encap, BGP-LS, SFP, BFD Discriminator, Prefix-SID and BIER TLV value lengths are checked (was: silent u8/u16 truncation) - FlowSpec NLRI length is bounded at 0x0FFF, matching the 12-bit wire field that parse_length reads (was: unchecked cast to u16, corrupting any NLRI of 4096..=65535 bytes) - encode_nlri propagates labeled-prefix errors and now encodes flowspec_nlris (was: silently dropped) Attribute::encode_to writes the value through with_u8_len/with_u16_len according to the attribute's EXTENDED_LENGTH flag, so an oversized value yields ValueTooLarge instead of a truncated length byte. ATTR_SET returns Unencodable instead of Ok + empty value. Attributes::encode now returns Result; the two call sites in BgpUpdateMessage and RibEntry carry temporary expect()s until they are converted next. Tlv::length()/SubTlv::length() (silently saturating, dead on production paths) are removed. Assisted-by: Claude Code
All encode() methods from BgpOpenMessage up to MrtRecord now return Result<Bytes, EncodingError>; no silent truncation and no panicking wrappers remain on any encode path: - BgpOpenMessage rejects optional parameter type 255 (reserved by RFC 9072 as the extended-length marker; emitting it round-trips to a structurally different message) and returns an error instead of debug_assert when extended parameters exceed u16::MAX - BgpUpdateMessage writes withdrawn-routes and path-attribute sections through checked length prefixes - BgpMessage checks the total message length against the u16 wire field - TableDumpMessage uses the shared Attributes::encode_to (removing a hand-rolled duplicate loop); RibAfiEntries checks the entry count (was: wrapping cast) and propagates per-entry errors - PeerIndexTable/GeoPeerTable check view-name length and peer count - MrtMessage::encode returns Unencodable for RIB_GENERIC instead of todo!(); Bgp4MpMessage, MrtRecord, and both MRT encoders' export_bytes() propagate errors Assisted-by: Claude Code
PeerIndexTable::add_peer returns Result<u16, EncodingError> and refuses the 65536th distinct peer (the wire Peer Count field is 16-bit), leaving the table unmodified instead of silently wrapping the id and aliasing routes onto an existing peer. MrtRibEncoder::process_elem propagates the error so a writer learns about the overflow when it happens, not after the in-memory table is already corrupted. Assisted-by: Claude Code
deny(unused_must_use) at the crate root so a dropped Result from an encode_to() call — which would silently skip wire data — fails the build. New tests cover one error path per wire bound: non-extended and extended attribute value lengths, ATTR_SET (Unencodable), MP_REACH labeled prefix with an empty label stack (previously swallowed with a log line), RIB entry attribute overflow propagation (previously the entry vanished from the record), RIB entry count, OPEN parameter type 255, BGP message total length, tunnel-encap sub-TLV length, and the FlowSpec 4096..=65535 range that fits a u16 but not the 12-bit wire length field. Assisted-by: Claude Code
Assisted-by: Claude Code
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #315 +/- ##
==========================================
+ Coverage 90.31% 90.51% +0.20%
==========================================
Files 91 92 +1
Lines 19306 19519 +213
==========================================
+ Hits 17436 17668 +232
+ Misses 1870 1851 -19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4 tasks
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
2 participants
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.
LLM-driven (Fable) approach of an alternative to the approach in #314 (comment). Follows the plan steps in fallible-encoding-redesign.md