Packet: the wire format's shared pieces in one module

What the parsers and writers of messages, attributes and capabilities
had each written for themselves is one module (wire.rs): the shape of a
parser's result and its error, reading a list to the end, checking that
nothing was left over, a non-empty list of fixed-size records (the
community attributes and CLUSTER_LIST, which now fail alike), an AS
number at the session's width, an AFI/SAFI pair, and a length written
before what it counts. open.rs loses its fail! macro and attributes.rs
its own copies of the same.

Tests for what had none: AS4_AGGREGATOR kept opaque and discarded when
malformed; IPv4 routes with IPv6 next hops (RFC 8950) in the property
tests; and what a part-level parser accepts re-encodes to what it read,
but for the fields the decoders normalise.

Nlri::wire_len had no callers; the mask of a prefix's last octet is one
function; a test's name is spelled right; a comment is on the test it
describes.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

crates/packet/src/attributes.rs

@@ -9,17 +9,20 @@ use std::net::{Ipv4Addr, Ipv6Addr};
 
 use bitfield::bitfield;
 use bytes::{BufMut, BytesMut};
-use nom::Err::Failure;
+use nom::Parser;
-use nom::number::complete::{be_u8, be_u16, be_u32};
+use nom::number::complete::{be_u8, be_u32};
-use nom::{IResult, Parser};
 use serde::Serialize;
 use serde_repr::Serialize_repr;
 
-use bgp_primitives::constants::{Afi, AfiSafi, IpVersion, Safi};
+use bgp_primitives::constants::{AfiSafi, IpVersion, Safi};
 use bgp_primitives::ip_prefix::{IpPrefix, Nlri};
 use bgp_primitives::parser::{BgpParserError, EncodeError, ParserContext};
 
-use crate::message::{AS_TRANS, write_nlri};
+use crate::message::write_nlri;
+use crate::wire::{
+    Parsed, list, malformed, parse_afi_safi, parse_asn, put_afi_safi, put_asn, put_length, records,
+    whole,
+};
 
 bitfield! {
     #[derive(Clone, Debug, Serialize, PartialEq, Eq)]
@@ -109,18 +112,6 @@ pub(crate) fn defined_flags(type_code: u8) -> Option<(bool, bool)> {
     })
 }
 
-/// A parsed value, provided the parser used all of it.
-fn whole<T>((rest, parsed): (&[u8], T)) -> Result<T, nom::Err<BgpParserError<&[u8]>>> {
-    if rest.is_empty() {
-        Ok(parsed)
-    } else {
-        Err(Failure(BgpParserError::Malformed(format!(
-            "{} octets left over",
-            rest.len()
-        ))))
-    }
-}
-
 /// One attribute as it lies on the wire, split out but not interpreted.
 #[derive(Debug, Clone)]
 pub struct RawAttribute<'a> {
@@ -228,12 +219,8 @@ impl PathAttribute {
     /// Parses one attribute, header and value, from the front of `buf`.
     /// The flags are not checked against the attribute's definition;
     /// [`crate::message::UpdateMessage::decode`] does that for attributes off a session.
-    pub fn from_wire<'a>(
+    pub fn from_wire<'a>(ctx: &ParserContext, buf: &'a [u8]) -> Parsed<'a, Self> {
-        ctx: &ParserContext,
+        let (raw, rest) = split_attribute(buf).map_err(|e| malformed(e.to_string()))?;
-        buf: &'a [u8],
-    ) -> IResult<&'a [u8], Self, BgpParserError<&'a [u8]>> {
-        let (raw, rest) =
-            split_attribute(buf).map_err(|e| Failure(BgpParserError::Malformed(e.to_string())))?;
         let attribute = Self::parse_value(ctx, raw.flags, raw.type_code, raw.value)?;
         Ok((rest, attribute))
     }
@@ -249,32 +236,50 @@ impl PathAttribute {
     ) -> Result<Self, nom::Err<BgpParserError<&'a [u8]>>> {
         use attr_type::*;
         Ok(match type_code {
-            ORIGIN => Self::Origin(whole(OriginPathAttribute::from_wire(value)?)?),
+            ORIGIN => Self::Origin(whole("ORIGIN", OriginPathAttribute::from_wire(value)?)?),
-            AS_PATH => Self::AsPath(whole(AsPathAttribute::from_wire(ctx, value)?)?),
+            AS_PATH => Self::AsPath(whole("AS_PATH", AsPathAttribute::from_wire(ctx, value)?)?),
-            NEXT_HOP => Self::NextHop(whole(NextHopPathAttribute::from_wire(value)?)?),
+            NEXT_HOP => Self::NextHop(whole("NEXT_HOP", NextHopPathAttribute::from_wire(value)?)?),
-            MULTI_EXIT_DISC => {
+            MULTI_EXIT_DISC => Self::MultiExitDisc(whole(
-                Self::MultiExitDisc(whole(MultiExitDiscPathAttribute::from_wire(value)?)?)
+                "MULTI_EXIT_DISC",
-            }
+                MultiExitDiscPathAttribute::from_wire(value)?,
-            LOCAL_PREF => Self::LocalPref(whole(LocalPrefPathAttribute::from_wire(value)?)?),
+            )?),
-            ATOMIC_AGGREGATE => {
+            LOCAL_PREF => Self::LocalPref(whole(
-                Self::AtomicAggregate(whole((value, AtomicAggregatePathAttribute {}))?)
+                "LOCAL_PREF",
-            }
+                LocalPrefPathAttribute::from_wire(value)?,
-            AGGREGATOR => Self::Aggregator(whole(AggregatorPathAttribute::from_wire(ctx, value)?)?),
+            )?),
-            COMMUNITIES => Self::Communities(whole(CommunitiesPathAttribute::from_wire(value)?)?),
+            ATOMIC_AGGREGATE => Self::AtomicAggregate(whole(
-            MP_REACH_NLRI => {
+                "ATOMIC_AGGREGATE",
-                Self::MpReachNlri(whole(MpReachNlriPathAttribute::from_wire(ctx, value)?)?)
+                (value, AtomicAggregatePathAttribute {}),
-            }
+            )?),
-            MP_UNREACH_NLRI => {
+            AGGREGATOR => Self::Aggregator(whole(
-                Self::MpUnreachNlri(whole(MpUnreachNlriPathAttribute::from_wire(ctx, value)?)?)
+                "AGGREGATOR",
-            }
+                AggregatorPathAttribute::from_wire(ctx, value)?,
+            )?),
+            COMMUNITIES => Self::Communities(whole(
+                "COMMUNITIES",
+                CommunitiesPathAttribute::from_wire(value)?,
+            )?),
+            MP_REACH_NLRI => Self::MpReachNlri(whole(
+                "MP_REACH_NLRI",
+                MpReachNlriPathAttribute::from_wire(ctx, value)?,
+            )?),
+            MP_UNREACH_NLRI => Self::MpUnreachNlri(whole(
+                "MP_UNREACH_NLRI",
+                MpUnreachNlriPathAttribute::from_wire(ctx, value)?,
+            )?),
             EXTENDED_COMMUNITIES => Self::ExtendedCommunities(whole(
+                "EXTENDED_COMMUNITIES",
                 ExtendedCommunitiesPathAttribute::from_wire(value)?,
             )?),
             // Always four octets wide, whatever the session's AS_PATH is.
-            AS4_PATH => Self::As4Path(whole(AsPathAttribute::from_wire_with_width(true, value)?)?),
+            AS4_PATH => Self::As4Path(whole(
-            LARGE_COMMUNITIES => {
+                "AS4_PATH",
-                Self::LargeCommunities(whole(LargeCommunitiesPathAttribute::from_wire(value)?)?)
+                AsPathAttribute::from_wire_with_width(true, value)?,
-            }
+            )?),
+            LARGE_COMMUNITIES => Self::LargeCommunities(whole(
+                "LARGE_COMMUNITIES",
+                LargeCommunitiesPathAttribute::from_wire(value)?,
+            )?),
             ONLY_TO_CUSTOMER => {
                 let asn = <[u8; 4]>::try_from(value).map_err(|_| {
                     malformed(format!(
@@ -293,22 +298,12 @@ impl PathAttribute {
                 })?;
                 Self::OriginatorId(Ipv4Addr::from(id))
             }
-            CLUSTER_LIST => {
+            CLUSTER_LIST => Self::ClusterList(
-                if value.is_empty() || !value.len().is_multiple_of(4) {
+                records::<4>(value, "CLUSTER_LIST")?
-                    return Err(malformed(format!(
-                        "CLUSTER_LIST is {} octets long, not a multiple of four",
-                        value.len()
-                    )));
-                }
-                Self::ClusterList(
-                    value
-                        .as_chunks::<4>()
-                        .0
                     .iter()
                     .map(|id| Ipv4Addr::from(*id))
                     .collect(),
-                )
+            ),
-            }
             _ => Self::UnknownPathAttribute {
                 flags,
                 type_code,
@@ -325,26 +320,21 @@ impl PathAttribute {
         let mut flags = self.flags();
         out.put_u8(0);
         out.put_u8(self.type_code());
+        let length_at = out.len();
         out.put_u8(0);
-        let value_start = out.len();
         self.value_to_wire(ctx, out)?;
-        let length = out.len() - value_start;
+        let length = out.len() - length_at - 1;
 
         if let Ok(short) = u8::try_from(length)
             && !flags.extended_length()
         {
-            out[start + 2] = short;
+            out[length_at] = short;
         } else {
-            let long = u16::try_from(length).map_err(|_| EncodeError::TooLong {
-                what: "path attribute",
-                len: length,
-                max: u16::MAX.into(),
-            })?;
             flags.set_extended_length(true);
             // Move the value up by one octet to make room.
             out.put_u8(0);
-            out.copy_within(value_start..value_start + length, value_start + 1);
+            out.copy_within(length_at + 1..length_at + 1 + length, length_at + 2);
-            out[start + 2..start + 4].copy_from_slice(&long.to_be_bytes());
+            put_length(out, length_at, 2, length, u16::MAX.into(), "path attribute")?;
         }
         out[start] = flags.0;
         Ok(())
@@ -394,12 +384,6 @@ pub fn only_to_customer_wire(asn: u32) -> [u8; 7] {
     ]
 }
 
-type Parsed<'a, T> = IResult<&'a [u8], T, BgpParserError<&'a [u8]>>;
-
-fn malformed<'a>(detail: String) -> nom::Err<BgpParserError<&'a [u8]>> {
-    Failure(BgpParserError::Malformed(detail))
-}
-
 /// ORIGIN: how the route came into BGP (RFC 4271 §5.1.1).
 #[derive(Debug, PartialEq, Eq, Copy, Clone, Serialize_repr)]
 #[repr(u8)]
@@ -518,16 +502,11 @@ impl AsPathAttribute {
                 )));
             }
             if len == 0 {
-                return Err(malformed("AS path segment with no AS numbers".into()));
+                return Err(malformed("AS path segment with no AS numbers"));
             }
             let count = usize::from(len);
-            let (buf, path): (_, Vec<u32>) = if four_octet {
+            let (buf, path) =
-                nom::multi::many_m_n(count, count, be_u32).parse(buf)?
+                nom::multi::many_m_n(count, count, |buf| parse_asn(four_octet, buf)).parse(buf)?;
-            } else {
-                nom::multi::many_m_n(count, count, be_u16)
-                    .parse(buf)
-                    .map(|(buf, asns)| (buf, asns.into_iter().map(u32::from).collect()))?
-            };
             Ok((
                 buf,
                 AsPathSegment {
@@ -536,16 +515,7 @@ impl AsPathAttribute {
                 },
             ))
         };
-
+        let (buf, segments) = list(buf, parse_segment)?;
-        // Not many0: that stops quietly at a segment it cannot read, and a
-        // truncated segment is an error.
-        let mut segments = Vec::new();
-        let mut buf = buf;
-        while !buf.is_empty() {
-            let (rest, segment) = parse_segment(buf)?;
-            segments.push(segment);
-            buf = rest;
-        }
         Ok((buf, Self { segments }))
     }
 
@@ -567,11 +537,7 @@ impl AsPathAttribute {
             })?;
             out.put_u8(count);
             for asn in &segment.path {
-                if four_octet {
+                put_asn(four_octet, *asn, out);
-                    out.put_u32(*asn);
-                } else {
-                    out.put_u16(u16::try_from(*asn).unwrap_or(AS_TRANS));
-                }
             }
         }
         Ok(())
@@ -645,11 +611,7 @@ impl AggregatorPathAttribute {
     /// The AS number is as wide as the session's AS numbers (RFC 6793), so
     /// the attribute is six or eight octets.
     pub fn from_wire<'a>(ctx: &ParserContext, buf: &'a [u8]) -> Parsed<'a, Self> {
-        let (buf, asn) = if ctx.four_octet_asn {
+        let (buf, asn) = parse_asn(ctx.four_octet_asn, buf)?;
-            be_u32(buf)?
-        } else {
-            be_u16(buf).map(|(buf, asn)| (buf, u32::from(asn)))?
-        };
         let (buf, ip) = be_u32(buf)?;
         Ok((
             buf,
@@ -663,11 +625,7 @@ impl AggregatorPathAttribute {
     /// On a two-octet session an AS number that does not fit is written as
     /// AS_TRANS (RFC 6793 §4.2.2).
     pub fn to_wire(&self, ctx: &ParserContext, out: &mut BytesMut) {
-        if ctx.four_octet_asn {
+        put_asn(ctx.four_octet_asn, self.asn, out);
-            out.put_u32(self.asn);
-        } else {
-            out.put_u16(u16::try_from(self.asn).unwrap_or(AS_TRANS));
-        }
         out.put_u32(self.ip.into());
     }
 }
@@ -676,10 +634,13 @@ impl AggregatorPathAttribute {
 pub struct CommunitiesPathAttribute(pub Vec<(u16, u16)>);
 
 impl CommunitiesPathAttribute {
+    /// Reads all of `buf` as communities, four octets each.
     pub fn from_wire(buf: &[u8]) -> Parsed<'_, Self> {
-        // At least one: an empty COMMUNITIES is malformed (RFC 7606 §7.8).
+        let values = records::<4>(buf, "COMMUNITIES")?
-        let (buf, values) = nom::multi::many1(|i| (be_u16, be_u16).parse(i)).parse(buf)?;
+            .iter()
-        Ok((buf, CommunitiesPathAttribute(values)))
+            .map(|[a, b, c, d]| (u16::from_be_bytes([*a, *b]), u16::from_be_bytes([*c, *d])))
+            .collect();
+        Ok((&buf[buf.len()..], CommunitiesPathAttribute(values)))
     }
 
     pub fn to_wire(&self, out: &mut BytesMut) {
@@ -697,16 +658,11 @@ pub struct ExtendedCommunitiesPathAttribute {
 }
 
 impl ExtendedCommunitiesPathAttribute {
+    /// Reads all of `buf` as extended communities, eight octets each.
     pub fn from_wire(buf: &[u8]) -> Parsed<'_, Self> {
-        let (communities, rest) = buf.as_chunks::<8>();
+        let communities = records::<8>(buf, "EXTENDED_COMMUNITIES")?;
-        if communities.is_empty() || !rest.is_empty() {
-            return Err(malformed(format!(
-                "{} octets are not a list of extended communities",
-                buf.len()
-            )));
-        }
         Ok((
-            rest,
+            &buf[buf.len()..],
             Self {
                 extended_communities: communities.iter().copied().map(ExtendedCommunity).collect(),
             },
@@ -743,9 +699,17 @@ pub struct LargeCommunitiesPathAttribute {
 }
 
 impl LargeCommunitiesPathAttribute {
+    /// Reads all of `buf` as large communities, twelve octets each.
     pub fn from_wire(buf: &[u8]) -> Parsed<'_, Self> {
-        let (buf, communities) = nom::multi::many1(LargeCommunity::from_wire).parse(buf)?;
+        let communities = records::<12>(buf, "LARGE_COMMUNITIES")?
-        Ok((buf, Self { communities }))
+            .iter()
+            .map(|community| {
+                let (_, community) =
+                    LargeCommunity::from_wire(community).expect("twelve octets are one");
+                community
+            })
+            .collect();
+        Ok((&buf[buf.len()..], Self { communities }))
     }
 
     pub fn to_wire(&self, out: &mut BytesMut) {
@@ -864,16 +828,9 @@ fn parse_nlri(version: IpVersion, with_path_id: bool, buf: &[u8]) -> Parsed<'_,
 pub(crate) fn parse_nlri_list(
     version: IpVersion,
     with_path_id: bool,
-    mut buf: &[u8],
+    buf: &[u8],
 ) -> Parsed<'_, Vec<Nlri>> {
-    // Not many0: that stops quietly at a prefix it cannot read.
+    list(buf, |buf| parse_nlri(version, with_path_id, buf))
-    let mut nlri = Vec::new();
-    while !buf.is_empty() {
-        let (rest, parsed) = parse_nlri(version, with_path_id, buf)?;
-        nlri.push(parsed);
-        buf = rest;
-    }
-    Ok((buf, nlri))
 }
 
 /// The IP version of the prefixes that make up the NLRI of `family`.
@@ -892,12 +849,6 @@ fn ip_version_of_nlri<'a>(
     IpVersion::try_from(family.afi).map_err(|_| unsupported())
 }
 
-fn parse_family(buf: &[u8]) -> Parsed<'_, AfiSafi> {
-    let (buf, afi) = be_u16(buf)?;
-    let (buf, safi) = be_u8(buf)?;
-    Ok((buf, AfiSafi::new(Afi(afi), Safi(safi))))
-}
-
 /// MP_REACH_NLRI: routes of a family other than the one in the message
 /// body, with their next hop (RFC 4760 §3).
 #[derive(Debug, Clone, Serialize, PartialEq, Eq)]
@@ -909,7 +860,7 @@ pub struct MpReachNlriPathAttribute {
 
 impl MpReachNlriPathAttribute {
     pub fn from_wire<'a>(ctx: &ParserContext, buf: &'a [u8]) -> Parsed<'a, Self> {
-        let (buf, family) = parse_family(buf)?;
+        let (buf, family) = parse_afi_safi(buf)?;
         let version = ip_version_of_nlri(family)?;
 
         let (buf, next_hop_len) = be_u8(buf)?;
@@ -942,8 +893,7 @@ impl MpReachNlriPathAttribute {
     }
 
     pub fn to_wire(&self, ctx: &ParserContext, out: &mut BytesMut) {
-        out.put_u16(self.family.afi.0);
+        put_afi_safi(self.family, out);
-        out.put_u8(self.family.safi.0);
         out.put_u8(self.next_hop.wire_len());
         self.next_hop.to_wire(out);
         out.put_u8(0);
@@ -961,15 +911,14 @@ pub struct MpUnreachNlriPathAttribute {
 
 impl MpUnreachNlriPathAttribute {
     pub fn from_wire<'a>(ctx: &ParserContext, buf: &'a [u8]) -> Parsed<'a, Self> {
-        let (buf, family) = parse_family(buf)?;
+        let (buf, family) = parse_afi_safi(buf)?;
         let version = ip_version_of_nlri(family)?;
         let (buf, prefixes) = parse_nlri_list(version, ctx.has_add_path(family), buf)?;
         Ok((buf, Self { family, prefixes }))
     }
 
     pub fn to_wire(&self, ctx: &ParserContext, out: &mut BytesMut) {
-        out.put_u16(self.family.afi.0);
+        put_afi_safi(self.family, out);
-        out.put_u8(self.family.safi.0);
         write_nlri(&self.prefixes, ctx.has_add_path(self.family), out);
     }
 }

crates/packet/src/lib.rs

@@ -4,3 +4,4 @@ pub mod errors;
 pub mod message;
 pub mod open;
 pub mod update_decode;
+mod wire;

crates/packet/src/message.rs

@@ -15,6 +15,7 @@ use crate::errors::DecodeError;
 use crate::errors::MsgHeaderSubcode;
 use crate::errors::OpenMsgSubcode;
 use crate::open::OpenMessage;
+use crate::wire::{fill_length, put_length};
 
 /// A sentinel AS number used to denote that the actual AS number is provided as
 /// a 4 byte value in the capabilities instead.
@@ -162,30 +163,27 @@ impl Message {
     pub fn to_wire(&self, ctx: &ParserContext, out: &mut BytesMut) -> Result<(), EncodeError> {
         let start = out.len();
         out.put_slice(&BGP_MARKER);
-        // The length, filled in once the body is written.
+        // The length, filled in once the body is written. It counts the
+        // header too.
+        let length_at = out.len();
         out.put_u16(0);
         out.put_u8(self.type_code());
         let written = self.body_to_wire(ctx, out).and_then(|()| {
             let length = out.len() - start;
-            u16::try_from(length)
+            put_length(
-                .ok()
+                out,
-                .filter(|length| *length <= BGP_MAX_MSG_LEN)
+                length_at,
-                .ok_or(EncodeError::TooLong {
+                2,
-                    what: "BGP message",
+                length,
-                    len: length,
+                BGP_MAX_MSG_LEN.into(),
-                    max: BGP_MAX_MSG_LEN.into(),
+                "BGP message",
-                })
+            )
         });
-        match written {
+        if let Err(e) = written {
-            Ok(length) => {
-                out[start + 16..start + 18].copy_from_slice(&length.to_be_bytes());
-                Ok(())
-            }
-            Err(e) => {
             out.truncate(start);
-                Err(e)
+            return Err(e);
-            }
         }
+        Ok(())
     }
 
     fn body_to_wire(&self, ctx: &ParserContext, out: &mut BytesMut) -> Result<(), EncodeError> {
@@ -337,15 +335,19 @@ impl UpdateMessage {
         // The prefixes in the body are IPv4 unicast (RFC 4271 §4.3).
         let with_path_id = ctx.has_add_path(AfiSafi::IPV4_UNICAST);
 
-        let withdrawn = reserve_length(out);
+        // Each field's two-octet length is filled in once the field is
+        // written.
+        let withdrawn = out.len();
+        out.put_u16(0);
         write_nlri(&self.withdrawn_routes, with_path_id, out);
-        fill_length(out, withdrawn, "withdrawn routes")?;
+        fill_length(out, withdrawn, 2, u16::MAX.into(), "withdrawn routes")?;
 
-        let attributes = reserve_length(out);
+        let attributes = out.len();
+        out.put_u16(0);
         for attribute in &self.path_attributes {
             attribute.to_wire(ctx, out)?;
         }
-        fill_length(out, attributes, "path attributes")?;
+        fill_length(out, attributes, 2, u16::MAX.into(), "path attributes")?;
 
         // The NLRI take up the rest of the message.
         write_nlri(&self.announced_routes, with_path_id, out);
@@ -353,26 +355,6 @@ impl UpdateMessage {
     }
 }
 
-/// Leaves room at the end of `out` for a two-octet length that is known
-/// only once what it counts has been written, and returns where.
-fn reserve_length(out: &mut BytesMut) -> usize {
-    let at = out.len();
-    out.put_u16(0);
-    at
-}
-
-/// Fills in the length reserved at `at`: everything written since.
-fn fill_length(out: &mut BytesMut, at: usize, what: &'static str) -> Result<(), EncodeError> {
-    let length = out.len() - at - 2;
-    let field = u16::try_from(length).map_err(|_| EncodeError::TooLong {
-        what,
-        len: length,
-        max: u16::MAX.into(),
-    })?;
-    out[at..at + 2].copy_from_slice(&field.to_be_bytes());
-    Ok(())
-}
-
 /// Writes prefixes as an UPDATE names them: for each, its path identifier
 /// if the session uses them for the family (RFC 7911 §3), its length in
 /// bits, and the bytes that length covers.
@@ -807,7 +789,6 @@ mod tests {
         }
     );
 
-    // Rejects trailing garbage after the NLRI section (a partial prefix).
     /// With ADD-PATH in use for a family, each of its prefixes has a
     /// four-octet path identifier in front (RFC 7911 §3): in the body for
     /// IPv4 unicast, in MP_REACH_NLRI and MP_UNREACH_NLRI for the others.
@@ -1067,6 +1048,7 @@ mod tests {
         assert_eq!(narrow.as4_path(), None);
     }
 
+    /// Rejects trailing garbage after the NLRI section (a partial prefix).
     #[test]
     fn test_update_rejects_trailing_garbage() {
         let ctx = default_v6_context();

crates/packet/src/open.rs

@@ -4,7 +4,7 @@ use bitfield::bitfield;
 use bytes::{BufMut, BytesMut};
 use nom::{
     Err::Failure,
-    IResult, Parser,
+    Parser,
     number::complete::{be_u8, be_u16, be_u24, be_u32},
 };
 use serde::Serialize;
@@ -16,26 +16,8 @@ use bgp_primitives::{
     parser::{BgpParserError, EncodeError, ProtocolErrorValues},
 };
 
-type Parsed<'a, T> = IResult<&'a [u8], T, BgpParserError<&'a [u8]>>;
+use crate::message::BGP4_VERSION;
-
+use crate::wire::{Parsed, fill_length, list, malformed, parse_afi_safi, put_afi_safi, whole};
-macro_rules! fail {
-    ( $($arg:expr),* $(,)? ) => {
-        Failure(BgpParserError::Malformed(format!($($arg),*)))
-    };
-}
-
-/// Parses `buf` to its end as a list of `item`s. Unlike nom's `many0`,
-/// which stops quietly at an item it cannot read, an error in an item is an
-/// error of the list.
-fn list<'a, T>(mut buf: &'a [u8], item: impl Fn(&'a [u8]) -> Parsed<'a, T>) -> Parsed<'a, Vec<T>> {
-    let mut items = Vec::new();
-    while !buf.is_empty() {
-        let (rest, parsed) = item(buf)?;
-        items.push(parsed);
-        buf = rest;
-    }
-    Ok((buf, items))
-}
 
 /// The optional parameter type that marks an OPEN whose optional parameters
 /// have two-octet lengths (RFC 9072).
@@ -62,7 +44,7 @@ impl OpenMessage {
     /// in a well-formed OPEN is nothing.
     pub fn from_wire(buf: &[u8]) -> Parsed<'_, Self> {
         let (buf, version) = be_u8(buf)?;
-        if version != 4 {
+        if version != BGP4_VERSION {
             return Err(Failure(BgpParserError::ProtocolError(
                 ProtocolErrorValues::UnsupportedVersion(version),
             )));
@@ -316,12 +298,6 @@ fn afi_reserved_safi(buf: &[u8]) -> Parsed<'_, (Afi, Safi)> {
     Ok((buf, (Afi(afi), Safi(safi))))
 }
 
-fn afi_safi(buf: &[u8]) -> Parsed<'_, (Afi, Safi)> {
-    let (buf, afi) = be_u16(buf)?;
-    let (buf, safi) = be_u8(buf)?;
-    Ok((buf, (Afi(afi), Safi(safi))))
-}
-
 impl Capability {
     fn type_code(&self) -> u8 {
         use cap_type::*;
@@ -387,7 +363,7 @@ impl Capability {
                     // else: no SAFI is larger than 255.
                     let safi = u8::try_from(safi)
                         .map(Safi)
-                        .map_err(|_| fail!("SAFI {} does not fit in one octet", safi))?;
+                        .map_err(|_| malformed(format!("SAFI {safi} does not fit in one octet")))?;
                     Ok((
                         buf,
                         ExtendedNextHop {
@@ -398,7 +374,7 @@ impl Capability {
                     ))
                 })?;
                 if entries.is_empty() {
-                    return Err(fail!("Extended Next Hop capability with no entries"));
+                    return Err(malformed("Extended Next Hop capability with no entries"));
                 }
                 (rest, Self::ExtendedNextHop { entries })
             }
@@ -416,12 +392,12 @@ impl Capability {
             }
             MULTI_LABEL => {
                 let (rest, entries) = list(value, |buf| {
-                    let (buf, (afi, safi)) = afi_safi(buf)?;
+                    let (buf, AfiSafi { afi, safi }) = parse_afi_safi(buf)?;
                     let (buf, count) = be_u8(buf)?;
                     Ok((buf, MultiLabelEntry { afi, safi, count }))
                 })?;
                 if entries.is_empty() {
-                    return Err(fail!("Multiple Labels capability with no entries"));
+                    return Err(malformed("Multiple Labels capability with no entries"));
                 }
                 (rest, Self::MultiLabelCompat { entries })
             }
@@ -438,7 +414,7 @@ impl Capability {
                 // Four bits of flags and twelve of restart time.
                 let (rest, flags_restart_time) = be_u16(value)?;
                 let (rest, entries) = list(rest, |buf| {
-                    let (buf, (afi, safi)) = afi_safi(buf)?;
+                    let (buf, AfiSafi { afi, safi }) = parse_afi_safi(buf)?;
                     let (buf, flags) = be_u8(buf)?;
                     Ok((
                         buf,
@@ -485,7 +461,7 @@ impl Capability {
             ENHANCED_ROUTE_REFRESH => (value, Self::EnhancedRouteRefresh),
             LONG_LIVED_GRACEFUL_RESTART => {
                 let (rest, entries) = list(value, |buf| {
-                    let (buf, (afi, safi)) = afi_safi(buf)?;
+                    let (buf, AfiSafi { afi, safi }) = parse_afi_safi(buf)?;
                     let (buf, flags) = be_u8(buf)?;
                     let (buf, stale_time) = be_u24(buf)?;
                     Ok((
@@ -508,13 +484,7 @@ impl Capability {
                 },
             ),
         };
-        if !rest.is_empty() {
+        whole(format_args!("capability {type_code}"), (rest, capability))
-            return Err(fail!(
-                "{} octets left over in capability {type_code}",
-                rest.len()
-            ));
-        }
-        Ok(capability)
     }
 
     /// Writes the capability, with its type and length, to the end of
@@ -525,13 +495,7 @@ impl Capability {
         let length_at = out.len();
         out.put_u8(0);
         self.value_to_wire(out)?;
-        let length = out.len() - length_at - 1;
+        fill_length(out, length_at, 1, u8::MAX.into(), "capability")
-        out[length_at] = u8::try_from(length).map_err(|_| EncodeError::TooLong {
-            what: "capability",
-            len: length,
-            max: u8::MAX.into(),
-        })?;
-        Ok(())
     }
 
     fn value_to_wire(&self, out: &mut BytesMut) -> Result<(), EncodeError> {
@@ -562,8 +526,7 @@ impl Capability {
             }
             Capability::MultiLabelCompat { entries } => {
                 for entry in entries {
-                    out.put_u16(entry.afi.0);
+                    put_afi_safi(AfiSafi::new(entry.afi, entry.safi), out);
-                    out.put_u8(entry.safi.0);
                     out.put_u8(entry.count);
                 }
             }
@@ -585,23 +548,20 @@ impl Capability {
                     u16::from(*restart_state) << 15 | u16::from(*notification) << 14 | restart_time,
                 );
                 for entry in entries {
-                    out.put_u16(entry.afi.0);
+                    put_afi_safi(AfiSafi::new(entry.afi, entry.safi), out);
-                    out.put_u8(entry.safi.0);
                     out.put_u8(u8::from(entry.preserve_forwarding) << 7);
                 }
             }
             Capability::FourByteAsn { asn } => out.put_u32(*asn),
             Capability::AddPath { entries } => {
                 for entry in entries {
-                    out.put_u16(entry.afi.0);
+                    put_afi_safi(entry.family(), out);
-                    out.put_u8(entry.safi.0);
                     out.put_u8(entry.send_recv as u8);
                 }
             }
             Capability::LongLivedGracefulRestart(entries) => {
                 for entry in entries {
-                    out.put_u16(entry.afi.0);
+                    put_afi_safi(AfiSafi::new(entry.afi, entry.safi), out);
-                    out.put_u8(entry.safi.0);
                     out.put_u8(entry.flags);
                     out.put_slice(&entry.stale_time.to_be_bytes()[1..]);
                 }
@@ -630,7 +590,7 @@ impl OrfFamily {
             let (rest, orf_type) = be_u8(buf)?;
             let (rest, send_recv) = be_u8(rest)?;
             let send_recv = OrfSendRecv::try_from(send_recv)
-                .map_err(|e| fail!("Failed to parse ORF send/recv: {}", e))?;
+                .map_err(|e| malformed(format!("Failed to parse ORF send/recv: {e}")))?;
             orfs.push(OutboundRouteFilterEntry {
                 r#type: OrfType(orf_type),
                 send_recv,
@@ -843,7 +803,7 @@ mod tests {
     }
 
     test_capability_roundtrip!(
-        test_multiptotocol,
+        test_multiprotocol,
         &[0x01, 0x04, 0x00, 0x01, 0x00, 0x01],
         Capability::MultiProtocol {
             afi: Afi::IPV4,

crates/packet/src/update_decode.rs

@@ -201,8 +201,9 @@ pub fn merge_as4_path(attributes: &mut Vec<PathAttribute>) {
     else {
         return;
     };
-    let PathAttribute::As4Path(wide) = attributes.remove(at) else {
+    let wide = match attributes.remove(at) {
-        return;
+        PathAttribute::As4Path(wide) => wide,
+        other => unreachable!("{other:?} is not the AS4_PATH found at {at}"),
     };
     for attribute in attributes {
         if let PathAttribute::AsPath(path) = attribute {
@@ -501,12 +502,6 @@ mod tests {
         OriginPathAttribute,
     };
 
-    const MED: u8 = 4;
-    const COMMUNITIES: u8 = 8;
-    const EXTENDED_COMMUNITIES: u8 = 16;
-    const LARGE_COMMUNITY: u8 = 32;
-    const ONLY_TO_CUSTOMER: u8 = 35;
-
     const WELL_KNOWN: u8 = 0x40;
     const OPTIONAL: u8 = 0x80;
     const OPTIONAL_TRANSITIVE: u8 = 0xc0;
@@ -602,7 +597,7 @@ mod tests {
 
     #[test]
     fn a_well_formed_update_is_left_alone() {
-        let decoded = announce_with(&ebgp(), attr(OPTIONAL, MED, &[0, 0, 0, 50]));
+        let decoded = announce_with(&ebgp(), attr(OPTIONAL, MULTI_EXIT_DISC, &[0, 0, 0, 50]));
         assert!(decoded.errors.is_empty());
         assert_eq!(decoded.update.announced_routes, [route()]);
         assert_eq!(
@@ -632,7 +627,7 @@ mod tests {
 
         // Nothing when the routes were withdrawn instead: there is no route
         // for the attributes to belong to.
-        let decoded = announce_with(&ebgp(), attr(OPTIONAL, MED, &[1]));
+        let decoded = announce_with(&ebgp(), attr(OPTIONAL, MULTI_EXIT_DISC, &[1]));
         assert_eq!(decoded.attributes_wire, None);
         let decoded = UpdateMessage::decode(&ebgp(), &body(&NLRI, &[], &[])).unwrap();
         assert_eq!(decoded.attributes_wire, None);
@@ -691,7 +686,11 @@ mod tests {
     #[test]
     fn a_malformed_attribute_withdraws_the_routes_and_keeps_the_session() {
         let cases: Vec<(&str, u8, Vec<u8>)> = vec![
-            ("MED of three octets", MED, attr(OPTIONAL, MED, &[0, 0, 50])),
+            (
+                "MED of three octets",
+                MULTI_EXIT_DISC,
+                attr(OPTIONAL, MULTI_EXIT_DISC, &[0, 0, 50]),
+            ),
             (
                 "COMMUNITIES with nothing in it",
                 COMMUNITIES,
@@ -709,13 +708,13 @@ mod tests {
             ),
             (
                 "LARGE_COMMUNITY of eleven octets",
-                LARGE_COMMUNITY,
+                LARGE_COMMUNITIES,
-                attr(OPTIONAL_TRANSITIVE, LARGE_COMMUNITY, &[0; 11]),
+                attr(OPTIONAL_TRANSITIVE, LARGE_COMMUNITIES, &[0; 11]),
             ),
             (
                 "MED marked transitive",
-                MED,
+                MULTI_EXIT_DISC,
-                attr(OPTIONAL_TRANSITIVE, MED, &[0, 0, 0, 50]),
+                attr(OPTIONAL_TRANSITIVE, MULTI_EXIT_DISC, &[0, 0, 0, 50]),
             ),
             // RFC 9234 §5.
             (
@@ -803,13 +802,13 @@ mod tests {
     fn an_attribute_that_overruns_the_attributes_withdraws_what_was_found() {
         // A MED that claims 200 octets.
         let mut attributes = mandatory();
-        attributes.push(vec![OPTIONAL, MED, 200, 0, 0]);
+        attributes.push(vec![OPTIONAL, MULTI_EXIT_DISC, 200, 0, 0]);
         let decoded = UpdateMessage::decode(&ebgp(), &body(&[], &attributes, &NLRI)).unwrap();
-        assert_withdrawn(&decoded, Some(MED), "overrun");
+        assert_withdrawn(&decoded, Some(MULTI_EXIT_DISC), "overrun");
 
         // Two stray octets after the last attribute: not even a header.
         let mut attributes = mandatory();
-        attributes.push(vec![OPTIONAL, MED]);
+        attributes.push(vec![OPTIONAL, MULTI_EXIT_DISC]);
         let decoded = UpdateMessage::decode(&ebgp(), &body(&[], &attributes, &NLRI)).unwrap();
         assert_withdrawn(&decoded, None, "stray octets");
     }
@@ -860,10 +859,13 @@ mod tests {
     #[test]
     fn a_repeated_attribute_keeps_its_first_value() {
         let mut attributes = mandatory();
-        attributes.push(attr(OPTIONAL, MED, &[0, 0, 0, 50]));
+        attributes.push(attr(OPTIONAL, MULTI_EXIT_DISC, &[0, 0, 0, 50]));
-        attributes.push(attr(OPTIONAL, MED, &[0, 0, 0, 99]));
+        attributes.push(attr(OPTIONAL, MULTI_EXIT_DISC, &[0, 0, 0, 99]));
         let decoded = UpdateMessage::decode(&ebgp(), &body(&[], &attributes, &NLRI)).unwrap();
-        assert_eq!(handled(&decoded), [(Handling::AttributeDiscard, Some(MED))]);
+        assert_eq!(
+            handled(&decoded),
+            [(Handling::AttributeDiscard, Some(MULTI_EXIT_DISC))]
+        );
         assert!(
             decoded
                 .update
@@ -972,6 +974,56 @@ mod tests {
         assert_eq!(decoded.update.announced_routes, [route()]);
     }
 
+    /// AS4_AGGREGATOR (RFC 6793 §4.2.2) is kept as it came, once its
+    /// length is known to be the four octets of an AS number and the four
+    /// of an address. One of another length is dropped and the route kept
+    /// (RFC 6793 §6), like a malformed AGGREGATOR.
+    #[test]
+    fn as4_aggregator_is_kept_opaque_and_dropped_if_malformed() {
+        let ctx = ParserContext {
+            four_octet_asn: false,
+            ibgp: false,
+            add_path: Vec::new(),
+        };
+        let attributes = |as4_aggregator: Vec<u8>| {
+            vec![
+                origin(),
+                attr(WELL_KNOWN, AS_PATH, &[2, 1, 0x5b, 0xa0]),
+                next_hop(),
+                as4_aggregator,
+            ]
+        };
+        // AS 4200000001, 192.0.2.9.
+        let value = [0xfa, 0x56, 0xea, 0x01, 192, 0, 2, 9];
+        let good = attr(OPTIONAL_TRANSITIVE, AS4_AGGREGATOR, &value);
+        let decoded = UpdateMessage::decode(&ctx, &body(&[], &attributes(good), &NLRI)).unwrap();
+        assert!(decoded.errors.is_empty());
+        assert_eq!(
+            decoded.update.path_attributes.last(),
+            Some(&PathAttribute::UnknownPathAttribute {
+                flags: PathAttributeFlags(OPTIONAL_TRANSITIVE),
+                type_code: AS4_AGGREGATOR,
+                payload: value.to_vec(),
+            })
+        );
+
+        for (case, bad) in [
+            ("two-octet AS number", &value[2..]),
+            ("no address", &value[..4]),
+            ("nothing", &[][..]),
+        ] {
+            let bad = attr(OPTIONAL_TRANSITIVE, AS4_AGGREGATOR, bad);
+            let decoded = UpdateMessage::decode(&ctx, &body(&[], &attributes(bad), &NLRI)).unwrap();
+            assert_eq!(
+                handled(&decoded),
+                [(Handling::AttributeDiscard, Some(AS4_AGGREGATOR))],
+                "{case}"
+            );
+            assert_eq!(decoded.update.announced_routes, [route()], "{case}");
+            assert_eq!(decoded.update.path_attributes.len(), 3, "{case}");
+        }
+    }
+
     /// RFC 6793 §4.2.3. AS4_PATH is the tail of the path; the front is
     /// kept from AS_PATH. One longer than the path is not used, and is
     /// gone all the same: nothing after the decoder is to see two paths,

crates/packet/src/wire.rs

@@ -0,0 +1,189 @@
+//! What the readers and writers of the wire format share: the shape of a
+//! parser's result and its errors, the fields that several messages and
+//! attributes have in common, and the lengths that are written before what
+//! they count.
+
+use bytes::{BufMut, BytesMut};
+use nom::Err::Failure;
+use nom::IResult;
+use nom::number::complete::{be_u8, be_u16, be_u32};
+
+use bgp_primitives::constants::{Afi, AfiSafi, Safi};
+use bgp_primitives::parser::{BgpParserError, EncodeError};
+
+use crate::message::AS_TRANS;
+
+/// A parser's result: what is left of the input, and what was read.
+pub(crate) type Parsed<'a, T> = IResult<&'a [u8], T, BgpParserError<&'a [u8]>>;
+
+/// The error for input that does not follow the format; `detail` says how.
+pub(crate) fn malformed<'a>(detail: impl Into<String>) -> nom::Err<BgpParserError<&'a [u8]>> {
+    Failure(BgpParserError::Malformed(detail.into()))
+}
+
+/// Parses `buf` to its end as a list of `item`s. Unlike nom's `many0`,
+/// which stops quietly at an item it cannot read, an error in an item is an
+/// error of the list.
+pub(crate) fn list<'a, T>(
+    mut buf: &'a [u8],
+    item: impl Fn(&'a [u8]) -> Parsed<'a, T>,
+) -> Parsed<'a, Vec<T>> {
+    let mut items = Vec::new();
+    while !buf.is_empty() {
+        let (rest, parsed) = item(buf)?;
+        items.push(parsed);
+        buf = rest;
+    }
+    Ok((buf, items))
+}
+
+/// A parsed value, provided the parser used all of the input. `what` names
+/// the input in the error.
+pub(crate) fn whole<T>(
+    what: impl std::fmt::Display,
+    (rest, parsed): (&[u8], T),
+) -> Result<T, nom::Err<BgpParserError<&[u8]>>> {
+    if rest.is_empty() {
+        Ok(parsed)
+    } else {
+        Err(malformed(format!(
+            "{} octets left over in {what}",
+            rest.len()
+        )))
+    }
+}
+
+/// `value` as a list of one or more records of `N` octets each: how the
+/// community attributes and CLUSTER_LIST are laid out. An empty list is
+/// malformed (RFC 7606 §7.8 for COMMUNITIES, and the others alike). `what`
+/// names the attribute in the error.
+pub(crate) fn records<'a, const N: usize>(
+    value: &'a [u8],
+    what: &str,
+) -> Result<&'a [[u8; N]], nom::Err<BgpParserError<&'a [u8]>>> {
+    let (records, rest) = value.as_chunks::<N>();
+    if records.is_empty() || !rest.is_empty() {
+        return Err(malformed(format!(
+            "{what} is {} octets long, not a non-empty multiple of {N}",
+            value.len()
+        )));
+    }
+    Ok(records)
+}
+
+/// Parses an AS number as wide as the session's: four octets or two
+/// (RFC 6793).
+pub(crate) fn parse_asn(four_octet: bool, buf: &[u8]) -> Parsed<'_, u32> {
+    if four_octet {
+        be_u32(buf)
+    } else {
+        be_u16(buf).map(|(buf, asn)| (buf, u32::from(asn)))
+    }
+}
+
+/// Writes an AS number as wide as the session's. On a two-octet session
+/// one that does not fit is written as AS_TRANS (RFC 6793 §4.2.2).
+pub(crate) fn put_asn(four_octet: bool, asn: u32, out: &mut BytesMut) {
+    if four_octet {
+        out.put_u32(asn);
+    } else {
+        out.put_u16(u16::try_from(asn).unwrap_or(AS_TRANS));
+    }
+}
+
+/// Parses an address family as the multiprotocol attributes and most
+/// capabilities name it: a two-octet AFI and a one-octet SAFI.
+pub(crate) fn parse_afi_safi(buf: &[u8]) -> Parsed<'_, AfiSafi> {
+    let (buf, afi) = be_u16(buf)?;
+    let (buf, safi) = be_u8(buf)?;
+    Ok((buf, AfiSafi::new(Afi(afi), Safi(safi))))
+}
+
+/// Writes an address family as [`parse_afi_safi`] reads it.
+pub(crate) fn put_afi_safi(family: AfiSafi, out: &mut BytesMut) {
+    out.put_u16(family.afi.0);
+    out.put_u8(family.safi.0);
+}
+
+/// Writes `length` into the field of `width` octets reserved at `at`, if
+/// it is no more than `max`. `what` names the field in the error.
+pub(crate) fn put_length(
+    out: &mut BytesMut,
+    at: usize,
+    width: usize,
+    length: usize,
+    max: usize,
+    what: &'static str,
+) -> Result<(), EncodeError> {
+    if length > max {
+        return Err(EncodeError::TooLong {
+            what,
+            len: length,
+            max,
+        });
+    }
+    let bytes = length.to_be_bytes();
+    out[at..at + width].copy_from_slice(&bytes[bytes.len() - width..]);
+    Ok(())
+}
+
+/// Fills in the field of `width` octets reserved at `at` with how much
+/// has been written after it, if that is no more than `max`: for a length
+/// that is known only once what it counts has been written. `what` names
+/// the field in the error.
+pub(crate) fn fill_length(
+    out: &mut BytesMut,
+    at: usize,
+    width: usize,
+    max: usize,
+    what: &'static str,
+) -> Result<(), EncodeError> {
+    let length = out.len() - at - width;
+    put_length(out, at, width, length, max, what)
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn a_length_is_filled_in_after_what_it_counts() {
+        let mut out = BytesMut::new();
+        out.put_u8(0xaa);
+        let at = out.len();
+        out.put_u16(0);
+        out.put_slice(&[1, 2, 3]);
+        fill_length(&mut out, at, 2, u16::MAX.into(), "field").unwrap();
+        assert_eq!(&out[..], [0xaa, 0, 3, 1, 2, 3]);
+
+        let mut out = BytesMut::new();
+        out.put_u8(0);
+        out.put_slice(&[0; 300]);
+        assert_eq!(
+            fill_length(&mut out, 0, 1, u8::MAX.into(), "field"),
+            Err(EncodeError::TooLong {
+                what: "field",
+                len: 300,
+                max: 255,
+            })
+        );
+    }
+
+    #[test]
+    fn records_are_a_whole_number_of_them_and_at_least_one() {
+        assert_eq!(
+            records::<4>(&[1, 2, 3, 4, 5, 6, 7, 8], "X").unwrap().len(),
+            2
+        );
+        for value in [&[][..], &[1, 2, 3][..], &[1, 2, 3, 4, 5][..]] {
+            let error = records::<4>(value, "X").unwrap_err();
+            assert_eq!(
+                crate::message::describe(&error),
+                format!(
+                    "X is {} octets long, not a non-empty multiple of 4",
+                    value.len()
+                )
+            );
+        }
+    }
+}

crates/packet/tests/properties.rs

@@ -137,19 +137,6 @@ fn extended_community() -> impl Strategy<Value = ExtendedCommunity> {
 
 fn mp_reach(session: Session) -> impl Strategy<Value = MpReachNlriPathAttribute> {
     let ip_safi = || prop_oneof![Just(Safi::UNICAST), Just(Safi::MULTICAST)];
-    let v4 = ip_safi()
-        .prop_flat_map(move |safi| {
-            (
-                Just(safi),
-                any::<[u8; 4]>(),
-                nlri(session, IpVersion::Ipv4, safi),
-            )
-        })
-        .prop_map(|(safi, next_hop, prefixes)| MpReachNlriPathAttribute {
-            family: AfiSafi::new(Afi::IPV4, safi),
-            next_hop: NlriNextHop::Ipv4(Ipv4Addr::from(next_hop)),
-            prefixes,
-        });
     let v6_next_hop = || {
         prop_oneof![
             any::<[u8; 16]>().prop_map(|a| NlriNextHop::Ipv6(Ipv6Addr::from(a))),
@@ -161,6 +148,27 @@ fn mp_reach(session: Session) -> impl Strategy<Value = MpReachNlriPathAttribute>
             }),
         ]
     };
+    // IPv4 routes have an IPv4 next hop, or an IPv6 one over a link that
+    // has only IPv6 addresses (RFC 8950).
+    let v4_next_hop = move || {
+        prop_oneof![
+            3 => any::<[u8; 4]>().prop_map(|a| NlriNextHop::Ipv4(Ipv4Addr::from(a))),
+            1 => v6_next_hop(),
+        ]
+    };
+    let v4 = ip_safi()
+        .prop_flat_map(move |safi| {
+            (
+                Just(safi),
+                v4_next_hop(),
+                nlri(session, IpVersion::Ipv4, safi),
+            )
+        })
+        .prop_map(|(safi, next_hop, prefixes)| MpReachNlriPathAttribute {
+            family: AfiSafi::new(Afi::IPV4, safi),
+            next_hop,
+            prefixes,
+        });
     let v6 = ip_safi()
         .prop_flat_map(move |safi| {
             (
@@ -692,7 +700,9 @@ proptest! {
         check_decoding(&ctx, &damaged(&wire, &damage))?;
     }
 
-    /// The parsers below the message level are public too.
+    /// The parsers below the message level are public too. What they
+    /// accept must write back as it was read, where the format leaves
+    /// nothing for the reader to normalise.
     #[test]
     fn noise_is_never_more_than_an_error_for_the_parts(
         session in session(),
@@ -700,8 +710,52 @@ proptest! {
     ) {
         let ctx = session.ctx();
         let _ = OpenMessage::from_wire(&bytes);
-        let _ = Capability::from_wire(&bytes);
         let _ = UpdateMessage::decode(&ctx, &bytes);
-        let _ = PathAttribute::from_wire(&ctx, &bytes);
+
+        if let Ok((rest, capability)) = Capability::from_wire(&bytes) {
+            let consumed = &bytes[..bytes.len() - rest.len()];
+            let mut rewritten = BytesMut::new();
+            capability.to_wire(&mut rewritten)
+                .map_err(|e| TestCaseError::fail(format!("accepted but cannot be written: {e}")))?;
+            // ADD-PATH families with a Send/Receive value nobody defined
+            // are left out (RFC 7911 §4), and the reserved octet of
+            // Multiprotocol and ORF and the reserved bits of Graceful
+            // Restart are read as zero whatever they were. Everything else
+            // is written as it was read.
+            let normalised = matches!(
+                capability,
+                Capability::AddPath { .. }
+                    | Capability::MultiProtocol { .. }
+                    | Capability::OutboundRouteFilter(_)
+                    | Capability::GracefulRestart { .. }
+            );
+            if !normalised {
+                prop_assert_eq!(&rewritten[..], consumed, "{:?}", capability);
+            }
+            let again = Capability::from_wire(&rewritten)
+                .map_err(|e| TestCaseError::fail(format!("written but cannot be read: {e:?}")))?;
+            prop_assert_eq!(again, (&[][..], capability));
+        }
+
+        if let Ok((rest, attribute)) = PathAttribute::from_wire(&ctx, &bytes) {
+            let consumed = &bytes[..bytes.len() - rest.len()];
+            let mut rewritten = BytesMut::new();
+            attribute.to_wire(&ctx, &mut rewritten)
+                .map_err(|e| TestCaseError::fail(format!("accepted but cannot be written: {e}")))?;
+            // The flags are not checked against the attribute's definition
+            // here, and are written as defined; and the bits of a prefix
+            // past its length are cleared. With the flags as defined and
+            // no prefixes, the attribute is written as it was read.
+            let names_prefixes = matches!(
+                attribute,
+                PathAttribute::MpReachNlri(_) | PathAttribute::MpUnreachNlri(_)
+            );
+            if rewritten[0] == consumed[0] && !names_prefixes {
+                prop_assert_eq!(&rewritten[..], consumed, "{:?}", attribute);
+            }
+            let again = PathAttribute::from_wire(&ctx, &rewritten)
+                .map_err(|e| TestCaseError::fail(format!("written but cannot be read: {e:?}")))?;
+            prop_assert_eq!(again, (&[][..], attribute));
+        }
     }
 }

crates/primitives/src/ip_prefix.rs

@@ -64,7 +64,7 @@ impl IpPrefix {
         let mut address = [0u8; 16];
         address[..needed].copy_from_slice(&prefix[..needed]);
         if let (Some(last), rem @ 1..) = (address[..needed].last_mut(), length % 8) {
-            *last &= u8::MAX << (8 - rem);
+            *last &= high_bits_mask(rem);
         }
         Ok(Self {
             address_family,
@@ -98,7 +98,7 @@ impl IpPrefix {
         let whole = usize::from(length / 8);
         address[..whole].copy_from_slice(&self.address[..whole]);
         if !length.is_multiple_of(8) {
-            address[whole] = self.address[whole] & (u8::MAX << (8 - length % 8));
+            address[whole] = self.address[whole] & high_bits_mask(length % 8);
         }
         Self {
             address_family: self.address_family,
@@ -122,11 +122,18 @@ impl IpPrefix {
         if rem_bits == 0 {
             return true;
         }
-        let mask = u8::MAX << (8 - rem_bits);
+        let mask = high_bits_mask(rem_bits);
         (self.address[full_bytes] & mask) == (other.address[full_bytes] & mask)
     }
 }
 
+/// The mask that keeps the first `bits` bits of an octet and clears the
+/// rest, for `bits` from 1 to 7: what the last octet of a prefix whose
+/// length is not a whole number of octets is masked with.
+fn high_bits_mask(bits: u8) -> u8 {
+    u8::MAX << (8 - bits)
+}
+
 /// A prefix as an UPDATE names it: the prefix and, on a session where
 /// ADD-PATH is in use for the address family (RFC 7911), the identifier
 /// that tells apart the several routes a peer may announce for it.
@@ -144,11 +151,6 @@ impl Nlri {
     pub fn plain(prefixes: impl IntoIterator<Item = IpPrefix>) -> Vec<Self> {
         prefixes.into_iter().map(Self::from).collect()
     }
-
-    /// The bytes this takes on the wire, with or without its identifier.
-    pub fn wire_len(&self, with_path_id: bool) -> usize {
-        1 + self.prefix.bytes().len() + if with_path_id { 4 } else { 0 }
-    }
 }
 
 impl From<IpPrefix> for Nlri {