diff --git a/CHANGELOG.md b/CHANGELOG.md index ef558e5..83ea381 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,10 +6,35 @@ All notable changes to this project will be documented in this file. ### Breaking changes +* **Fallible encoding** ([#313](https://github.com/bgpkit/bgpkit-parser/issues/313)): all encode paths now return `Result<_, EncodingError>` instead of silently truncating values that do not fit their wire-format fields. There are no panicking wrappers; callers that want the old "just give me bytes" ergonomics can `.unwrap()`. Migration table: + + | Before | After | + | --- | --- | + | `Attribute::encode(asn_len) -> Bytes` | `-> Result` (also `encode_to(asn_len, &mut BytesMut)`) | + | `Attributes::encode(asn_len) -> Bytes` | `-> Result` (also `encode_to`) | + | `BgpOpenMessage::encode() -> Bytes` | `-> Result` | + | `BgpUpdateMessage::encode(asn_len) -> Bytes` | `-> Result` | + | `BgpMessage::encode(asn_len) -> Bytes` | `-> Result` | + | `TableDumpMessage::encode() -> Bytes` | `-> Result` | + | `RibEntry::encode()` / `RibAfiEntries::encode() -> Bytes` | `-> Result` | + | `PeerIndexTable::encode()` / `GeoPeerTable::encode() -> Bytes` | `-> Result` | + | `PeerIndexTable::add_peer(peer) -> u16` | `-> Result` (rejects the 65,536th peer instead of wrapping the id) | + | `MrtMessage::encode(sub_type)` / `Bgp4MpMessage::encode(asn_len)` / `MrtRecord::encode() -> Bytes` | `-> Result` | + | `MrtRibEncoder::process_elem(&elem)` | `-> Result<(), EncodingError>` | + | `MrtRibEncoder::export_bytes()` / `MrtUpdatesEncoder::export_bytes() -> Bytes` | `-> Result` | + + `EncodingError` has two variants: `ValueTooLarge { field, actual, max }` for wire-capacity overflows and `Unencodable { field, reason }` for values with no valid wire representation (ATTR_SET, RIB_GENERIC, OPEN parameter type 255, labeled NLRI with an empty label stack). +* **`Tlv::length()` and `SubTlv::length()` removed**: both silently saturated at `u16::MAX`; encoders now compute checked lengths internally. * **`OptParam` no longer has a `param_len` field**: the field was redundant now that the encoder always derives the wire length from `param_value`, and the parser recomputes it on read. Construct `OptParam` with just `param_type` and `param_value`. ### Fixed +* **Silent truncation on encode eliminated** ([#313](https://github.com/bgpkit/bgpkit-parser/issues/313)): AS_PATH segments with >255 ASes, attribute values exceeding their (extended or non-extended) length field, TLV values in Tunnel Encapsulation / BGP-LS / SFP / BFD Discriminator / Prefix-SID / BIER attributes, RIB entry counts, peer counts, view names, and BGP message total lengths now return `EncodingError` instead of writing corrupt length fields. +* **FlowSpec NLRI length bound**: the wire length field is 12-bit (RFC 8955); NLRI data of 4096..=65535 bytes previously passed an unchecked `u16` cast and encoded a corrupt length byte. It is now rejected with `ValueTooLarge`. +* **MP_REACH/MP_UNREACH encoding errors are no longer swallowed**: labeled-NLRI failures previously logged a warning and emitted a zero-length (RFC-invalid) attribute value; they now propagate as `EncodingError`. +* **FlowSpec NLRI entries are now encoded**: `Nlri::flowspec_nlris` was previously silently dropped when encoding MP_REACH/MP_UNREACH attributes. +* **Peer index aliasing**: `PeerIndexTable::add_peer` previously wrapped the peer id at 65,536 peers, silently attributing routes to the wrong peer; it now returns an error and leaves the table unmodified. +* **BGP OPEN parameter type 255 rejected**: RFC 9072 reserves type 255 as the extended-length marker; encoding it as a real parameter produced output that round-tripped to a structurally different message. * **BGP OPEN optional-parameter encoding**: Encode the Optional Parameters Length as the total byte length required by RFC 4271 instead of the number of parameters. OPEN messages now also use the extended length format from RFC 9072 when requested or required. ## v0.19.0 - 2026-07-28 diff --git a/examples/filter_export_rib.rs b/examples/filter_export_rib.rs index 73a5be2..31d58fa 100644 --- a/examples/filter_export_rib.rs +++ b/examples/filter_export_rib.rs @@ -18,12 +18,14 @@ fn main() { info!("processing rib {}", RIB_URL); for elem in parser { - encoder.process_elem(&elem); + encoder.process_elem(&elem).unwrap(); } info!("exporting filtered RIB..."); let mut writer = oneio::get_writer("filtered-13335.rib.gz").unwrap(); - writer.write_all(encoder.export_bytes().as_ref()).unwrap(); + writer + .write_all(encoder.export_bytes().unwrap().as_ref()) + .unwrap(); drop(writer); info!("exporting filtered RIB...done"); diff --git a/examples/mrt_filter_archiver.rs b/examples/mrt_filter_archiver.rs index be3b859..4927a8b 100644 --- a/examples/mrt_filter_archiver.rs +++ b/examples/mrt_filter_archiver.rs @@ -27,7 +27,7 @@ fn main() { .unwrap() .into_record_iter() .for_each(|record| { - let bytes = record.encode(); + let bytes = record.encode().unwrap(); mrt_writer.write_all(&bytes).unwrap(); records_count += 1; let mut elementor = Elementor::new(); diff --git a/examples/parse_bmp_mpls.rs b/examples/parse_bmp_mpls.rs index 1dbb37f..6b63f91 100644 --- a/examples/parse_bmp_mpls.rs +++ b/examples/parse_bmp_mpls.rs @@ -72,7 +72,7 @@ fn create_bmp_mpls_message() -> Vec { let bgp_msg = BgpMessage::Update(bgp_update); // Encode the BGP message - let bgp_bytes = bgp_msg.encode(AsnLength::Bits32); + let bgp_bytes = bgp_msg.encode(AsnLength::Bits32).unwrap(); // Now construct the BMP message // BMP Common Header (6 bytes) + Per-Peer Header (42 bytes) + BGP message diff --git a/examples/raw_attributes.rs b/examples/raw_attributes.rs index d6637fa..376ad0f 100644 --- a/examples/raw_attributes.rs +++ b/examples/raw_attributes.rs @@ -90,7 +90,7 @@ fn main() { ); // Encode and decode round-trip - let encoded = attributes.encode(AsnLength::Bits32); + let encoded = attributes.encode(AsnLength::Bits32).unwrap(); println!("\nEncoded size: {} bytes", encoded.len()); // Show raw access to the undecoded bytes diff --git a/examples/real_time_routeviews_kafka_to_mrt.rs b/examples/real_time_routeviews_kafka_to_mrt.rs index ae1a1fc..a8aac67 100644 --- a/examples/real_time_routeviews_kafka_to_mrt.rs +++ b/examples/real_time_routeviews_kafka_to_mrt.rs @@ -56,7 +56,7 @@ fn consume_and_archive( } }; - let bytes = mrt_record.encode(); + let bytes = mrt_record.encode().unwrap(); archive_writer.write_all(&bytes).unwrap(); archive_writer.flush().unwrap(); records_count += 1; diff --git a/src/encoder/mod.rs b/src/encoder/mod.rs index 8ffafb9..02410b2 100644 --- a/src/encoder/mod.rs +++ b/src/encoder/mod.rs @@ -1,4 +1,5 @@ mod rib_encoder; +pub(crate) mod sink; mod updates_encoder; pub use rib_encoder::MrtRibEncoder; diff --git a/src/encoder/rib_encoder.rs b/src/encoder/rib_encoder.rs index 1d38826..14d77d8 100644 --- a/src/encoder/rib_encoder.rs +++ b/src/encoder/rib_encoder.rs @@ -4,6 +4,7 @@ //! difficulty part of this process is the handling of TableDumpV2 RIB dumps, which requires //! reconstructing the peer index table before encoding all other contents. +use crate::error::EncodingError; use crate::models::{ Attributes, BgpElem, CommonHeader, EntryType, MrtMessage, NetworkPrefix, Peer, PeerIndexTable, RibAfiEntries, RibEntry, TableDumpV2Message, TableDumpV2Type, @@ -45,10 +46,14 @@ impl MrtRibEncoder { /// Processes a BgpElem and updates the internal data structures. /// + /// Returns [`EncodingError::ValueTooLarge`] if the element's peer would + /// exceed the 65535-peer capacity of the PEER_INDEX_TABLE; the encoder's + /// state is left unchanged in that case. + /// /// # Arguments /// /// * `elem` - A reference to a BgpElem that contains the information to be processed. - pub fn process_elem(&mut self, elem: &BgpElem) { + pub fn process_elem(&mut self, elem: &BgpElem) -> Result<(), EncodingError> { if self.timestamp == 0.0 { self.timestamp = elem.timestamp; } @@ -57,7 +62,7 @@ impl MrtRibEncoder { IpAddr::V6(_ip) => Ipv4Addr::from(0), }; let peer = Peer::new(bgp_identifier, elem.peer_ip, elem.peer_asn); - let peer_index = self.index_table.add_peer(peer); + let peer_index = self.index_table.add_peer(peer)?; let path_id = elem.prefix.path_id; let prefix = elem.prefix.prefix; @@ -69,6 +74,7 @@ impl MrtRibEncoder { attributes: Attributes::from(elem), }; entries_map.insert(peer_index, entry); + Ok(()) } /// Export the data stored in the struct to a byte array. @@ -78,8 +84,9 @@ impl MrtRibEncoder { /// The resulting `BytesMut` object is then converted to an immutable `Bytes` object using `freeze()` and returned. /// /// # Return - /// Returns a `Bytes` object containing the exported data as a byte array. - pub fn export_bytes(&mut self) -> Bytes { + /// Returns a `Bytes` object containing the exported data as a byte array, + /// or an [EncodingError] if any element cannot be represented in wire format. + pub fn export_bytes(&mut self) -> Result { let mut bytes = BytesMut::new(); // encode peer-index-table @@ -88,7 +95,7 @@ impl MrtRibEncoder { )); let (seconds, _microseconds) = convert_timestamp(self.timestamp); let subtype = TableDumpV2Type::PeerIndexTable as u16; - let data_bytes = mrt_message.encode(subtype); + let data_bytes = mrt_message.encode(subtype)?; let header = CommonHeader { timestamp: seconds, microsecond_timestamp: None, @@ -120,7 +127,7 @@ impl MrtRibEncoder { let (seconds, _microseconds) = convert_timestamp(self.timestamp); let subtype = rib_type as u16; - let data_bytes = mrt_message.encode(subtype); + let data_bytes = mrt_message.encode(subtype)?; let header_bytes = CommonHeader { timestamp: seconds, microsecond_timestamp: None, @@ -135,7 +142,7 @@ impl MrtRibEncoder { self.reset(); - bytes.freeze() + Ok(bytes.freeze()) } } @@ -156,10 +163,10 @@ mod tests { ..Default::default() }; elem.prefix.prefix = "10.250.0.0/24".parse().unwrap(); - encoder.process_elem(&elem); + encoder.process_elem(&elem).unwrap(); elem.prefix.prefix = "10.251.0.0/24".parse().unwrap(); - encoder.process_elem(&elem); - let bytes = encoder.export_bytes(); + encoder.process_elem(&elem).unwrap(); + let bytes = encoder.export_bytes().unwrap(); let mut cursor = Cursor::new(bytes.clone()); while cursor.has_remaining() { @@ -175,8 +182,8 @@ mod tests { }; // ipv6 prefix elem.prefix.prefix = "2001:db8::/32".parse().unwrap(); - encoder.process_elem(&elem); - let bytes = encoder.export_bytes(); + encoder.process_elem(&elem).unwrap(); + let bytes = encoder.export_bytes().unwrap(); let mut cursor = Cursor::new(bytes.clone()); while cursor.has_remaining() { @@ -193,9 +200,9 @@ mod tests { ..Default::default() }; elem.prefix = NetworkPrefix::new("10.250.0.0/24".parse().unwrap(), Some(42)); - encoder.process_elem(&elem); + encoder.process_elem(&elem).unwrap(); - let bytes = encoder.export_bytes(); + let bytes = encoder.export_bytes().unwrap(); let mut cursor = Cursor::new(bytes); let _peer_table = parse_mrt_record(&mut cursor).unwrap(); let parsed = parse_mrt_record(&mut cursor).unwrap(); diff --git a/src/encoder/sink.rs b/src/encoder/sink.rs new file mode 100644 index 0000000..aea2046 --- /dev/null +++ b/src/encoder/sink.rs @@ -0,0 +1,200 @@ +/*! +Back-patching helpers for length-prefixed wire fields. + +All "write a length, then the payload" encoding in this crate goes through +these helpers: a placeholder length is written, the payload is encoded +directly into the shared buffer, and the length is patched in afterwards with +a checked conversion. This centralizes every wire-capacity check and avoids +per-node intermediate buffers. +*/ +use crate::error::{check_max, EncodingError}; +use bytes::{BufMut, BytesMut}; + +/// Encode a payload with a 1-byte length prefix. +/// +/// Runs `f` to append the payload to `buf`, then back-patches the length. +/// Returns `EncodingError::ValueTooLarge` (naming `field`) if the payload +/// exceeds 255 bytes. +pub(crate) fn with_u8_len( + buf: &mut BytesMut, + field: &'static str, + f: impl FnOnce(&mut BytesMut) -> Result<(), EncodingError>, +) -> Result<(), EncodingError> { + let at = buf.len(); + buf.put_u8(0); + f(buf)?; + let len = buf.len() - at - 1; + check_max(field, len, u8::MAX as usize)?; + buf[at] = len as u8; + Ok(()) +} + +/// Encode a payload with a 2-byte big-endian length prefix. +/// +/// Runs `f` to append the payload to `buf`, then back-patches the length. +/// Returns `EncodingError::ValueTooLarge` (naming `field`) if the payload +/// exceeds 65535 bytes. +pub(crate) fn with_u16_len( + buf: &mut BytesMut, + field: &'static str, + f: impl FnOnce(&mut BytesMut) -> Result<(), EncodingError>, +) -> Result<(), EncodingError> { + let at = buf.len(); + buf.put_u16(0); + f(buf)?; + let len = buf.len() - at - 2; + check_max(field, len, u16::MAX as usize)?; + buf[at..at + 2].copy_from_slice(&(len as u16).to_be_bytes()); + Ok(()) +} + +/// Write `slice` preceded by its length as 1 byte. +/// +/// Returns `EncodingError::ValueTooLarge` (naming `field`) if the slice +/// exceeds 255 bytes; `buf` is not modified in that case. +pub(crate) fn put_u8_len_slice( + buf: &mut BytesMut, + field: &'static str, + slice: &[u8], +) -> Result<(), EncodingError> { + check_max(field, slice.len(), u8::MAX as usize)?; + buf.put_u8(slice.len() as u8); + buf.put_slice(slice); + Ok(()) +} + +/// Write `slice` preceded by its length as 2 big-endian bytes. +/// +/// Returns `EncodingError::ValueTooLarge` (naming `field`) if the slice +/// exceeds 65535 bytes; `buf` is not modified in that case. +pub(crate) fn put_u16_len_slice( + buf: &mut BytesMut, + field: &'static str, + slice: &[u8], +) -> Result<(), EncodingError> { + check_max(field, slice.len(), u16::MAX as usize)?; + buf.put_u16(slice.len() as u16); + buf.put_slice(slice); + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::error::check_max; + + #[test] + fn test_with_u8_len_basic() { + let mut buf = BytesMut::new(); + with_u8_len(&mut buf, "test", |b| { + b.put_slice(&[1, 2, 3]); + Ok(()) + }) + .unwrap(); + assert_eq!(&buf[..], &[3, 1, 2, 3]); + } + + #[test] + fn test_with_u8_len_empty() { + let mut buf = BytesMut::new(); + with_u8_len(&mut buf, "test", |_| Ok(())).unwrap(); + assert_eq!(&buf[..], &[0]); + } + + #[test] + fn test_with_u8_len_at_max() { + let mut buf = BytesMut::new(); + with_u8_len(&mut buf, "test", |b| { + b.put_slice(&[0xAA; 255]); + Ok(()) + }) + .unwrap(); + assert_eq!(buf[0], 255); + assert_eq!(buf.len(), 256); + } + + #[test] + fn test_with_u8_len_overflow() { + let mut buf = BytesMut::new(); + let err = with_u8_len(&mut buf, "test field", |b| { + b.put_slice(&[0xAA; 256]); + Ok(()) + }) + .unwrap_err(); + assert_eq!( + err, + EncodingError::ValueTooLarge { + field: "test field", + actual: 256, + max: 255 + } + ); + } + + #[test] + fn test_with_u16_len_basic() { + let mut buf = BytesMut::new(); + buf.put_u8(0xFF); // pre-existing content must be preserved + with_u16_len(&mut buf, "test", |b| { + b.put_slice(&[1, 2, 3]); + Ok(()) + }) + .unwrap(); + assert_eq!(&buf[..], &[0xFF, 0, 3, 1, 2, 3]); + } + + #[test] + fn test_with_u16_len_overflow() { + let mut buf = BytesMut::new(); + let err = with_u16_len(&mut buf, "test field", |b| { + b.put_slice(&vec![0xAA; 65536]); + Ok(()) + }) + .unwrap_err(); + assert_eq!( + err, + EncodingError::ValueTooLarge { + field: "test field", + actual: 65536, + max: 65535 + } + ); + } + + #[test] + fn test_nested_prefixes() { + let mut buf = BytesMut::new(); + with_u16_len(&mut buf, "outer", |b| { + with_u8_len(b, "inner", |b| { + b.put_slice(&[7, 8]); + Ok(()) + }) + }) + .unwrap(); + // outer len = 3 (inner prefix byte + 2 payload bytes) + assert_eq!(&buf[..], &[0, 3, 2, 7, 8]); + } + + #[test] + fn test_inner_error_propagates() { + let mut buf = BytesMut::new(); + let err = with_u16_len(&mut buf, "outer", |_| { + Err(EncodingError::too_large("inner", 1, 0)) + }) + .unwrap_err(); + assert_eq!( + err, + EncodingError::ValueTooLarge { + field: "inner", + actual: 1, + max: 0 + } + ); + } + + #[test] + fn test_check_max_custom_bound() { + assert_eq!(check_max("f", 4095, 0x0FFF), Ok(4095)); + assert!(check_max("f", 4096, 0x0FFF).is_err()); + } +} diff --git a/src/encoder/updates_encoder.rs b/src/encoder/updates_encoder.rs index e503829..fee1054 100644 --- a/src/encoder/updates_encoder.rs +++ b/src/encoder/updates_encoder.rs @@ -1,6 +1,7 @@ use std::net::IpAddr; use std::str::FromStr; +use crate::error::EncodingError; use crate::models::{ Asn, Bgp4MpEnum, Bgp4MpMessage, Bgp4MpType, BgpMessage, BgpUpdateMessage, CommonHeader, EntryType, MrtMessage, @@ -27,7 +28,7 @@ impl MrtUpdatesEncoder { self.cached_elems.push(elem.clone()); } - pub fn export_bytes(&mut self) -> Bytes { + pub fn export_bytes(&mut self) -> Result { let mut bytes = BytesMut::new(); for elem in &self.cached_elems { @@ -55,7 +56,7 @@ impl MrtUpdatesEncoder { let (seconds, microseconds) = convert_timestamp(elem.timestamp); let subtype = Bgp4MpType::MessageAs4 as u16; - let data_bytes = mrt_message.encode(subtype); + let data_bytes = mrt_message.encode(subtype)?; let header_bytes = CommonHeader { timestamp: seconds, microsecond_timestamp: Some(microseconds), @@ -70,7 +71,7 @@ impl MrtUpdatesEncoder { self.reset(); - bytes.freeze() + Ok(bytes.freeze()) } } @@ -94,7 +95,7 @@ mod tests { encoder.process_elem(&elem); elem.prefix.prefix = "10.251.0.0/24".parse().unwrap(); encoder.process_elem(&elem); - let bytes = encoder.export_bytes(); + let bytes = encoder.export_bytes().unwrap(); let mut cursor = Cursor::new(bytes.clone()); while cursor.has_remaining() { @@ -114,7 +115,7 @@ mod tests { // ipv6 prefix elem.prefix = NetworkPrefix::from_str("2001:db8::/32").unwrap(); encoder.process_elem(&elem); - let bytes = encoder.export_bytes(); + let bytes = encoder.export_bytes().unwrap(); let mut cursor = Cursor::new(bytes.clone()); while cursor.has_remaining() { let _parsed = parse_mrt_record(&mut cursor).unwrap(); diff --git a/src/error.rs b/src/error.rs index 2c1bcf9..606d2f6 100644 --- a/src/error.rs +++ b/src/error.rs @@ -38,6 +38,76 @@ pub enum ParserError { impl Error for ParserError {} +/// Errors that can occur when encoding BGP/MRT messages to wire format. +/// +/// These arise when in-memory data structures contain values that cannot be +/// represented in their wire-format fields (e.g. an AS_PATH segment with more +/// than 255 ASes, or an attribute value exceeding 65535 bytes). All such +/// conditions were previously handled by silently truncating — see issue #313. +#[derive(Debug, Clone, PartialEq, Eq)] +#[non_exhaustive] +pub enum EncodingError { + /// A value exceeded the maximum size that fits in its wire-format field. + /// + /// `field` names the wire field (e.g. `"AS_PATH segment count"`, + /// `"BGP attribute value length"`). `actual` is the byte/element count + /// that overflowed; `max` is the field's capacity. + ValueTooLarge { + field: &'static str, + actual: usize, + max: usize, + }, + /// The value cannot be represented in wire format at all (e.g. ATTR_SET + /// encoding is not implemented, a labeled NLRI has an empty label stack, + /// or a BGP OPEN optional parameter uses the reserved type 255). + Unencodable { field: &'static str, reason: String }, +} + +impl EncodingError { + pub(crate) fn too_large(field: &'static str, actual: usize, max: usize) -> Self { + EncodingError::ValueTooLarge { field, actual, max } + } + + pub(crate) fn unencodable(field: &'static str, reason: impl Into) -> Self { + EncodingError::Unencodable { + field, + reason: reason.into(), + } + } +} + +/// Check that `actual` fits within `max`, returning `actual` unchanged. +/// +/// This is the single place where wire-format capacity checks happen, so +/// non-power-of-two bounds (e.g. the 12-bit FlowSpec NLRI length) are an +/// explicit, greppable decision at the call site. +pub(crate) fn check_max( + field: &'static str, + actual: usize, + max: usize, +) -> Result { + if actual > max { + return Err(EncodingError::too_large(field, actual, max)); + } + Ok(actual) +} + +impl Display for EncodingError { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + match self { + EncodingError::ValueTooLarge { field, actual, max } => write!( + f, + "encoding error: {field} ({actual}) exceeds maximum ({max})" + ), + EncodingError::Unencodable { field, reason } => { + write!(f, "encoding error: {field} cannot be encoded: {reason}") + } + } + } +} + +impl Error for EncodingError {} + #[derive(Debug)] pub struct ParserErrorWithBytes { pub error: ParserError, diff --git a/src/lib.rs b/src/lib.rs index 59afdfd..dcf5945 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -357,7 +357,7 @@ bgpkit_parser::BgpkitParser::new( }); let mut mrt_writer = oneio::get_writer("as3356_mrt.gz").unwrap(); -mrt_writer.write_all(updates_encoder.export_bytes().as_ref()).unwrap(); +mrt_writer.write_all(updates_encoder.export_bytes().unwrap().as_ref()).unwrap(); drop(mrt_writer); ``` @@ -858,6 +858,9 @@ Additional known attribute type codes are raw-retained (`AttributeValue::Raw`) a html_logo_url = "https://raw.githubusercontent.com/bgpkit/assets/main/logos/icon-transparent.png", html_favicon_url = "https://raw.githubusercontent.com/bgpkit/assets/main/logos/favicon.ico" )] +// A dropped `Result` from an encode_to() call would silently skip wire data — +// exactly the corruption class the fallible encoding API exists to prevent. +#![deny(unused_must_use)] #[cfg(feature = "parser")] pub mod encoder; diff --git a/src/models/bgp/flowspec/nlri.rs b/src/models/bgp/flowspec/nlri.rs index ac16714..50d3a54 100644 --- a/src/models/bgp/flowspec/nlri.rs +++ b/src/models/bgp/flowspec/nlri.rs @@ -1,4 +1,5 @@ use super::*; +use crate::error::{check_max, EncodingError}; use crate::models::NetworkPrefix; use ipnet::IpNet; @@ -54,8 +55,12 @@ pub fn parse_flowspec_nlri(data: &[u8]) -> Result { Ok(FlowSpecNlri { components }) } +/// Maximum encodable FlowSpec NLRI length: the wire length field is 12 bits +/// (RFC 8955 §4.1 — a 2-octet length has its high nibble fixed to 0xF). +pub(crate) const FLOWSPEC_NLRI_MAX_LEN: usize = 0x0FFF; + /// Encode Flow-Spec NLRI to byte data -pub fn encode_flowspec_nlri(nlri: &FlowSpecNlri) -> Vec { +pub fn encode_flowspec_nlri(nlri: &FlowSpecNlri) -> Result, EncodingError> { let mut data = Vec::new(); // Encode each component @@ -88,11 +93,16 @@ pub fn encode_flowspec_nlri(nlri: &FlowSpecNlri) -> Vec { } } - // Prepend length + // Prepend length; the wire length field is 12-bit, not a full u16 + check_max( + "FlowSpec NLRI total length", + data.len(), + FLOWSPEC_NLRI_MAX_LEN, + )?; let mut result = Vec::new(); encode_length(data.len() as u16, &mut result); result.extend(data); - result + Ok(result) } /// Parse length field (1 or 2 octets) @@ -502,7 +512,7 @@ mod tests { FlowSpecComponent::DestinationPort(vec![NumericOperator::equal_to(80)]), ]); - let encoded = encode_flowspec_nlri(&original_nlri); + let encoded = encode_flowspec_nlri(&original_nlri).unwrap(); let parsed_nlri = parse_flowspec_nlri(&encoded).unwrap(); assert_eq!(original_nlri, parsed_nlri); diff --git a/src/models/bgp/flowspec/tests.rs b/src/models/bgp/flowspec/tests.rs index 02d32c5..911fcf4 100644 --- a/src/models/bgp/flowspec/tests.rs +++ b/src/models/bgp/flowspec/tests.rs @@ -63,7 +63,7 @@ mod rfc_examples { } // Test round-trip encoding - let encoded = encode_flowspec_nlri(&nlri); + let encoded = encode_flowspec_nlri(&nlri).unwrap(); assert_eq!(encoded, data); } @@ -326,6 +326,42 @@ mod error_handling { let parsed_length = parse_length(&data, &mut offset).unwrap(); assert_eq!(parsed_length, 4095); } + + #[test] + fn test_encode_rejects_length_beyond_12_bit_field() { + use crate::error::EncodingError; + + // Component data in the 4096..=65535 range fits a u16 but NOT the + // 12-bit wire length field: encoding it would corrupt the length byte + // (e.g. 5000 = 0x1388 encodes as 0xF3 0x88, decoding back as 904). + // 2500 numeric operators encode to 2 bytes each, plus the type byte. + let nlri = FlowSpecNlri::new(vec![FlowSpecComponent::Port(vec![ + NumericOperator::equal_to( + 80 + ); + 2500 + ])]); + let err = encode_flowspec_nlri(&nlri).unwrap_err(); + assert_eq!( + err, + EncodingError::ValueTooLarge { + field: "FlowSpec NLRI total length", + actual: 5001, + max: 0x0FFF + } + ); + + // 2000 operators (4001 bytes) still fit within the 12-bit bound + let nlri = FlowSpecNlri::new(vec![FlowSpecComponent::Port(vec![ + NumericOperator::equal_to( + 80 + ); + 2000 + ])]); + let encoded = encode_flowspec_nlri(&nlri).unwrap(); + let mut offset = 0; + assert_eq!(parse_length(&encoded, &mut offset).unwrap(), 4001); + } } /// IPv6-specific test cases from RFC 8956 @@ -726,7 +762,7 @@ mod nlri_parsing_tests { prefix, }]); - let encoded = encode_flowspec_nlri(&nlri); + let encoded = encode_flowspec_nlri(&nlri).unwrap(); // Should start with length, then type 1, then prefix len, then offset assert!(encoded.len() > 4); @@ -789,7 +825,7 @@ mod nlri_parsing_tests { FlowSpecComponent::TcpFlags(vec![bm_op1, bm_op2]), ]); - let encoded = encode_flowspec_nlri(&nlri); + let encoded = encode_flowspec_nlri(&nlri).unwrap(); let parsed = parse_flowspec_nlri(&encoded).unwrap(); // Verify round-trip encoding worked diff --git a/src/models/bgp/linkstate.rs b/src/models/bgp/linkstate.rs index 5ef4f96..ead21ad 100644 --- a/src/models/bgp/linkstate.rs +++ b/src/models/bgp/linkstate.rs @@ -183,10 +183,6 @@ impl Tlv { pub fn new(tlv_type: u16, value: Vec) -> Self { Self { tlv_type, value } } - - pub fn length(&self) -> u16 { - self.value.len() as u16 - } } /// Node Descriptor TLVs @@ -609,7 +605,6 @@ mod tests { let tlv = Tlv::new(1024, vec![0x01, 0x02, 0x03]); assert_eq!(tlv.tlv_type, 1024); assert_eq!(tlv.value, vec![0x01, 0x02, 0x03]); - assert_eq!(tlv.length(), 3); } #[test] diff --git a/src/models/bgp/tunnel_encap.rs b/src/models/bgp/tunnel_encap.rs index 7e2e57d..7204da7 100644 --- a/src/models/bgp/tunnel_encap.rs +++ b/src/models/bgp/tunnel_encap.rs @@ -124,10 +124,6 @@ impl SubTlv { value, } } - - pub fn length(&self) -> u16 { - self.value.len() as u16 - } } /// Tunnel Encapsulation TLV @@ -336,9 +332,10 @@ mod tests { } #[test] - fn test_sub_tlv_length() { + fn test_sub_tlv_creation() { let sub_tlv = SubTlv::new(SubTlvType::Color, vec![0x00, 0x00, 0x00, 0x64]); - assert_eq!(sub_tlv.length(), 4); + assert_eq!(sub_tlv.sub_tlv_type, SubTlvType::Color); + assert_eq!(sub_tlv.value.len(), 4); } #[test] diff --git a/src/parser/bgp/attributes/attr_02_17_as_path.rs b/src/parser/bgp/attributes/attr_02_17_as_path.rs index dd598d6..90c2551 100644 --- a/src/parser/bgp/attributes/attr_02_17_as_path.rs +++ b/src/parser/bgp/attributes/attr_02_17_as_path.rs @@ -1,3 +1,4 @@ +use crate::error::{check_max, EncodingError}; use crate::models::*; use crate::parser::ReadUtils; use crate::ParserError; @@ -54,33 +55,24 @@ fn parse_as_path_segment( } } -pub fn encode_as_path(path: &AsPath, asn_len: AsnLength) -> Bytes { - let mut output = BytesMut::with_capacity(1024); +pub fn encode_as_path( + path: &AsPath, + asn_len: AsnLength, + output: &mut BytesMut, +) -> Result<(), EncodingError> { for segment in path.segments.iter() { - match segment { - AsPathSegment::AsSet(asns) => { - output.put_u8(AS_PATH_AS_SET); - output.put_u8(asns.len() as u8); - write_asns(asns, asn_len, &mut output); - } - AsPathSegment::AsSequence(asns) => { - output.put_u8(AS_PATH_AS_SEQUENCE); - output.put_u8(asns.len() as u8); - write_asns(asns, asn_len, &mut output); - } - AsPathSegment::ConfedSequence(asns) => { - output.put_u8(AS_PATH_CONFED_SEQUENCE); - output.put_u8(asns.len() as u8); - write_asns(asns, asn_len, &mut output); - } - AsPathSegment::ConfedSet(asns) => { - output.put_u8(AS_PATH_CONFED_SET); - output.put_u8(asns.len() as u8); - write_asns(asns, asn_len, &mut output); - } - } + let (segment_type, asns) = match segment { + AsPathSegment::AsSet(asns) => (AS_PATH_AS_SET, asns), + AsPathSegment::AsSequence(asns) => (AS_PATH_AS_SEQUENCE, asns), + AsPathSegment::ConfedSequence(asns) => (AS_PATH_CONFED_SEQUENCE, asns), + AsPathSegment::ConfedSet(asns) => (AS_PATH_CONFED_SET, asns), + }; + check_max("AS_PATH segment count", asns.len(), u8::MAX as usize)?; + output.put_u8(segment_type); + output.put_u8(asns.len() as u8); + write_asns(asns, asn_len, output); } - output.freeze() + Ok(()) } fn write_asns(asns: &[Asn], asn_len: AsnLength, output: &mut BytesMut) { @@ -222,7 +214,8 @@ mod tests { 0, 3, // AS3 ]); let path = parse_as_path(data.clone(), &AsnLength::Bits16).unwrap(); - let encoded_bytes = encode_as_path(&path, AsnLength::Bits16); + let mut encoded_bytes = BytesMut::new(); + encode_as_path(&path, AsnLength::Bits16, &mut encoded_bytes).unwrap(); assert_eq!(data, encoded_bytes); let data = Bytes::from(vec![ @@ -233,10 +226,28 @@ mod tests { 0, 0, 0, 3, // AS3 ]); let path = parse_as_path(data.clone(), &AsnLength::Bits32).unwrap(); - let encoded_bytes = encode_as_path(&path, AsnLength::Bits32); + let mut encoded_bytes = BytesMut::new(); + encode_as_path(&path, AsnLength::Bits32, &mut encoded_bytes).unwrap(); assert_eq!(data, encoded_bytes); } + #[test] + fn test_encode_as_path_oversized_segment() { + let path = AsPath::from_segments(vec![AsPathSegment::AsSequence( + (0..256u32).map(Asn::from).collect(), + )]); + let mut buf = BytesMut::new(); + let err = encode_as_path(&path, AsnLength::Bits32, &mut buf).unwrap_err(); + assert_eq!( + err, + EncodingError::ValueTooLarge { + field: "AS_PATH segment count", + actual: 256, + max: 255 + } + ); + } + #[test] fn test_encode_confed() { let data = Bytes::from(vec![ @@ -245,7 +256,8 @@ mod tests { 0, 1, ]); let path = parse_as_path(data.clone(), &AsnLength::Bits16).unwrap(); - let encoded_bytes = encode_as_path(&path, AsnLength::Bits16); + let mut encoded_bytes = BytesMut::new(); + encode_as_path(&path, AsnLength::Bits16, &mut encoded_bytes).unwrap(); assert_eq!(data, encoded_bytes); let data = Bytes::from(vec![ @@ -254,7 +266,8 @@ mod tests { 0, 1, ]); let path = parse_as_path(data.clone(), &AsnLength::Bits16).unwrap(); - let encoded_bytes = encode_as_path(&path, AsnLength::Bits16); + let mut encoded_bytes = BytesMut::new(); + encode_as_path(&path, AsnLength::Bits16, &mut encoded_bytes).unwrap(); assert_eq!(data, encoded_bytes); } diff --git a/src/parser/bgp/attributes/attr_14_15_nlri.rs b/src/parser/bgp/attributes/attr_14_15_nlri.rs index 83eca5c..35ca1f9 100644 --- a/src/parser/bgp/attributes/attr_14_15_nlri.rs +++ b/src/parser/bgp/attributes/attr_14_15_nlri.rs @@ -1,3 +1,5 @@ +use crate::encoder::sink::put_u8_len_slice; +use crate::error::EncodingError; use crate::models::*; use crate::parser::bgp::attributes::attr_03_next_hop::parse_mp_next_hop; use crate::parser::bgp::attributes::attr_29_linkstate::parse_link_state_nlri; @@ -152,9 +154,12 @@ pub fn parse_nlri( /// * `reachable` - Whether this is MP_REACH_NLRI (true) or MP_UNREACH_NLRI (false) /// * `add_path` - Whether ADD-PATH capability was negotiated (RFC 7911). If true, /// path_id will be encoded; if false, prefixes with path_id will cause an error. -pub fn encode_nlri(nlri: &Nlri, reachable: bool, add_path: bool) -> Result { - let mut bytes = BytesMut::new(); - +pub fn encode_nlri( + nlri: &Nlri, + reachable: bool, + add_path: bool, + bytes: &mut BytesMut, +) -> Result<(), EncodingError> { // encode address family bytes.put_u16(nlri.afi as u16); bytes.put_u8(nlri.safi as u8); @@ -187,8 +192,8 @@ pub fn encode_nlri(nlri: &Nlri, reachable: bool, add_path: bool) -> Result Result Result Result Bytes { - let mut bytes = BytesMut::new(); - +pub fn encode_tunnel_encapsulation_attribute( + attr: &TunnelEncapAttribute, + bytes: &mut BytesMut, +) -> Result<(), EncodingError> { for tunnel_tlv in &attr.tunnel_tlvs { // Encode tunnel type bytes.put_u16(tunnel_tlv.tunnel_type as u16); - // Encode sub-TLVs first to calculate total length - let mut sub_tlv_bytes = BytesMut::new(); - for sub_tlv in &tunnel_tlv.sub_tlvs { - let sub_tlv_type = sub_tlv.sub_tlv_type as u16; - - // Encode sub-TLV type - if sub_tlv_type < 128 { - sub_tlv_bytes.put_u8(sub_tlv_type as u8); - sub_tlv_bytes.put_u8(sub_tlv.value.len() as u8); - } else { - sub_tlv_bytes.put_u8(sub_tlv_type as u8); - sub_tlv_bytes.put_u16(sub_tlv.value.len() as u16); + // Encode tunnel length, then sub-TLVs + with_u16_len(bytes, "Tunnel Encap TLV length", |b| { + for sub_tlv in &tunnel_tlv.sub_tlvs { + let sub_tlv_type = sub_tlv.sub_tlv_type as u16; + b.put_u8(sub_tlv_type as u8); + + // RFC 9012: sub-TLV types < 128 use a 1-octet length field, + // types >= 128 use a 2-octet length field + if sub_tlv_type < 128 { + put_u8_len_slice(b, "Tunnel Encap sub-TLV value length", &sub_tlv.value)?; + } else { + put_u16_len_slice(b, "Tunnel Encap sub-TLV value length", &sub_tlv.value)?; + } } - - // Encode sub-TLV value - sub_tlv_bytes.extend_from_slice(&sub_tlv.value); - } - - // Encode tunnel length - bytes.put_u16(sub_tlv_bytes.len() as u16); - - // Append sub-TLV data - bytes.extend_from_slice(&sub_tlv_bytes); + Ok(()) + })?; } - bytes.freeze() + Ok(()) } #[cfg(test)] @@ -231,10 +226,11 @@ mod tests { attr.add_tunnel_tlv(tunnel_tlv); - let encoded = encode_tunnel_encapsulation_attribute(&attr); + let mut encoded = BytesMut::new(); + encode_tunnel_encapsulation_attribute(&attr, &mut encoded).unwrap(); // Should encode back to the same format we can parse - let parsed = parse_tunnel_encapsulation_attribute(encoded).unwrap(); + let parsed = parse_tunnel_encapsulation_attribute(encoded.freeze()).unwrap(); if let AttributeValue::TunnelEncapsulation(parsed_attr) = parsed { assert_eq!(parsed_attr.tunnel_tlvs.len(), 1); diff --git a/src/parser/bgp/attributes/attr_29_linkstate.rs b/src/parser/bgp/attributes/attr_29_linkstate.rs index 7350db5..f2dcd9d 100644 --- a/src/parser/bgp/attributes/attr_29_linkstate.rs +++ b/src/parser/bgp/attributes/attr_29_linkstate.rs @@ -3,7 +3,8 @@ use bytes::{Buf, BufMut, Bytes, BytesMut}; use std::net::{Ipv4Addr, Ipv6Addr}; -use crate::error::ParserError; +use crate::encoder::sink::put_u16_len_slice; +use crate::error::{EncodingError, ParserError}; use crate::models::*; use crate::parser::ReadUtils; @@ -432,41 +433,37 @@ fn parse_ip_prefix_from_bytes(data: &[u8]) -> Result } /// Encode BGP Link-State attribute -pub fn encode_link_state_attribute(attr: &LinkStateAttribute) -> Bytes { - let mut bytes = BytesMut::new(); - - // Encode node attributes - for (attr_type, value) in &attr.node_attributes { - let type_code = u16::from(*attr_type); - bytes.put_u16(type_code); - bytes.put_u16(value.len() as u16); - bytes.extend_from_slice(value); - } - - // Encode link attributes - for (attr_type, value) in &attr.link_attributes { - let type_code = u16::from(*attr_type); - bytes.put_u16(type_code); - bytes.put_u16(value.len() as u16); - bytes.extend_from_slice(value); - } - - // Encode prefix attributes - for (attr_type, value) in &attr.prefix_attributes { - let type_code = u16::from(*attr_type); +pub fn encode_link_state_attribute( + attr: &LinkStateAttribute, + bytes: &mut BytesMut, +) -> Result<(), EncodingError> { + let typed_tlvs = attr + .node_attributes + .iter() + .map(|(attr_type, value)| (u16::from(*attr_type), value)) + .chain( + attr.link_attributes + .iter() + .map(|(attr_type, value)| (u16::from(*attr_type), value)), + ) + .chain( + attr.prefix_attributes + .iter() + .map(|(attr_type, value)| (u16::from(*attr_type), value)), + ); + + for (type_code, value) in typed_tlvs { bytes.put_u16(type_code); - bytes.put_u16(value.len() as u16); - bytes.extend_from_slice(value); + put_u16_len_slice(bytes, "BGP-LS TLV value length", value)?; } // Encode unknown attributes for tlv in &attr.unknown_attributes { bytes.put_u16(tlv.tlv_type); - bytes.put_u16(tlv.length()); - bytes.extend_from_slice(&tlv.value); + put_u16_len_slice(bytes, "BGP-LS TLV value length", &tlv.value)?; } - bytes.freeze() + Ok(()) } #[cfg(test)] @@ -531,7 +528,8 @@ mod tests { let mut attr = LinkStateAttribute::new(); attr.add_node_attribute(NodeAttributeType::NodeName, b"router1".to_vec()); - let encoded = encode_link_state_attribute(&attr); + let mut encoded = BytesMut::new(); + encode_link_state_attribute(&attr, &mut encoded).unwrap(); assert!(!encoded.is_empty()); // Should contain the node name TLV diff --git a/src/parser/bgp/attributes/attr_37_sfp.rs b/src/parser/bgp/attributes/attr_37_sfp.rs index e792b0d..0ceb0dc 100644 --- a/src/parser/bgp/attributes/attr_37_sfp.rs +++ b/src/parser/bgp/attributes/attr_37_sfp.rs @@ -1,3 +1,5 @@ +use crate::encoder::sink::put_u16_len_slice; +use crate::error::EncodingError; use crate::models::*; use crate::parser::ReadUtils; use crate::ParserError; @@ -27,14 +29,12 @@ pub fn parse_sfp(mut input: Bytes) -> Result { Ok(AttributeValue::Sfp(SfpAttribute { tlvs })) } -pub fn encode_sfp(attr: &SfpAttribute) -> Bytes { - let mut buf = BytesMut::new(); +pub fn encode_sfp(attr: &SfpAttribute, buf: &mut BytesMut) -> Result<(), EncodingError> { for tlv in &attr.tlvs { buf.put_u8(tlv.tlv_type); - buf.put_u16(tlv.value.len() as u16); - buf.extend_from_slice(&tlv.value); + put_u16_len_slice(buf, "SFP TLV value length", &tlv.value)?; } - buf.freeze() + Ok(()) } #[cfg(test)] @@ -53,7 +53,9 @@ mod tests { attr.tlvs[0].value, Bytes::from_static(&[0xaa, 0xbb, 0xcc, 0xdd]) ); - assert_eq!(encode_sfp(&attr), input); + let mut buf = BytesMut::new(); + encode_sfp(&attr, &mut buf).unwrap(); + assert_eq!(buf.freeze(), input); } value => panic!("expected SFP, got {value:?}"), } @@ -67,7 +69,9 @@ mod tests { AttributeValue::Sfp(attr) => { assert_eq!(attr.tlvs[0].tlv_type, 0x7f); assert_eq!(attr.tlvs[0].value, Bytes::from_static(&[0xde, 0xad])); - assert_eq!(encode_sfp(&attr), input); + let mut buf = BytesMut::new(); + encode_sfp(&attr, &mut buf).unwrap(); + assert_eq!(buf.freeze(), input); } value => panic!("expected SFP, got {value:?}"), } @@ -79,7 +83,9 @@ mod tests { match value { AttributeValue::Sfp(attr) => { assert!(attr.tlvs.is_empty()); - assert_eq!(encode_sfp(&attr), Bytes::new()); + let mut buf = BytesMut::new(); + encode_sfp(&attr, &mut buf).unwrap(); + assert!(buf.is_empty()); } value => panic!("expected SFP, got {value:?}"), } diff --git a/src/parser/bgp/attributes/attr_38_bfd_discriminator.rs b/src/parser/bgp/attributes/attr_38_bfd_discriminator.rs index 7ccf748..7323bb7 100644 --- a/src/parser/bgp/attributes/attr_38_bfd_discriminator.rs +++ b/src/parser/bgp/attributes/attr_38_bfd_discriminator.rs @@ -1,3 +1,5 @@ +use crate::encoder::sink::put_u8_len_slice; +use crate::error::EncodingError; use crate::models::*; use crate::parser::ReadUtils; use crate::ParserError; @@ -41,16 +43,17 @@ pub fn parse_bfd_discriminator(mut input: Bytes) -> Result Bytes { - let mut buf = BytesMut::new(); +pub fn encode_bfd_discriminator( + attr: &BfdDiscriminatorAttribute, + buf: &mut BytesMut, +) -> Result<(), EncodingError> { buf.put_u8(attr.mode); buf.put_u32(attr.discriminator); for tlv in &attr.tlvs { buf.put_u8(tlv.tlv_type); - buf.put_u8(tlv.value.len() as u8); - buf.extend_from_slice(&tlv.value); + put_u8_len_slice(buf, "BFD Discriminator TLV value length", &tlv.value)?; } - buf.freeze() + Ok(()) } #[cfg(test)] @@ -71,7 +74,9 @@ mod tests { assert_eq!(attr.tlvs.len(), 1); assert_eq!(attr.tlvs[0].tlv_type, 1); assert_eq!(attr.tlvs[0].value, Bytes::from_static(&[192, 0, 2, 1])); - assert_eq!(encode_bfd_discriminator(&attr), input); + let mut buf = BytesMut::new(); + encode_bfd_discriminator(&attr, &mut buf).unwrap(); + assert_eq!(buf.freeze(), input); } value => panic!("expected BFD Discriminator, got {value:?}"), } @@ -86,7 +91,9 @@ mod tests { assert_eq!(attr.mode, 1); assert_eq!(attr.discriminator, 0x01020304); assert!(attr.tlvs.is_empty()); - assert_eq!(encode_bfd_discriminator(&attr), input); + let mut buf = BytesMut::new(); + encode_bfd_discriminator(&attr, &mut buf).unwrap(); + assert_eq!(buf.freeze(), input); } value => panic!("expected BFD Discriminator, got {value:?}"), } diff --git a/src/parser/bgp/attributes/attr_40_bgp_prefix_sid.rs b/src/parser/bgp/attributes/attr_40_bgp_prefix_sid.rs index b65b6a9..61d9dda 100644 --- a/src/parser/bgp/attributes/attr_40_bgp_prefix_sid.rs +++ b/src/parser/bgp/attributes/attr_40_bgp_prefix_sid.rs @@ -1,3 +1,5 @@ +use crate::encoder::sink::put_u16_len_slice; +use crate::error::EncodingError; use crate::models::*; use crate::parser::ReadUtils; use crate::ParserError; @@ -27,14 +29,15 @@ pub fn parse_bgp_prefix_sid(mut input: Bytes) -> Result Bytes { - let mut buf = BytesMut::new(); +pub fn encode_bgp_prefix_sid( + attr: &BgpPrefixSidAttribute, + buf: &mut BytesMut, +) -> Result<(), EncodingError> { for tlv in &attr.tlvs { buf.put_u8(tlv.tlv_type); - buf.put_u16(tlv.value.len() as u16); - buf.extend_from_slice(&tlv.value); + put_u16_len_slice(buf, "BGP Prefix-SID TLV value length", &tlv.value)?; } - buf.freeze() + Ok(()) } #[cfg(test)] @@ -55,7 +58,9 @@ mod tests { assert_eq!(attr.tlvs.len(), 1); assert_eq!(attr.tlvs[0].tlv_type, 1); assert_eq!(attr.tlvs[0].value.len(), 7); - assert_eq!(encode_bgp_prefix_sid(&attr), input); + let mut buf = BytesMut::new(); + encode_bgp_prefix_sid(&attr, &mut buf).unwrap(); + assert_eq!(buf.freeze(), input); } value => panic!("expected Prefix-SID, got {value:?}"), } @@ -69,7 +74,9 @@ mod tests { AttributeValue::BgpPrefixSid(attr) => { assert_eq!(attr.tlvs[0].tlv_type, 0x7f); assert_eq!(attr.tlvs[0].value, Bytes::from_static(&[0xaa, 0xbb])); - assert_eq!(encode_bgp_prefix_sid(&attr), input); + let mut buf = BytesMut::new(); + encode_bgp_prefix_sid(&attr, &mut buf).unwrap(); + assert_eq!(buf.freeze(), input); } value => panic!("expected Prefix-SID, got {value:?}"), } @@ -81,7 +88,9 @@ mod tests { match value { AttributeValue::BgpPrefixSid(attr) => { assert!(attr.tlvs.is_empty()); - assert_eq!(encode_bgp_prefix_sid(&attr), Bytes::new()); + let mut buf = BytesMut::new(); + encode_bgp_prefix_sid(&attr, &mut buf).unwrap(); + assert!(buf.is_empty()); } value => panic!("expected Prefix-SID, got {value:?}"), } diff --git a/src/parser/bgp/attributes/attr_41_bier.rs b/src/parser/bgp/attributes/attr_41_bier.rs index d00d565..c4e4ae0 100644 --- a/src/parser/bgp/attributes/attr_41_bier.rs +++ b/src/parser/bgp/attributes/attr_41_bier.rs @@ -1,3 +1,5 @@ +use crate::encoder::sink::put_u16_len_slice; +use crate::error::EncodingError; use crate::models::*; use crate::parser::ReadUtils; use crate::ParserError; @@ -27,14 +29,12 @@ pub fn parse_bier(mut input: Bytes) -> Result { Ok(AttributeValue::Bier(BierAttribute { tlvs })) } -pub fn encode_bier(attr: &BierAttribute) -> Bytes { - let mut buf = BytesMut::new(); +pub fn encode_bier(attr: &BierAttribute, buf: &mut BytesMut) -> Result<(), EncodingError> { for tlv in &attr.tlvs { buf.put_u16(tlv.tlv_type); - buf.put_u16(tlv.value.len() as u16); - buf.extend_from_slice(&tlv.value); + put_u16_len_slice(buf, "BIER TLV value length", &tlv.value)?; } - buf.freeze() + Ok(()) } #[cfg(test)] @@ -54,7 +54,9 @@ mod tests { assert_eq!(attr.tlvs.len(), 1); assert_eq!(attr.tlvs[0].tlv_type, 1); assert_eq!(attr.tlvs[0].value, Bytes::from_static(&[0xaa, 0xbb, 0xcc])); - assert_eq!(encode_bier(&attr), input); + let mut buf = BytesMut::new(); + encode_bier(&attr, &mut buf).unwrap(); + assert_eq!(buf.freeze(), input); } value => panic!("expected BIER, got {value:?}"), } @@ -68,7 +70,9 @@ mod tests { AttributeValue::Bier(attr) => { assert_eq!(attr.tlvs[0].tlv_type, 0x1234); assert_eq!(attr.tlvs[0].value, Bytes::from_static(&[0xde, 0xad])); - assert_eq!(encode_bier(&attr), input); + let mut buf = BytesMut::new(); + encode_bier(&attr, &mut buf).unwrap(); + assert_eq!(buf.freeze(), input); } value => panic!("expected BIER, got {value:?}"), } @@ -80,7 +84,9 @@ mod tests { match value { AttributeValue::Bier(attr) => { assert!(attr.tlvs.is_empty()); - assert_eq!(encode_bier(&attr), Bytes::new()); + let mut buf = BytesMut::new(); + encode_bier(&attr, &mut buf).unwrap(); + assert!(buf.is_empty()); } value => panic!("expected BIER, got {value:?}"), } diff --git a/src/parser/bgp/attributes/mod.rs b/src/parser/bgp/attributes/mod.rs index 054b346..4004106 100644 --- a/src/parser/bgp/attributes/mod.rs +++ b/src/parser/bgp/attributes/mod.rs @@ -25,7 +25,8 @@ use std::net::IpAddr; use crate::models::*; -use crate::error::{BgpValidationWarning, ParserError}; +use crate::encoder::sink::{with_u16_len, with_u8_len}; +use crate::error::{BgpValidationWarning, EncodingError, ParserError}; use crate::parser::bgp::attributes::attr_01_origin::{encode_origin, parse_origin}; use crate::parser::bgp::attributes::attr_02_17_as_path::encode_as_path; pub(crate) use crate::parser::bgp::attributes::attr_02_17_as_path::parse_as_path; @@ -499,98 +500,113 @@ pub fn parse_attributes( } impl Attribute { - pub fn encode(&self, asn_len: AsnLength) -> Bytes { - let mut bytes = BytesMut::new(); - - let flag = self.flag.bits(); - let type_code = self.value.attr_code(); - - bytes.put_u8(flag); - bytes.put_u8(type_code); - - let value_bytes = match &self.value { - AttributeValue::Origin(v) => encode_origin(v), - AttributeValue::AsPath { path, is_as4 } => { - let four_byte = match is_as4 { - true => AsnLength::Bits32, - false => match asn_len.is_four_byte() { + /// Append the wire representation of this attribute to `buf`. + /// + /// The length-field width follows the attribute's EXTENDED_LENGTH flag; a + /// value that does not fit the resulting field yields + /// [`EncodingError::ValueTooLarge`] instead of being truncated. + pub fn encode_to(&self, asn_len: AsnLength, buf: &mut BytesMut) -> Result<(), EncodingError> { + buf.put_u8(self.flag.bits()); + buf.put_u8(self.value.attr_code()); + + let write_value = |b: &mut BytesMut| -> Result<(), EncodingError> { + match &self.value { + AttributeValue::Origin(v) => b.put_slice(&encode_origin(v)), + AttributeValue::AsPath { path, is_as4 } => { + let four_byte = match is_as4 { true => AsnLength::Bits32, - false => AsnLength::Bits16, - }, - }; - encode_as_path(path, four_byte) - } - AttributeValue::NextHop(v) => encode_next_hop(v), - AttributeValue::MultiExitDiscriminator(v) => encode_med(*v), - AttributeValue::LocalPreference(v) => encode_local_pref(*v), - AttributeValue::OnlyToCustomer(v) => encode_only_to_customer(v.into()), - AttributeValue::AtomicAggregate => Bytes::default(), - AttributeValue::Aggregator { asn, id, is_as4: _ } => { - encode_aggregator(asn, &IpAddr::from(*id)) - } - AttributeValue::Communities(v) => encode_regular_communities(v), - AttributeValue::ExtendedCommunities(v) => encode_extended_communities(v), - AttributeValue::LargeCommunities(v) => encode_large_communities(v), - AttributeValue::Ipv6AddressSpecificExtendedCommunities(v) => { - encode_ipv6_extended_communities(v) - } - AttributeValue::OriginatorId(v) => encode_originator_id(&IpAddr::from(*v)), - AttributeValue::Clusters(v) => encode_clusters(v), - AttributeValue::MpReachNlri(v) => { - // Infer ADD-PATH from presence of path_id in any labeled prefix - let add_path = v - .labeled_prefixes - .as_ref() - .is_some_and(|prefixes| prefixes.iter().any(|p| p.path_id.is_some())); - encode_nlri(v, true, add_path).unwrap_or_else(|e| { - log::warn!("Failed to encode MP_REACH_NLRI: {}", e); - Bytes::new() - }) - } - AttributeValue::MpUnreachNlri(v) => { - // Withdrawals don't use ADD-PATH encoding per RFC 8277 - encode_nlri(v, false, false).unwrap_or_else(|e| { - log::warn!("Failed to encode MP_UNREACH_NLRI: {}", e); - Bytes::new() - }) - } - AttributeValue::LinkState(v) => encode_link_state_attribute(v), - AttributeValue::TunnelEncapsulation(v) => encode_tunnel_encapsulation_attribute(v), - AttributeValue::BfdDiscriminator(v) => encode_bfd_discriminator(v), - AttributeValue::BgpPrefixSid(v) => encode_bgp_prefix_sid(v), - AttributeValue::Bier(v) => encode_bier(v), - AttributeValue::Sfp(v) => encode_sfp(v), - AttributeValue::Development(v) => Bytes::copy_from_slice(v), - AttributeValue::Raw(v) => v.bytes.clone(), - AttributeValue::Deprecated(v) => v.bytes.clone(), - AttributeValue::Unknown(v) => v.bytes.clone(), - AttributeValue::Aigp(v) => encode_aigp(v), - AttributeValue::AttrSet(_v) => { - // ATTR_SET encoding not yet implemented - return empty bytes - Bytes::new() + false => match asn_len.is_four_byte() { + true => AsnLength::Bits32, + false => AsnLength::Bits16, + }, + }; + encode_as_path(path, four_byte, b)?; + } + AttributeValue::NextHop(v) => b.put_slice(&encode_next_hop(v)), + AttributeValue::MultiExitDiscriminator(v) => b.put_slice(&encode_med(*v)), + AttributeValue::LocalPreference(v) => b.put_slice(&encode_local_pref(*v)), + AttributeValue::OnlyToCustomer(v) => { + b.put_slice(&encode_only_to_customer(v.into())) + } + AttributeValue::AtomicAggregate => {} + AttributeValue::Aggregator { asn, id, is_as4: _ } => { + b.put_slice(&encode_aggregator(asn, &IpAddr::from(*id))) + } + AttributeValue::Communities(v) => b.put_slice(&encode_regular_communities(v)), + AttributeValue::ExtendedCommunities(v) => { + b.put_slice(&encode_extended_communities(v)) + } + AttributeValue::LargeCommunities(v) => b.put_slice(&encode_large_communities(v)), + AttributeValue::Ipv6AddressSpecificExtendedCommunities(v) => { + b.put_slice(&encode_ipv6_extended_communities(v)) + } + AttributeValue::OriginatorId(v) => { + b.put_slice(&encode_originator_id(&IpAddr::from(*v))) + } + AttributeValue::Clusters(v) => b.put_slice(&encode_clusters(v)), + AttributeValue::MpReachNlri(v) => { + // Infer ADD-PATH from presence of path_id in any labeled prefix + let add_path = v + .labeled_prefixes + .as_ref() + .is_some_and(|prefixes| prefixes.iter().any(|p| p.path_id.is_some())); + encode_nlri(v, true, add_path, b)?; + } + AttributeValue::MpUnreachNlri(v) => { + // Withdrawals don't use ADD-PATH encoding per RFC 8277 + encode_nlri(v, false, false, b)?; + } + AttributeValue::LinkState(v) => encode_link_state_attribute(v, b)?, + AttributeValue::TunnelEncapsulation(v) => { + encode_tunnel_encapsulation_attribute(v, b)? + } + AttributeValue::BfdDiscriminator(v) => encode_bfd_discriminator(v, b)?, + AttributeValue::BgpPrefixSid(v) => encode_bgp_prefix_sid(v, b)?, + AttributeValue::Bier(v) => encode_bier(v, b)?, + AttributeValue::Sfp(v) => encode_sfp(v, b)?, + AttributeValue::Development(v) => b.put_slice(v), + AttributeValue::Raw(v) => b.put_slice(&v.bytes), + AttributeValue::Deprecated(v) => b.put_slice(&v.bytes), + AttributeValue::Unknown(v) => b.put_slice(&v.bytes), + AttributeValue::Aigp(v) => b.put_slice(&encode_aigp(v)), + AttributeValue::AttrSet(_v) => { + return Err(EncodingError::unencodable( + "ATTR_SET attribute", + "encoding not implemented", + )); + } } + Ok(()) }; match self.is_extended() { - false => { - bytes.put_u8(value_bytes.len() as u8); - } - true => { - bytes.put_u16(value_bytes.len() as u16); - } + false => with_u8_len(buf, "BGP attribute value length", write_value), + true => with_u16_len(buf, "BGP attribute value length (extended)", write_value), } - bytes.extend(value_bytes); - bytes.freeze() + } + + /// Encode this attribute into a fresh buffer. + pub fn encode(&self, asn_len: AsnLength) -> Result { + let mut buf = BytesMut::new(); + self.encode_to(asn_len, &mut buf)?; + Ok(buf.freeze()) } } impl Attributes { - pub fn encode(&self, asn_len: AsnLength) -> Bytes { - let mut bytes = BytesMut::new(); + /// Append the wire representation of all attributes to `buf`. + pub fn encode_to(&self, asn_len: AsnLength, buf: &mut BytesMut) -> Result<(), EncodingError> { for attr in &self.inner { - bytes.extend(attr.encode(asn_len)); + attr.encode_to(asn_len, buf)?; } - bytes.freeze() + Ok(()) + } + + /// Encode all attributes into a fresh buffer. + pub fn encode(&self, asn_len: AsnLength) -> Result { + let mut buf = BytesMut::new(); + self.encode_to(asn_len, &mut buf)?; + Ok(buf.freeze()) } } @@ -828,7 +844,7 @@ mod tests { value => panic!("expected Raw, got {value:?}"), } assert_eq!( - attributes.encode(AsnLength::Bits16), + attributes.encode(AsnLength::Bits16).unwrap(), Bytes::from_static(&[0x80, 0x16, 0x03, 0xaa, 0xbb, 0xcc]) ); } @@ -849,7 +865,7 @@ mod tests { value => panic!("expected Deprecated, got {value:?}"), } assert_eq!( - attributes.encode(AsnLength::Bits16), + attributes.encode(AsnLength::Bits16).unwrap(), Bytes::from_static(&[0x80, 0x0d, 0x04, 0x01, 0x02, 0x03, 0x04]) ); } @@ -870,7 +886,7 @@ mod tests { value => panic!("expected Raw fallback, got {value:?}"), } assert_eq!( - attributes.encode(AsnLength::Bits16), + attributes.encode(AsnLength::Bits16).unwrap(), Bytes::from_static(&[0x40, 0x03, 0x03, 0x01, 0x02, 0x03]) ); } @@ -899,7 +915,10 @@ mod tests { value => panic!("expected Raw for code {code}, got {value:?}"), } assert!(attributes.has_attr(AttrType::from(code)), "code {code}"); - assert_eq!(attributes.encode(AsnLength::Bits16), Bytes::from(wire)); + assert_eq!( + attributes.encode(AsnLength::Bits16).unwrap(), + Bytes::from(wire) + ); } } @@ -926,7 +945,10 @@ mod tests { value => panic!("expected Unknown, got {value:?}"), } assert!(attributes.has_attr(AttrType::Unknown(0x7f))); - assert_eq!(attributes.encode(AsnLength::Bits16), Bytes::from(wire)); + assert_eq!( + attributes.encode(AsnLength::Bits16).unwrap(), + Bytes::from(wire) + ); } #[test] @@ -960,7 +982,7 @@ mod tests { (_, value) => panic!("unexpected value for {name}: {value:?}"), } assert_eq!( - attributes.encode(AsnLength::Bits16), + attributes.encode(AsnLength::Bits16).unwrap(), Bytes::from(wire), "{name}" ); diff --git a/src/parser/bgp/messages.rs b/src/parser/bgp/messages.rs index 6ac41a2..dd39740 100644 --- a/src/parser/bgp/messages.rs +++ b/src/parser/bgp/messages.rs @@ -3,7 +3,8 @@ use bytes::{Buf, BufMut, Bytes, BytesMut}; use std::convert::TryFrom; use std::net::Ipv4Addr; -use crate::error::{BgpValidationWarning, ParserError}; +use crate::encoder::sink::{put_u16_len_slice, put_u8_len_slice, with_u16_len}; +use crate::error::{check_max, BgpValidationWarning, EncodingError, ParserError}; use crate::models::capabilities::{ AddPathCapability, BgpCapabilityType, BgpExtendedMessageCapability, BgpRoleCapability, ExtendedNextHopCapability, FourOctetAsCapability, GracefulRestartCapability, @@ -382,7 +383,7 @@ pub fn parse_bgp_open_message(input: &mut Bytes) -> Result Bytes { +fn encode_bgp_open_param_value(param: &OptParam) -> Result { let mut buf = BytesMut::new(); match ¶m.param_value { ParamValue::Capacities(capacities) => { @@ -399,24 +400,32 @@ fn encode_bgp_open_param_value(param: &OptParam) -> Bytes { CapabilityValue::BgpExtendedMessage(bem) => bem.encode(), CapabilityValue::Raw(raw) => Bytes::from(raw.clone()), }; - let capability_len = u8::try_from(encoded_value.len()) - .expect("BGP capability value length exceeds 255 octets"); - buf.put_u8(capability_len); - buf.put_slice(&encoded_value); + put_u8_len_slice(&mut buf, "BGP capability value length", &encoded_value)?; } } ParamValue::Raw(bytes) => buf.put_slice(bytes), } - buf.freeze() + Ok(buf.freeze()) } impl BgpOpenMessage { - pub fn encode(&self) -> Bytes { + pub fn encode(&self) -> Result { let encoded_params: Vec<(u8, Bytes)> = self .opt_params .iter() - .map(|param| (param.param_type, encode_bgp_open_param_value(param))) - .collect(); + .map(|param| { + // RFC 9072 reserves optional parameter type 255 as the + // extended-length marker; emitting it as a real parameter + // would be misparsed on the receiving side. + if param.param_type == u8::MAX { + return Err(EncodingError::unencodable( + "BGP OPEN optional parameter type", + "type 255 is reserved by RFC 9072 as the extended-length marker", + )); + } + Ok((param.param_type, encode_bgp_open_param_value(param)?)) + }) + .collect::>()?; let values_len: usize = encoded_params.iter().map(|(_, value)| value.len()).sum(); // Non-extended framing spends 2 header octets (type + 1-octet length) per @@ -449,11 +458,11 @@ impl BgpOpenMessage { if use_extended_length { // RFC 9072: type 255 signals a two-octet aggregate length and // two-octet lengths for each optional parameter. - debug_assert!( - encoded_params_len <= u16::MAX as usize, - "BGP OPEN optional parameters ({encoded_params_len} bytes) exceed the \ - two-octet extended length field; wire framing would be corrupt" - ); + check_max( + "BGP OPEN extended optional parameters total length", + encoded_params_len, + u16::MAX as usize, + )?; buf.put_u8(u8::MAX); buf.put_u16(encoded_params_len as u16); } @@ -461,12 +470,9 @@ impl BgpOpenMessage { for (param_type, value) in encoded_params { buf.put_u8(param_type); if use_extended_length { - debug_assert!( - value.len() <= u16::MAX as usize, - "BGP OPEN optional parameter ({} bytes) exceeds the two-octet \ - length field; wire framing would be corrupt", - value.len() - ); + // Cannot overflow: the aggregate check above bounds + // encoded_params_len, and each value.len() <= encoded_params_len - 3. + debug_assert!(value.len() <= u16::MAX as usize); buf.put_u16(value.len() as u16); } else { // Fits in a u8: use_extended_length is set above whenever the @@ -475,7 +481,7 @@ impl BgpOpenMessage { } buf.put_slice(&value); } - buf.freeze() + Ok(buf.freeze()) } } @@ -589,22 +595,24 @@ pub fn parse_bgp_update_message( } impl BgpUpdateMessage { - pub fn encode(&self, asn_len: AsnLength) -> Bytes { + pub fn encode(&self, asn_len: AsnLength) -> Result { let mut bytes = BytesMut::new(); // withdrawn prefixes let withdrawn_bytes = encode_nlri_prefixes(&self.withdrawn_prefixes); - bytes.put_u16(withdrawn_bytes.len() as u16); - bytes.put_slice(&withdrawn_bytes); + put_u16_len_slice( + &mut bytes, + "BGP UPDATE withdrawn routes length", + &withdrawn_bytes, + )?; // attributes - let attr_bytes = self.attributes.encode(asn_len); - - bytes.put_u16(attr_bytes.len() as u16); - bytes.put_slice(&attr_bytes); + with_u16_len(&mut bytes, "BGP UPDATE total path attribute length", |b| { + self.attributes.encode_to(asn_len, b) + })?; bytes.extend(encode_nlri_prefixes(&self.announced_prefixes)); - bytes.freeze() + Ok(bytes.freeze()) } /// Check if this is an end-of-rib message. @@ -654,23 +662,25 @@ impl BgpMessage { /// BGP marker value: 16 bytes of 0xFF (RFC 4271) const MARKER: [u8; 16] = [0xFF; 16]; - pub fn encode(&self, asn_len: AsnLength) -> Bytes { + pub fn encode(&self, asn_len: AsnLength) -> Result { let mut bytes = BytesMut::new(); // RFC 4271: Marker is 16 bytes of 0xFF bytes.put_slice(&Self::MARKER); let (msg_type, msg_bytes) = match self { - BgpMessage::Open(msg) => (BgpMessageType::OPEN, msg.encode()), - BgpMessage::Update(msg) => (BgpMessageType::UPDATE, msg.encode(asn_len)), + BgpMessage::Open(msg) => (BgpMessageType::OPEN, msg.encode()?), + BgpMessage::Update(msg) => (BgpMessageType::UPDATE, msg.encode(asn_len)?), BgpMessage::Notification(msg) => (BgpMessageType::NOTIFICATION, msg.encode()), BgpMessage::KeepAlive => (BgpMessageType::KEEPALIVE, Bytes::new()), }; // msg total bytes length = msg bytes + 16 bytes marker + 2 bytes length + 1 byte type - bytes.put_u16(msg_bytes.len() as u16 + 16 + 2 + 1); + let total_len = msg_bytes.len() + 16 + 2 + 1; + check_max("BGP message total length", total_len, u16::MAX as usize)?; + bytes.put_u16(total_len as u16); bytes.put_u8(msg_type as u8); bytes.put_slice(&msg_bytes); - bytes.freeze() + Ok(bytes.freeze()) } } @@ -895,7 +905,7 @@ mod tests { fn test_bgp_marker_encoding_rfc4271() { // Test that BgpMessage::encode produces correct RFC 4271 marker (all 0xFF) let msg = BgpMessage::KeepAlive; - let encoded = msg.encode(AsnLength::Bits16); + let encoded = msg.encode(AsnLength::Bits16).unwrap(); // First 16 bytes should be all 0xFF assert_eq!( @@ -1026,7 +1036,7 @@ mod tests { extended_length: false, opt_params: vec![], }; - let bytes = msg.encode(); + let bytes = msg.encode().unwrap(); assert_eq!( bytes, Bytes::from_static(&[ @@ -1084,7 +1094,7 @@ mod tests { parse_bgp_message(&mut input, false, &AsnLength::Bits16).unwrap_or_else(|error| { panic!("failed to parse {name}: {error}"); }); - let encoded = parsed.encode(AsnLength::Bits16); + let encoded = parsed.encode(AsnLength::Bits16).unwrap(); assert_eq!(encoded, wire, "{name}"); } @@ -1104,16 +1114,15 @@ mod tests { }], }; - let encoded = msg.encode(); + let encoded = msg.encode().unwrap(); assert_eq!(encoded[9], 5); assert_eq!(&encoded[10..], &[254, 3, 0xAA, 0xBB, 0xCC]); let parsed = parse_bgp_open_message(&mut encoded.clone()).unwrap(); - assert_eq!(parsed.encode(), encoded); + assert_eq!(parsed.encode().unwrap(), encoded); } #[test] - #[should_panic(expected = "BGP capability value length exceeds 255 octets")] fn test_bgp_open_encoding_rejects_oversized_add_path_capability() { use crate::models::capabilities::{AddPathAddressFamily, AddPathSendReceive}; @@ -1140,7 +1149,15 @@ mod tests { }], }; - msg.encode(); + let err = msg.encode().unwrap_err(); + assert_eq!( + err, + EncodingError::ValueTooLarge { + field: "BGP capability value length", + actual: 256, + max: 255 + } + ); } #[test] @@ -1157,7 +1174,7 @@ mod tests { }], }; - let encoded = msg.encode(); + let encoded = msg.encode().unwrap(); assert_eq!( &encoded[9..], @@ -1165,7 +1182,7 @@ mod tests { ); let parsed = parse_bgp_open_message(&mut encoded.clone()).unwrap(); assert!(parsed.extended_length); - assert_eq!(parsed.encode(), encoded); + assert_eq!(parsed.encode().unwrap(), encoded); } #[test] @@ -1182,13 +1199,13 @@ mod tests { }], }; - let encoded = msg.encode(); + let encoded = msg.encode().unwrap(); assert_eq!(encoded.len(), 272); assert_eq!(&encoded[9..16], &[0xFF, 0xFF, 0x01, 0x03, 254, 0x01, 0x00]); let parsed = parse_bgp_open_message(&mut encoded.clone()).unwrap(); assert!(parsed.extended_length); - assert_eq!(parsed.encode(), encoded); + assert_eq!(parsed.encode().unwrap(), encoded); } #[test] @@ -1197,7 +1214,7 @@ mod tests { error: BgpError::MessageHeaderError(MessageHeaderError::BAD_MESSAGE_LENGTH), data: vec![0x00, 0x00], }); - let bytes = bgp_message.encode(AsnLength::Bits16); + let bytes = bgp_message.encode(AsnLength::Bits16).unwrap(); // RFC 4271: Marker is 16 bytes of 0xFF assert_eq!( bytes, @@ -1452,7 +1469,7 @@ mod tests { use crate::models::capabilities::BgpExtendedMessageCapability; // Test that the encoding path for BgpExtendedMessage capability is covered - // This specifically tests the line: CapabilityValue::BgpExtendedMessage(bem) => bem.encode() + // This specifically tests the line: CapabilityValue::BgpExtendedMessage(bem) => bem.encode().unwrap() let capability_value = CapabilityValue::BgpExtendedMessage(BgpExtendedMessageCapability::new()); let capability = Capability { @@ -1475,7 +1492,7 @@ mod tests { }; // This will exercise the encoding path we need to test - let encoded = msg.encode(); + let encoded = msg.encode().unwrap(); assert!(!encoded.is_empty()); // Verify we can parse it back (exercises the parsing path too) @@ -1548,7 +1565,7 @@ mod tests { }], }; - let encoded = msg.encode(); + let encoded = msg.encode().unwrap(); // Parse the encoded message back and verify it matches let parsed = parse_bgp_open_message(&mut encoded.clone()).unwrap(); @@ -1606,7 +1623,7 @@ mod tests { }], }; - let encoded = msg.encode(); + let encoded = msg.encode().unwrap(); // Parse the encoded message back and verify it matches let parsed = parse_bgp_open_message(&mut encoded.clone()).unwrap(); @@ -1671,7 +1688,7 @@ mod tests { }; // Encode the message - let encoded = msg.encode(); + let encoded = msg.encode().unwrap(); // Parse it back let mut encoded_bytes = encoded.clone(); @@ -1752,7 +1769,7 @@ mod tests { }; // Encode and parse back - let encoded = msg.encode(); + let encoded = msg.encode().unwrap(); let mut encoded_bytes = encoded.clone(); let parsed = parse_bgp_open_message(&mut encoded_bytes).unwrap(); diff --git a/src/parser/bmp/messages/peer_up_notification.rs b/src/parser/bmp/messages/peer_up_notification.rs index e2391da..5e4ffaa 100644 --- a/src/parser/bmp/messages/peer_up_notification.rs +++ b/src/parser/bmp/messages/peer_up_notification.rs @@ -203,7 +203,7 @@ mod tests { extended_length: false, opt_params: vec![], }); - let bgp_open_message_bytes = bgp_open_message.encode(AsnLength::Bits32); + let bgp_open_message_bytes = bgp_open_message.encode(AsnLength::Bits32).unwrap(); data.extend_from_slice(&bgp_open_message_bytes); data.extend_from_slice(&bgp_open_message_bytes); @@ -322,7 +322,7 @@ mod tests { extended_length: false, opt_params: vec![], }); - let bgp1_bytes = bgp1.encode(AsnLength::Bits32); + let bgp1_bytes = bgp1.encode(AsnLength::Bits32).unwrap(); // Second BGP OPEN message let bgp2 = crate::models::BgpMessage::Open(BgpOpenMessage { @@ -333,7 +333,7 @@ mod tests { extended_length: false, opt_params: vec![], }); - let bgp2_bytes = bgp2.encode(AsnLength::Bits32); + let bgp2_bytes = bgp2.encode(AsnLength::Bits32).unwrap(); // Add both BGP messages consecutively data.extend_from_slice(&bgp1_bytes); @@ -441,7 +441,7 @@ mod tests { extended_length: false, opt_params: vec![], }); - let bgp_bytes = bgp_open.encode(AsnLength::Bits32); + let bgp_bytes = bgp_open.encode(AsnLength::Bits32).unwrap(); data.extend_from_slice(&bgp_bytes); // Add incomplete second BGP message (only partial header) @@ -483,7 +483,7 @@ mod tests { extended_length: false, opt_params: vec![], }); - let bgp_bytes = bgp_open.encode(AsnLength::Bits32); + let bgp_bytes = bgp_open.encode(AsnLength::Bits32).unwrap(); data.extend_from_slice(&bgp_bytes); data.extend_from_slice(&bgp_bytes); @@ -537,7 +537,7 @@ mod tests { extended_length: false, opt_params: vec![], // No capabilities }); - let bgp_bytes = bgp_open.encode(AsnLength::Bits32); + let bgp_bytes = bgp_open.encode(AsnLength::Bits32).unwrap(); data.extend_from_slice(&bgp_bytes); data.extend_from_slice(&bgp_bytes); @@ -586,7 +586,7 @@ mod tests { }]), }], }); - let bgp_bytes = bgp_open.encode(AsnLength::Bits32); + let bgp_bytes = bgp_open.encode(AsnLength::Bits32).unwrap(); data.extend_from_slice(&bgp_bytes); data.extend_from_slice(&bgp_bytes); @@ -635,7 +635,7 @@ mod tests { }]), }], }); - let bgp_bytes = bgp_open.encode(AsnLength::Bits32); + let bgp_bytes = bgp_open.encode(AsnLength::Bits32).unwrap(); data.extend_from_slice(&bgp_bytes); data.extend_from_slice(&bgp_bytes); @@ -689,7 +689,7 @@ mod tests { }]), }], }); - let bgp_bytes = bgp_open.encode(AsnLength::Bits32); + let bgp_bytes = bgp_open.encode(AsnLength::Bits32).unwrap(); data.extend_from_slice(&bgp_bytes); data.extend_from_slice(&bgp_bytes); @@ -751,7 +751,7 @@ mod tests { }]), }], }); - let bgp_bytes = bgp_open.encode(AsnLength::Bits32); + let bgp_bytes = bgp_open.encode(AsnLength::Bits32).unwrap(); data.extend_from_slice(&bgp_bytes); data.extend_from_slice(&bgp_bytes); @@ -804,7 +804,7 @@ mod tests { }]), }], }); - let bgp_bytes = bgp_open.encode(AsnLength::Bits32); + let bgp_bytes = bgp_open.encode(AsnLength::Bits32).unwrap(); data.extend_from_slice(&bgp_bytes); data.extend_from_slice(&bgp_bytes); @@ -846,7 +846,7 @@ mod tests { extended_length: false, opt_params: vec![], }); - let bgp_bytes = bgp_open.encode(AsnLength::Bits32); + let bgp_bytes = bgp_open.encode(AsnLength::Bits32).unwrap(); data.extend_from_slice(&bgp_bytes); data.extend_from_slice(&bgp_bytes); @@ -900,7 +900,7 @@ mod tests { extended_length: false, opt_params: vec![], }); - let bgp_bytes = bgp_open.encode(AsnLength::Bits32); + let bgp_bytes = bgp_open.encode(AsnLength::Bits32).unwrap(); data.extend_from_slice(&bgp_bytes); data.extend_from_slice(&bgp_bytes); diff --git a/src/parser/bmp/messages/route_mirroring.rs b/src/parser/bmp/messages/route_mirroring.rs index 0457aa3..ef5f3c7 100644 --- a/src/parser/bmp/messages/route_mirroring.rs +++ b/src/parser/bmp/messages/route_mirroring.rs @@ -85,7 +85,7 @@ mod tests { extended_length: false, opt_params: vec![], }); - let bgp_message_bytes = bgp_message.encode(AsnLength::Bits32); + let bgp_message_bytes = bgp_message.encode(AsnLength::Bits32).unwrap(); let expected_asn_len = AsnLength::Bits32; let actual_info_len = bgp_message_bytes.len() as u16; diff --git a/src/parser/bmp/messages/route_monitoring.rs b/src/parser/bmp/messages/route_monitoring.rs index 98b98c2..2f24cda 100644 --- a/src/parser/bmp/messages/route_monitoring.rs +++ b/src/parser/bmp/messages/route_monitoring.rs @@ -98,7 +98,7 @@ mod tests { attributes: Attributes::default(), announced_prefixes: vec![], }); - let bgp_bytes = bgp_update.encode(AsnLength::Bits16); + let bgp_bytes = bgp_update.encode(AsnLength::Bits16).unwrap(); let mut data = bgp_bytes; let asn_len = AsnLength::Bits16; // RFC 9069 violation @@ -121,7 +121,7 @@ mod tests { attributes: Attributes::default(), announced_prefixes: vec![], }); - let bgp_bytes = bgp_update.encode(AsnLength::Bits32); + let bgp_bytes = bgp_update.encode(AsnLength::Bits32).unwrap(); let mut data = bgp_bytes; let asn_len = AsnLength::Bits32; // RFC 9069 compliant @@ -144,7 +144,7 @@ mod tests { attributes: Attributes::default(), announced_prefixes: vec![], }); - let bgp_bytes = bgp_update.encode(AsnLength::Bits16); + let bgp_bytes = bgp_update.encode(AsnLength::Bits16).unwrap(); let mut data = bgp_bytes; let asn_len = AsnLength::Bits16; diff --git a/src/parser/iters/route.rs b/src/parser/iters/route.rs index 8d44cac..56ff299 100644 --- a/src/parser/iters/route.rs +++ b/src/parser/iters/route.rs @@ -964,7 +964,7 @@ mod tests { Asn::new_32bit(64496), ); let mut peer_table = PeerIndexTable::default(); - let peer_index = peer_table.add_peer(peer); + let peer_index = peer_table.add_peer(peer).unwrap(); let mut attributes = Attributes::default(); attributes.add_attr(AttributeValue::Origin(Origin::IGP).into()); @@ -1009,8 +1009,8 @@ mod tests { })), }; - let mut bytes = pit_record.encode().to_vec(); - bytes.extend_from_slice(&rib_record.encode()); + let mut bytes = pit_record.encode().unwrap().to_vec(); + bytes.extend_from_slice(&rib_record.encode().unwrap()); bytes } @@ -1021,7 +1021,7 @@ mod tests { Asn::new_32bit(64496), ); let mut peer_table = PeerIndexTable::default(); - let peer_index = peer_table.add_peer(peer); + let peer_index = peer_table.add_peer(peer).unwrap(); let pit_record = MrtRecord { common_header: CommonHeader { @@ -1047,7 +1047,7 @@ mod tests { rib_body.put_u32(1); rib_body.extend(NetworkPrefix::from_str("203.0.113.0/24").unwrap().encode()); rib_body.put_u16(2); - rib_body.extend(first_entry.encode()); + rib_body.extend(first_entry.encode().unwrap()); rib_body.put_u16(peer_index); rib_body.put_u32(1_699_999_998); rib_body.put_u16(32); @@ -1062,7 +1062,7 @@ mod tests { length: rib_body.len() as u32, }; - let mut bytes = pit_record.encode().to_vec(); + let mut bytes = pit_record.encode().unwrap().to_vec(); bytes.extend_from_slice(&rib_header.encode()); bytes.extend_from_slice(&rib_body); @@ -1112,7 +1112,7 @@ mod tests { data.put_u16(65001); data.put_u16(0); data.put_u16(Afi::LinkState as u16); - data.extend(&BgpMessage::KeepAlive.encode(AsnLength::Bits16)); + data.put_slice(&BgpMessage::KeepAlive.encode(AsnLength::Bits16).unwrap()); let error = match parse_bgp4mp_routes(Bgp4MpType::Message as u16, data.freeze(), 1_700_000_000.0) { @@ -1128,7 +1128,7 @@ mod tests { #[test] fn route_iterator_matches_elem_projection_for_update() { - let bytes = update_record().encode().to_vec(); + let bytes = update_record().encode().unwrap().to_vec(); let routes = assert_route_projection(bytes); assert_eq!(routes.len(), 2); assert_eq!(routes[0].elem_type, ElemType::ANNOUNCE); @@ -1150,6 +1150,7 @@ mod tests { }), ) .encode() + .unwrap() .to_vec(); let routes = BgpkitParser::from_reader(Cursor::new(bytes)) @@ -1202,6 +1203,7 @@ mod tests { }), ) .encode() + .unwrap() .to_vec(); let routes = assert_route_projection(bytes); @@ -1234,7 +1236,7 @@ mod tests { ]; let mut bytes = Vec::new(); for record in records { - bytes.extend_from_slice(&record.encode()); + bytes.extend_from_slice(&record.encode().unwrap()); } assert!(assert_route_projection(bytes).is_empty()); @@ -1251,6 +1253,7 @@ mod tests { }), ) .encode() + .unwrap() .to_vec(); let routes = assert_route_projection(bytes); @@ -1260,7 +1263,7 @@ mod tests { #[test] fn route_iterator_filters_match_elem_projection_for_update() { - let bytes = update_record().encode().to_vec(); + let bytes = update_record().encode().unwrap().to_vec(); let cases: &[&[(&str, &str)]] = &[ &[("peer_ip", "192.0.2.1")], &[("peer_ip", "192.0.2.99")], @@ -1303,7 +1306,7 @@ mod tests { ); let attrs = parse_route_attributes( - attributes.encode(AsnLength::Bits16), + attributes.encode(AsnLength::Bits16).unwrap(), &AsnLength::Bits16, false, RouteAttributeContext { @@ -1325,7 +1328,9 @@ mod tests { #[test] fn selective_attribute_parser_handles_as_path_without_as4_path() { let attrs = parse_route_attributes( - route_attributes([64500, 64501]).encode(AsnLength::Bits16), + route_attributes([64500, 64501]) + .encode(AsnLength::Bits16) + .unwrap(), &AsnLength::Bits16, false, RouteAttributeContext { @@ -1356,7 +1361,7 @@ mod tests { ); let attrs = parse_route_attributes( - attributes.encode(AsnLength::Bits16), + attributes.encode(AsnLength::Bits16).unwrap(), &AsnLength::Bits16, false, RouteAttributeContext { @@ -1459,7 +1464,7 @@ mod tests { #[test] fn route_iterator_matches_elem_projection_for_table_dump() { - let bytes = table_dump_record().encode().to_vec(); + let bytes = table_dump_record().encode().unwrap().to_vec(); let routes = assert_route_projection(bytes); assert_eq!(routes.len(), 1); assert_eq!(routes[0].timestamp, 1_699_999_998.0); @@ -1468,7 +1473,7 @@ mod tests { #[test] fn route_iterator_matches_elem_projection_for_table_dump_ipv6() { - let bytes = table_dump_ipv6_record().encode().to_vec(); + let bytes = table_dump_ipv6_record().encode().unwrap().to_vec(); let routes = assert_route_projection(bytes); assert_eq!(routes.len(), 1); assert_eq!( @@ -1501,7 +1506,7 @@ mod tests { Asn::new_32bit(64496), ); let mut peer_table = PeerIndexTable::default(); - let peer_index = peer_table.add_peer(peer); + let peer_index = peer_table.add_peer(peer).unwrap(); let pit_record = MrtRecord { common_header: CommonHeader { @@ -1534,8 +1539,8 @@ mod tests { })), }; - let mut bytes = pit_record.encode().to_vec(); - bytes.extend_from_slice(&rib_record.encode()); + let mut bytes = pit_record.encode().unwrap().to_vec(); + bytes.extend_from_slice(&rib_record.encode().unwrap()); let routes = assert_route_projection(bytes); assert_eq!(routes.len(), 1); assert_eq!( @@ -1569,7 +1574,7 @@ mod tests { })), }; - let routes = assert_route_projection(record.encode().to_vec()); + let routes = assert_route_projection(record.encode().unwrap().to_vec()); assert_eq!(routes.len(), 1); assert_eq!( routes[0].peer_ip, @@ -1690,7 +1695,7 @@ mod tests { let mut no_peer_table = None; assert!(parse_table_dump_v2_routes( TableDumpV2Type::RibIpv4Unicast as u16, - rib.encode(), + rib.encode().unwrap(), &mut no_peer_table, ) .is_err()); @@ -1699,7 +1704,7 @@ mod tests { let routes = collect_route_record_iter( parse_table_dump_v2_routes( TableDumpV2Type::RibIpv4Unicast as u16, - rib.encode(), + rib.encode().unwrap(), &mut empty_peer_table, ) .unwrap(), @@ -1729,7 +1734,7 @@ mod tests { Asn::new_32bit(64496), ); let mut peer_table = PeerIndexTable::default(); - let peer_index = peer_table.add_peer(peer); + let peer_index = peer_table.add_peer(peer).unwrap(); let first_entry = RibEntry { peer_index, @@ -1741,7 +1746,7 @@ mod tests { add_path_truncated.put_u32(1); add_path_truncated.extend(NetworkPrefix::from_str("203.0.113.0/24").unwrap().encode()); add_path_truncated.put_u16(2); - add_path_truncated.extend(first_entry.encode()); + add_path_truncated.extend(first_entry.encode().unwrap()); add_path_truncated.put_u16(peer_index); add_path_truncated.put_u32(1_699_999_998); add_path_truncated.put_u32(5678); @@ -1837,6 +1842,7 @@ mod tests { let routes = BgpkitParser::from_reader(Cursor::new( table_dump_v2_rib_without_peer_table_record() .encode() + .unwrap() .to_vec(), )) .into_route_iter() @@ -1847,12 +1853,13 @@ mod tests { #[test] fn fallible_route_iterator_applies_filters_to_cached_routes() { - let routes = BgpkitParser::from_reader(Cursor::new(update_record().encode().to_vec())) - .add_filter("type", "w") - .unwrap() - .into_fallible_route_iter() - .collect::, _>>() - .unwrap(); + let routes = + BgpkitParser::from_reader(Cursor::new(update_record().encode().unwrap().to_vec())) + .add_filter("type", "w") + .unwrap() + .into_fallible_route_iter() + .collect::, _>>() + .unwrap(); assert_eq!(routes.len(), 1); assert_eq!(routes[0].elem_type, ElemType::WITHDRAW); @@ -1863,6 +1870,7 @@ mod tests { let mut iter = BgpkitParser::from_reader(Cursor::new( table_dump_v2_rib_without_peer_table_record() .encode() + .unwrap() .to_vec(), )) .into_fallible_route_iter(); @@ -1872,7 +1880,7 @@ mod tests { #[test] fn fallible_route_iterator_yields_routes() { - let bytes = update_record().encode().to_vec(); + let bytes = update_record().encode().unwrap().to_vec(); let routes = BgpkitParser::from_reader(Cursor::new(bytes)) .into_fallible_route_iter() .collect::, _>>() diff --git a/src/parser/mrt/messages/bgp4mp.rs b/src/parser/mrt/messages/bgp4mp.rs index e76734f..57db802 100644 --- a/src/parser/mrt/messages/bgp4mp.rs +++ b/src/parser/mrt/messages/bgp4mp.rs @@ -1,4 +1,4 @@ -use crate::error::ParserError; +use crate::error::{EncodingError, ParserError}; use crate::models::*; use crate::parser::bgp::messages::parse_bgp_message; use crate::parser::{encode_asn, encode_ipaddr, ReadUtils}; @@ -171,7 +171,7 @@ pub fn parse_bgp4mp_message( } impl Bgp4MpMessage { - pub fn encode(&self, asn_len: AsnLength) -> Bytes { + pub fn encode(&self, asn_len: AsnLength) -> Result { let mut bytes = BytesMut::new(); bytes.extend(encode_asn(&self.peer_asn, &asn_len)); bytes.extend(encode_asn(&self.local_asn, &asn_len)); @@ -179,8 +179,8 @@ impl Bgp4MpMessage { bytes.put_u16(address_family(&self.peer_ip)); bytes.extend(encode_ipaddr(&self.peer_ip)); bytes.extend(encode_ipaddr(&self.local_ip)); - bytes.extend(&self.bgp_message.encode(asn_len)); - bytes.freeze() + bytes.put_slice(&self.bgp_message.encode(asn_len)?); + Ok(bytes.freeze()) } } @@ -274,7 +274,7 @@ mod tests { bgp_message: BgpMessage::KeepAlive, }; - let encoded = message.encode(AsnLength::Bits16); + let encoded = message.encode(AsnLength::Bits16).unwrap(); let parsed = parse_bgp4mp(Bgp4MpType::Message as u16, encoded).unwrap(); match parsed { @@ -297,7 +297,7 @@ mod tests { data.put_u16(65001); data.put_u16(0); data.put_u16(Afi::LinkState as u16); - data.extend(&BgpMessage::KeepAlive.encode(AsnLength::Bits16)); + data.put_slice(&BgpMessage::KeepAlive.encode(AsnLength::Bits16).unwrap()); let error = match parse_bgp4mp(Bgp4MpType::Message as u16, data.freeze()) { Err(error) => error, diff --git a/src/parser/mrt/messages/mod.rs b/src/parser/mrt/messages/mod.rs index 3f0d238..1f1e905 100644 --- a/src/parser/mrt/messages/mod.rs +++ b/src/parser/mrt/messages/mod.rs @@ -1,3 +1,4 @@ +use crate::error::EncodingError; use crate::models::{AsnLength, Bgp4MpEnum, Bgp4MpType, MrtMessage, TableDumpV2Message}; use bytes::Bytes; @@ -6,16 +7,19 @@ pub(crate) mod table_dump; pub(crate) mod table_dump_v2; impl MrtMessage { - pub fn encode(&self, sub_type: u16) -> Bytes { + pub fn encode(&self, sub_type: u16) -> Result { let msg_bytes: Bytes = match self { - MrtMessage::TableDumpMessage(m) => m.encode(), + MrtMessage::TableDumpMessage(m) => m.encode()?, MrtMessage::TableDumpV2Message(m) => match m { - TableDumpV2Message::PeerIndexTable(p) => p.encode(), - TableDumpV2Message::RibAfi(r) => r.encode(), + TableDumpV2Message::PeerIndexTable(p) => p.encode()?, + TableDumpV2Message::RibAfi(r) => r.encode()?, TableDumpV2Message::RibGeneric(_) => { - todo!("RibGeneric message is not supported yet"); + return Err(EncodingError::unencodable( + "MRT TABLE_DUMP_V2 RIB_GENERIC message", + "encoding not implemented", + )); } - TableDumpV2Message::GeoPeerTable(g) => g.encode(), + TableDumpV2Message::GeoPeerTable(g) => g.encode()?, }, MrtMessage::Bgp4Mp(m) => { let msg_type = Bgp4MpType::try_from(sub_type).unwrap(); @@ -39,13 +43,13 @@ impl MrtMessage { true => AsnLength::Bits32, false => AsnLength::Bits16, }; - msg.encode(asn_len) + msg.encode(asn_len)? } } } }; - msg_bytes + Ok(msg_bytes) } } @@ -70,7 +74,7 @@ mod tests { MrtMessage::TableDumpV2Message(TableDumpV2Message::GeoPeerTable(geo_table)); let subtype = TableDumpV2Type::GeoPeerTable as u16; - let encoded = mrt_message.encode(subtype); + let encoded = mrt_message.encode(subtype).unwrap(); // Should produce some encoded bytes assert!(!encoded.is_empty()); diff --git a/src/parser/mrt/messages/table_dump.rs b/src/parser/mrt/messages/table_dump.rs index 4d20185..61d3f02 100644 --- a/src/parser/mrt/messages/table_dump.rs +++ b/src/parser/mrt/messages/table_dump.rs @@ -1,3 +1,4 @@ +use crate::encoder::sink::with_u16_len; use crate::error::*; use crate::models::*; use crate::parser::bgp::attributes::parse_attributes; @@ -118,7 +119,7 @@ pub fn parse_table_dump_message( } impl TableDumpMessage { - pub fn encode(&self) -> Bytes { + pub fn encode(&self) -> Result { let mut bytes = BytesMut::new(); bytes.put_u16(self.view_number); bytes.put_u16(self.sequence_number); @@ -146,17 +147,12 @@ impl TableDumpMessage { } bytes.put_u16(self.peer_asn.into()); - // encode attributes - let mut attr_bytes = BytesMut::new(); - for attr in &self.attributes.inner { - // asn_len always 16 bites - attr_bytes.extend(attr.encode(AsnLength::Bits16)); - } - - bytes.put_u16(attr_bytes.len() as u16); - bytes.put_slice(&attr_bytes); + // encode attributes; asn_len is always 16-bit for TABLE_DUMP + with_u16_len(&mut bytes, "TABLE_DUMP attribute length", |b| { + self.attributes.encode_to(AsnLength::Bits16, b) + })?; - bytes.freeze() + Ok(bytes.freeze()) } } @@ -213,7 +209,7 @@ mod tests { "SEQUENCE_NUMBER mismatch" ); // Add more assertions here as per your actual requirements - let encoded = table_dump_message.encode(); + let encoded = table_dump_message.encode().unwrap(); assert_eq!(encoded, bytes); } #[test] @@ -252,7 +248,7 @@ mod tests { // Add more assertions here as per your actual requirements // test encoding - let encoded = table_dump_message.encode(); + let encoded = table_dump_message.encode().unwrap(); assert_eq!(encoded, bytes); } @@ -311,7 +307,7 @@ mod tests { attributes, }; - // This should exercise the attr.encode(AsnLength::Bits16) line - let _encoded = table_dump.encode(); + // This should exercise the attr.encode(AsnLength::Bits16).unwrap() line + let _encoded = table_dump.encode().unwrap(); } } diff --git a/src/parser/mrt/messages/table_dump_v2/geo_peer_table.rs b/src/parser/mrt/messages/table_dump_v2/geo_peer_table.rs index a906487..ec22242 100644 --- a/src/parser/mrt/messages/table_dump_v2/geo_peer_table.rs +++ b/src/parser/mrt/messages/table_dump_v2/geo_peer_table.rs @@ -1,6 +1,7 @@ //! RFC 6397: GEO_PEER_TABLE parsing for MRT TABLE_DUMP_V2 format -use crate::error::ParserError; +use crate::encoder::sink::put_u16_len_slice; +use crate::error::{check_max, EncodingError, ParserError}; use crate::models::*; use crate::parser::ReadUtils; use bytes::{Buf, BufMut, Bytes, BytesMut}; @@ -134,24 +135,31 @@ impl GeoPeerTable { /// -0.1278, // London longitude /// ); /// - /// let encoded = geo_table.encode(); + /// let encoded = geo_table.encode().unwrap(); /// ``` - pub fn encode(&self) -> Bytes { + pub fn encode(&self) -> Result { let mut buf = BytesMut::new(); // Encode collector BGP ID (4 bytes) buf.put_u32(self.collector_bgp_id.into()); // Encode view name length and view name - let view_name_bytes = self.view_name.as_bytes(); - buf.put_u16(view_name_bytes.len() as u16); - buf.extend(view_name_bytes); + put_u16_len_slice( + &mut buf, + "GeoPeerTable view name length", + self.view_name.as_bytes(), + )?; // Encode collector coordinates (4 bytes each, 32-bit float) buf.put_f32(self.collector_latitude); buf.put_f32(self.collector_longitude); // Encode peer count + check_max( + "GeoPeerTable peer count", + self.geo_peers.len(), + u16::MAX as usize, + )?; buf.put_u16(self.geo_peers.len() as u16); // Encode each peer entry @@ -188,7 +196,7 @@ impl GeoPeerTable { buf.put_f32(geo_peer.peer_longitude); } - buf.freeze() + Ok(buf.freeze()) } } @@ -344,7 +352,7 @@ mod tests { original_table.add_geo_peer(geo_peer2); // Encode and then parse back - let encoded = original_table.encode(); + let encoded = original_table.encode().unwrap(); let mut encoded_bytes = encoded; let parsed_table = parse_geo_peer_table(&mut encoded_bytes).unwrap(); @@ -423,7 +431,7 @@ mod tests { geo_table.add_geo_peer(geo_peer2); // Encode the geo table - let encoded = geo_table.encode(); + let encoded = geo_table.encode().unwrap(); // Create expected bytes manually for comparison let mut expected = BytesMut::new(); diff --git a/src/parser/mrt/messages/table_dump_v2/peer_index_table.rs b/src/parser/mrt/messages/table_dump_v2/peer_index_table.rs index bd3a84a..838794c 100644 --- a/src/parser/mrt/messages/table_dump_v2/peer_index_table.rs +++ b/src/parser/mrt/messages/table_dump_v2/peer_index_table.rs @@ -1,3 +1,5 @@ +use crate::encoder::sink::put_u16_len_slice; +use crate::error::{check_max, EncodingError}; use crate::models::{Afi, AsnLength, Peer, PeerIndexTable, PeerType}; use crate::parser::ReadUtils; use crate::ParserError; @@ -65,15 +67,22 @@ pub fn parse_peer_index_table(data: &mut Bytes) -> Result u16 { + /// Add peer to peer index table and return peer id. + /// + /// The PEER_INDEX_TABLE wire format uses a 16-bit peer count, so at most + /// 65535 distinct peers can be stored. Adding a peer beyond that returns + /// [`EncodingError::ValueTooLarge`] and leaves the table unmodified — + /// previously the id silently wrapped, aliasing routes to the wrong peer. + pub fn add_peer(&mut self, peer: Peer) -> Result { match self.peer_ip_id_map.get(&peer.peer_ip) { - Some(id) => *id, + Some(id) => Ok(*id), None => { - let peer_id = self.peer_ip_id_map.len() as u16; + let next_id = self.peer_ip_id_map.len(); + check_max("PeerIndexTable peer count", next_id + 1, u16::MAX as usize)?; + let peer_id = next_id as u16; self.peer_ip_id_map.insert(peer.peer_ip, peer_id); self.id_peer_map.insert(peer_id, peer); - peer_id + Ok(peer_id) } } } @@ -136,24 +145,25 @@ impl PeerIndexTable { /// peer_ip_id_map: Default::default(), /// }; /// - /// let encoded = data.encode(); + /// let encoded = data.encode().unwrap(); /// ``` - pub fn encode(&self) -> Bytes { + pub fn encode(&self) -> Result { let mut buf = BytesMut::new(); // Encode collector_bgp_id buf.put_u32(self.collector_bgp_id.into()); - // Encode view_name_length - let view_name_bytes = self.view_name.as_bytes(); - buf.put_u16(view_name_bytes.len() as u16); - - // Encode view_name - buf.extend(view_name_bytes); + // Encode view_name_length and view_name + put_u16_len_slice( + &mut buf, + "PeerIndexTable view name length", + self.view_name.as_bytes(), + )?; // Encode peer_count - let peer_count = self.id_peer_map.len() as u16; - buf.put_u16(peer_count); + let peer_count = self.id_peer_map.len(); + check_max("PeerIndexTable peer count", peer_count, u16::MAX as usize)?; + buf.put_u16(peer_count as u16); // Encode peers let mut peer_ids: Vec<_> = self.id_peer_map.keys().collect(); @@ -184,7 +194,7 @@ impl PeerIndexTable { } // Return Bytes - buf.freeze() + Ok(buf.freeze()) } } @@ -203,18 +213,22 @@ mod tests { peer_ip_id_map: Default::default(), }; - index_table.add_peer(Peer::new( - Ipv4Addr::from(1234), - IpAddr::from_str("192.168.1.1").unwrap(), - Asn::new_32bit(1234), - )); - index_table.add_peer(Peer::new( - Ipv4Addr::from(12345), - IpAddr::from_str("192.168.1.2").unwrap(), - Asn::new_32bit(12345), - )); + index_table + .add_peer(Peer::new( + Ipv4Addr::from(1234), + IpAddr::from_str("192.168.1.1").unwrap(), + Asn::new_32bit(1234), + )) + .unwrap(); + index_table + .add_peer(Peer::new( + Ipv4Addr::from(12345), + IpAddr::from_str("192.168.1.2").unwrap(), + Asn::new_32bit(12345), + )) + .unwrap(); - let encoded = index_table.encode(); + let encoded = index_table.encode().unwrap(); let parsed_index_table = parse_peer_index_table(&mut encoded.clone()).unwrap(); assert_eq!(index_table, parsed_index_table); } @@ -239,8 +253,8 @@ mod tests { Asn::new_32bit(12345), ); - let peer1_id = index_table.add_peer(peer1); - let peer2_id = index_table.add_peer(peer2); + let peer1_id = index_table.add_peer(peer1).unwrap(); + let peer2_id = index_table.add_peer(peer2).unwrap(); assert_eq!( index_table.get_peer_by_id(&peer1_id), @@ -259,4 +273,43 @@ mod tests { )) ); } + + #[test] + fn test_add_peer_rejects_overflow_without_corruption() { + let mut index_table = PeerIndexTable::default(); + + // fill the table to its 16-bit wire capacity of 65535 peers + for i in 0..(u16::MAX as u32) { + let ip = IpAddr::from(Ipv4Addr::from(i + 1)); + index_table + .add_peer(Peer::new(Ipv4Addr::from(1), ip, Asn::new_32bit(i))) + .unwrap(); + } + assert_eq!(index_table.id_peer_map.len(), u16::MAX as usize); + + // the 65536th peer must be rejected, not aliased onto an existing id + let overflow_ip = IpAddr::from(Ipv4Addr::from(u16::MAX as u32 + 1)); + let overflow_peer = Peer::new(Ipv4Addr::from(1), overflow_ip, Asn::new_32bit(65536)); + let err = index_table.add_peer(overflow_peer).unwrap_err(); + assert_eq!( + err, + EncodingError::ValueTooLarge { + field: "PeerIndexTable peer count", + actual: u16::MAX as usize + 1, + max: u16::MAX as usize + } + ); + + // the failed insert must not have modified the table + assert_eq!(index_table.id_peer_map.len(), u16::MAX as usize); + assert_eq!(index_table.get_peer_id_by_addr(&overflow_ip), None); + + // adding an existing peer still returns its id without error + let existing_ip = IpAddr::from(Ipv4Addr::from(1u32)); + let existing = Peer::new(Ipv4Addr::from(1), existing_ip, Asn::new_32bit(0)); + assert_eq!(index_table.add_peer(existing).unwrap(), 0); + + // the full table still encodes successfully + index_table.encode().unwrap(); + } } diff --git a/src/parser/mrt/messages/table_dump_v2/rib_afi_entries.rs b/src/parser/mrt/messages/table_dump_v2/rib_afi_entries.rs index 02a354f..369408b 100644 --- a/src/parser/mrt/messages/table_dump_v2/rib_afi_entries.rs +++ b/src/parser/mrt/messages/table_dump_v2/rib_afi_entries.rs @@ -1,4 +1,6 @@ use crate::bgp::attributes::parse_attributes; +use crate::encoder::sink::with_u16_len; +use crate::error::{check_max, EncodingError}; use crate::models::{ Afi, AsnLength, NetworkPrefix, RibAfiEntries, RibEntry, Safi, TableDumpV2Type, }; @@ -164,7 +166,7 @@ pub fn parse_rib_entry( } impl RibAfiEntries { - pub fn encode(&self) -> Bytes { + pub fn encode(&self) -> Result { let mut bytes = BytesMut::new(); let is_add_path = is_add_path_rib_type(self.rib_type); @@ -172,23 +174,29 @@ impl RibAfiEntries { bytes.extend(self.prefix.encode()); let entry_count = self.rib_entries.len(); + check_max("RIB entry count", entry_count, u16::MAX as usize)?; bytes.put_u16(entry_count as u16); for entry in &self.rib_entries { - bytes.extend(entry.encode_for_rib_type(is_add_path)); + entry.encode_for_rib_type(is_add_path, &mut bytes)?; } - bytes.freeze() + Ok(bytes.freeze()) } } impl RibEntry { - pub fn encode(&self) -> Bytes { - self.encode_for_rib_type(self.path_id.is_some()) + pub fn encode(&self) -> Result { + let mut bytes = BytesMut::new(); + self.encode_for_rib_type(self.path_id.is_some(), &mut bytes)?; + Ok(bytes.freeze()) } - fn encode_for_rib_type(&self, include_path_id: bool) -> Bytes { - let mut bytes = BytesMut::new(); + fn encode_for_rib_type( + &self, + include_path_id: bool, + bytes: &mut BytesMut, + ) -> Result<(), EncodingError> { bytes.put_u16(self.peer_index); bytes.put_u32(self.originated_time); if include_path_id { @@ -196,10 +204,9 @@ impl RibEntry { bytes.put_u32(path_id); } } - let attr_bytes = self.attributes.encode(AsnLength::Bits32); - bytes.put_u16(attr_bytes.len() as u16); - bytes.extend(attr_bytes); - bytes.freeze() + with_u16_len(bytes, "RIB entry attribute length", |b| { + self.attributes.encode_to(AsnLength::Bits32, b) + }) } } @@ -270,7 +277,7 @@ mod tests { attributes, }; - let mut encoded = rib_entry.encode(); + let mut encoded = rib_entry.encode().unwrap(); assert_eq!(encoded.read_u16().unwrap(), 1); assert_eq!(encoded.read_u32().unwrap(), 12345); assert_eq!(encoded.read_u32().unwrap(), 42); @@ -297,7 +304,7 @@ mod tests { }], }; - let encoded = rib.encode(); + let encoded = rib.encode().unwrap(); let parsed = parse_rib_afi_entries(&mut encoded.clone(), rib.rib_type).unwrap(); assert_eq!(parsed.rib_type, rib.rib_type); assert_eq!(parsed.sequence_number, rib.sequence_number); diff --git a/src/parser/mrt/mrt_record.rs b/src/parser/mrt/mrt_record.rs index b14ad76..259bc54 100644 --- a/src/parser/mrt/mrt_record.rs +++ b/src/parser/mrt/mrt_record.rs @@ -1,6 +1,6 @@ use super::mrt_header::parse_common_header_with_bytes; use crate::bmp::messages::{BmpMessage, BmpMessageBody}; -use crate::error::ParserError; +use crate::error::{check_max, EncodingError, ParserError}; use crate::models::*; use crate::parser::{ parse_bgp4mp, parse_table_dump_message, parse_table_dump_v2_message, ParserErrorWithBytes, @@ -227,8 +227,8 @@ pub fn parse_mrt_body( } impl MrtRecord { - pub fn encode(&self) -> Bytes { - let message_bytes = self.message.encode(self.common_header.entry_subtype); + pub fn encode(&self) -> Result { + let message_bytes = self.message.encode(self.common_header.entry_subtype)?; let mut new_header = self.common_header; if message_bytes.len() != new_header.length as usize { warn!( @@ -237,6 +237,11 @@ impl MrtRecord { new_header.length ); } + check_max( + "MRT record message length", + message_bytes.len(), + u32::MAX as usize, + )?; new_header.length = message_bytes.len() as u32; let header_bytes = new_header.encode(); @@ -253,7 +258,7 @@ impl MrtRecord { let mut bytes = BytesMut::with_capacity(header_bytes.len() + message_bytes.len()); bytes.put_slice(&header_bytes); bytes.put_slice(&message_bytes); - bytes.freeze() + Ok(bytes.freeze()) } } @@ -294,12 +299,15 @@ impl TryFrom<&BmpMessage> for MrtRecord { let (seconds, microseconds) = convert_timestamp(bmp_header.timestamp); let subtype = Bgp4MpType::MessageAs4 as u16; + let encoded_message = mrt_message + .encode(subtype) + .map_err(|e| format!("cannot encode MRT message: {e}"))?; let mrt_header = CommonHeader { timestamp: seconds, microsecond_timestamp: Some(microseconds), entry_type: EntryType::BGP4MP_ET, entry_subtype: Bgp4MpType::MessageAs4 as u16, - length: mrt_message.encode(subtype).len() as u32, + length: encoded_message.len() as u32, }; Ok(MrtRecord { @@ -501,12 +509,13 @@ mod tests { })), }; - let encoded = record.encode(); + let encoded = record.encode().unwrap(); let mut cursor = Cursor::new(encoded); let parsed = parse_mrt_record(&mut cursor).unwrap(); let expected_len = parsed .message .encode(parsed.common_header.entry_subtype) + .unwrap() .len() as u32; assert_eq!(parsed.common_header.length, expected_len); diff --git a/tests/test_encoding.rs b/tests/test_encoding.rs index 7f4ceb3..12d121e 100644 --- a/tests/test_encoding.rs +++ b/tests/test_encoding.rs @@ -9,7 +9,7 @@ mod tests { let url = "https://spaces.bgpkit.org/parser/update-example.gz"; let parser = BgpkitParser::new(url).unwrap(); for record in parser.into_record_iter() { - let bytes = record.encode(); + let bytes = record.encode().unwrap(); let parsed_record = parse_mrt_record(&mut Cursor::new(bytes)).unwrap(); assert_eq!(record, parsed_record); } @@ -17,7 +17,7 @@ mod tests { let url = "http://archive.routeviews.org/bgpdata/2023.10/UPDATES/updates.20231029.2015.bz2"; let parser = BgpkitParser::new(url).unwrap(); for record in parser.into_record_iter() { - let bytes = record.encode(); + let bytes = record.encode().unwrap(); let parsed_record = parse_mrt_record(&mut Cursor::new(bytes)).unwrap(); assert_eq!(record, parsed_record); } @@ -28,7 +28,7 @@ mod tests { let url = "http://archive.routeviews.org/route-views6/bgpdata/2023.10/UPDATES/updates.20231029.2115.bz2"; let parser = BgpkitParser::new(url).unwrap(); for record in parser.into_record_iter() { - let bytes = record.encode(); + let bytes = record.encode().unwrap(); let parsed_record = match parse_mrt_record(&mut Cursor::new(bytes.clone())) { Ok(r) => r, Err(_) => { @@ -48,7 +48,7 @@ mod tests { let url = "http://archive.routeviews.org/route-views.amsix/bgpdata/2023.05/UPDATES/updates.20230505.0330.bz2"; let parser = BgpkitParser::new(url).unwrap(); for record in parser.into_record_iter() { - let bytes = record.encode(); + let bytes = record.encode().unwrap(); let parsed_record = parse_mrt_record(&mut Cursor::new(bytes)).unwrap(); assert_eq!(record, parsed_record); } @@ -71,7 +71,7 @@ mod tests { let mut writer = oneio::get_writer(tempfile.as_str()).unwrap(); for record in input_records.iter() { - let bytes = record.encode(); + let bytes = record.encode().unwrap(); writer.write_all(&bytes).unwrap(); let parsed_record = parse_mrt_record(&mut Cursor::new(bytes)).unwrap(); assert_eq!(*record, parsed_record); diff --git a/tests/test_encoding_errors.rs b/tests/test_encoding_errors.rs new file mode 100644 index 0000000..72629a7 --- /dev/null +++ b/tests/test_encoding_errors.rs @@ -0,0 +1,221 @@ +//! Error-path tests for the fallible encoding API (issue #313). +//! +//! Every wire-format capacity bound must surface as an `EncodingError` +//! instead of silently truncating, wrapping, or dropping data. + +use bgpkit_parser::error::EncodingError; +use bgpkit_parser::models::*; +use std::net::Ipv4Addr; +use std::str::FromStr; + +fn oversized_attribute() -> Attribute { + // 30 large communities = 360 bytes, exceeding the 255-byte limit of a + // non-extended attribute length field + let communities = vec![LargeCommunity::new(1, [2, 3]); 30]; + Attribute { + value: AttributeValue::LargeCommunities(communities), + flag: AttrFlags::OPTIONAL | AttrFlags::TRANSITIVE, + } +} + +#[test] +fn test_oversized_attribute_value_non_extended() { + let attr = oversized_attribute(); + assert!(!attr.is_extended()); + let err = attr.encode(AsnLength::Bits32).unwrap_err(); + assert_eq!( + err, + EncodingError::ValueTooLarge { + field: "BGP attribute value length", + actual: 360, + max: 255 + } + ); +} + +#[test] +fn test_oversized_attribute_value_extended() { + // 6000 large communities = 72000 bytes, exceeding even the extended + // 2-byte attribute length field + let communities = vec![LargeCommunity::new(1, [2, 3]); 6000]; + let attr = Attribute { + value: AttributeValue::LargeCommunities(communities), + flag: AttrFlags::OPTIONAL | AttrFlags::TRANSITIVE | AttrFlags::EXTENDED, + }; + let err = attr.encode(AsnLength::Bits32).unwrap_err(); + assert_eq!( + err, + EncodingError::ValueTooLarge { + field: "BGP attribute value length (extended)", + actual: 72000, + max: 65535 + } + ); +} + +#[test] +fn test_attr_set_is_unencodable() { + let attr = Attribute::from(AttributeValue::AttrSet(AttrSet { + origin_as: Asn::from(65000), + attributes: Attributes::default(), + })); + let err = attr.encode(AsnLength::Bits32).unwrap_err(); + assert!(matches!( + err, + EncodingError::Unencodable { + field: "ATTR_SET attribute", + .. + } + )); +} + +#[test] +fn test_mp_reach_empty_label_stack_propagates() { + // Previously this error was swallowed (log + empty attribute value); + // it must now surface through Attribute::encode. + let nlri = Nlri { + afi: Afi::Ipv4, + safi: Safi::MplsLabel, + next_hop: Some(NextHopAddress::Ipv4(Ipv4Addr::new(192, 0, 2, 1))), + prefixes: vec![], + labeled_prefixes: Some(vec![LabeledNetworkPrefix { + prefix: "203.0.113.0/24".parse().unwrap(), + labels: Default::default(), // empty label stack cannot be encoded + path_id: None, + }]), + link_state_nlris: None, + flowspec_nlris: None, + }; + let attr = Attribute::from(AttributeValue::MpReachNlri(nlri)); + let err = attr.encode(AsnLength::Bits32).unwrap_err(); + assert!(matches!( + err, + EncodingError::Unencodable { + field: "MP NLRI labeled prefix", + .. + } + )); +} + +#[test] +fn test_rib_entry_oversized_attributes_propagate() { + // Regression guard: an entry whose attributes cannot be encoded must fail + // the whole RibAfiEntries::encode, not be silently dropped from the record. + let entry = RibEntry { + peer_index: 0, + originated_time: 0, + path_id: None, + attributes: Attributes::from(vec![oversized_attribute()]), + }; + let rib = RibAfiEntries { + rib_type: TableDumpV2Type::RibIpv4Unicast, + sequence_number: 1, + prefix: NetworkPrefix::from_str("203.0.113.0/24").unwrap(), + rib_entries: vec![entry], + }; + let err = rib.encode().unwrap_err(); + assert_eq!( + err, + EncodingError::ValueTooLarge { + field: "BGP attribute value length", + actual: 360, + max: 255 + } + ); +} + +#[test] +fn test_rib_entry_count_overflow() { + let entry = RibEntry { + peer_index: 0, + originated_time: 0, + path_id: None, + attributes: Attributes::default(), + }; + let rib = RibAfiEntries { + rib_type: TableDumpV2Type::RibIpv4Unicast, + sequence_number: 1, + prefix: NetworkPrefix::from_str("203.0.113.0/24").unwrap(), + rib_entries: vec![entry; 65536], + }; + let err = rib.encode().unwrap_err(); + assert_eq!( + err, + EncodingError::ValueTooLarge { + field: "RIB entry count", + actual: 65536, + max: 65535 + } + ); +} + +#[test] +fn test_open_message_rejects_reserved_param_type_255() { + // RFC 9072 reserves parameter type 255 as the extended-length marker; + // encoding it as a real parameter would be misparsed on round trip. + let msg = BgpOpenMessage { + version: 4, + asn: Asn::new_16bit(64512), + hold_time: 90, + bgp_identifier: Ipv4Addr::new(192, 0, 2, 1), + extended_length: false, + opt_params: vec![OptParam { + param_type: 255, + param_value: ParamValue::Raw(vec![0xAA, 0xBB]), + }], + }; + let err = msg.encode().unwrap_err(); + assert!(matches!( + err, + EncodingError::Unencodable { + field: "BGP OPEN optional parameter type", + .. + } + )); +} + +#[test] +fn test_bgp_message_total_length_overflow() { + // 17000 distinct /24 prefixes at 4 bytes each push the UPDATE body past + // the 16-bit BGP message length field. + let prefixes: Vec = (0..17000u32) + .map(|i| { + NetworkPrefix::from_str(&format!("10.{}.{}.0/24", (i >> 8) & 0xFF, i & 0xFF)).unwrap() + }) + .collect(); + let update = BgpUpdateMessage { + withdrawn_prefixes: vec![], + attributes: Attributes::default(), + announced_prefixes: prefixes, + }; + let err = BgpMessage::Update(update) + .encode(AsnLength::Bits32) + .unwrap_err(); + assert!(matches!( + err, + EncodingError::ValueTooLarge { + field: "BGP message total length", + max: 65535, + .. + } + )); +} + +#[test] +fn test_tunnel_encap_oversized_sub_tlv() { + // Sub-TLV types < 128 carry a 1-octet length field + let mut tlv = TunnelEncapTlv::new(TunnelType::Vxlan); + tlv.add_sub_tlv(SubTlv::new(SubTlvType::Color, vec![0u8; 300])); + let mut attr = TunnelEncapAttribute::new(); + attr.add_tunnel_tlv(tlv); + let attribute = Attribute::from(AttributeValue::TunnelEncapsulation(attr)); + let err = attribute.encode(AsnLength::Bits32).unwrap_err(); + assert_eq!( + err, + EncodingError::ValueTooLarge { + field: "Tunnel Encap sub-TLV value length", + actual: 300, + max: 255 + } + ); +}