Handle errors in UPDATE messages as RFC 7606 says, not by resetting

Any malformed UPDATE used to reset the session, withdrawing every route
learned over it. One bad attribute, typically originated ASes away and
passed on by a peer that did not look at it, could take down a session
carrying a full table.

UpdateMessage::decode now applies RFC 7606 and returns the UPDATE as it is
to be acted on, plus a report of what was wrong:

- Treat-as-withdraw: a malformed or misflagged attribute, a missing
  mandatory one, or an attribute that overruns the attribute list. The
  routes the UPDATE announced (in the body and in MP_REACH_NLRI) are
  withdrawn.
- Attribute discard: malformed ATOMIC_AGGREGATE, AGGREGATOR, AS4_PATH and
  AS4_AGGREGATOR; an attribute sent twice (the first is kept); LOCAL_PREF,
  ORIGINATOR_ID and CLUSTER_LIST from an external peer; AS4_PATH between
  four-octet speakers.
- Session reset remains where the prefixes are in doubt: the lengths that
  frame the message, an unreadable prefix, a malformed or repeated
  MP_REACH_NLRI or MP_UNREACH_NLRI, an unrecognised well-known attribute.
  Each now sends its own subcode instead of always Malformed Attribute
  List.

The session logs each handled error and counts it in
bgp_update_errors_total{handling}. The parser context says whether the
peer is internal (replacing a field nothing read).

Stricter than before, because the checks did not exist:
- An announcement must carry ORIGIN and AS_PATH, NEXT_HOP for routes in the
  body, and LOCAL_PREF from an internal peer.
- An AS_PATH segment must be an AS_SET or AS_SEQUENCE and not empty.
- An attribute value must be used up entirely by its parser; a NEXT_HOP of
  six octets, say, was accepted before.

Bugs found on the way: we sent ATOMIC_AGGREGATE flagged optional and
AGGREGATOR flagged non-transitive. The flags now come from one table used
for both writing and checking.

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

crates/integration-tests/tests/basic_connection.rs

@@ -5,8 +5,9 @@ use bgp_integration_tests::test_peer::TestPeer;
 use bgp_integration_tests::test_server::TestServer;
 use bgp_packet::errors::{BgpError, CeaseSubcode, FsmSubcode, MsgHeaderSubcode, UpdateMsgSubcode};
 use bgp_packet::message::{
-    Message, MpReachNlriPathAttribute, MpUnreachNlriPathAttribute, NlriNextHop,
+    AsPathAttribute, Message, MpReachNlriPathAttribute, MpUnreachNlriPathAttribute,
-    NotificationMessage, PathAttribute, UpdateMessage,
+    NextHopPathAttribute, NlriNextHop, NotificationMessage, OriginPathAttribute, PathAttribute,
+    UpdateMessage,
 };
 use bgp_primitives::constants::{AddressFamilyId, Afi, Safi};
 use bgp_primitives::ip_prefix::IpPrefix;
@@ -15,6 +16,27 @@ use eyre::Result;
 
 const TIMEOUT: Duration = Duration::from_secs(5);
 
+/// The attributes every announcement of IPv4 routes must carry. An UPDATE
+/// without them has its routes withdrawn instead (RFC 7606).
+fn mandatory_attributes() -> Vec<PathAttribute> {
+    let mut attributes = with_origin_and_as_path(PathAttribute::NextHop(NextHopPathAttribute(
+        Ipv4Addr::new(192, 0, 2, 1),
+    )));
+    // In order of type code: ORIGIN, AS_PATH, NEXT_HOP.
+    attributes.rotate_left(1);
+    attributes
+}
+
+/// `first` followed by the attributes every announcement must carry.
+/// Routes in MP_REACH_NLRI have their next hop there.
+fn with_origin_and_as_path(first: PathAttribute) -> Vec<PathAttribute> {
+    vec![
+        first,
+        PathAttribute::Origin(OriginPathAttribute::IGP),
+        AsPathAttribute::from_asns(vec![65001]),
+    ]
+}
+
 // ---------------------------------------------------------------------------
 // Connectivity
 // ---------------------------------------------------------------------------
@@ -195,7 +217,7 @@ async fn test_message_split_across_a_keepalive_tick_is_still_one_message() -> Re
     let mut update = bytes::BytesMut::new();
     Message::Update(UpdateMessage {
         withdrawn_routes: vec![],
-        path_attributes: vec![],
+        path_attributes: mandatory_attributes(),
         announced_routes: vec![prefix.clone()],
     })
     .to_wire(&peer.ctx, &mut update)?;
@@ -322,7 +344,7 @@ async fn test_server_accepts_update() -> Result<()> {
 
     let update = Message::Update(UpdateMessage {
         withdrawn_routes: vec![],
-        path_attributes: vec![],
+        path_attributes: mandatory_attributes(),
         // 10.0.0.0/8 — one byte of prefix for a /8.
         announced_routes: vec![IpPrefix::new(AddressFamilyId::Ipv4, vec![10], 8)?],
     });
@@ -345,12 +367,14 @@ async fn test_server_accepts_ipv6_update() -> Result<()> {
     let prefix = IpPrefix::new(AddressFamilyId::Ipv6, vec![0x20, 0x01, 0x0d, 0xb8], 32)?;
     let update = Message::Update(UpdateMessage {
         withdrawn_routes: vec![],
-        path_attributes: vec![PathAttribute::MpReachNlri(MpReachNlriPathAttribute {
+        path_attributes: with_origin_and_as_path(PathAttribute::MpReachNlri(
-            afi: Afi::IPV6,
+            MpReachNlriPathAttribute {
-            safi: Safi::UNICAST,
+                afi: Afi::IPV6,
-            next_hop: NlriNextHop::Ipv6("2001:db8::1".parse().unwrap()),
+                safi: Safi::UNICAST,
-            prefixes: vec![prefix],
+                next_hop: NlriNextHop::Ipv6("2001:db8::1".parse().unwrap()),
-        })],
+                prefixes: vec![prefix],
+            },
+        )),
         announced_routes: vec![],
     });
     peer.send(&update).await?;
@@ -377,7 +401,7 @@ async fn test_rib_records_ipv4_announce() -> Result<()> {
     let prefix = IpPrefix::new(AddressFamilyId::Ipv4, vec![10], 8)?;
     peer.send(&Message::Update(UpdateMessage {
         withdrawn_routes: vec![],
-        path_attributes: vec![],
+        path_attributes: mandatory_attributes(),
         announced_routes: vec![prefix.clone()],
     }))
     .await?;
@@ -411,7 +435,7 @@ async fn test_rib_records_ipv4_withdraw() -> Result<()> {
     // Announce first.
     peer.send(&Message::Update(UpdateMessage {
         withdrawn_routes: vec![],
-        path_attributes: vec![],
+        path_attributes: mandatory_attributes(),
         announced_routes: vec![prefix.clone()],
     }))
     .await?;
@@ -452,12 +476,14 @@ async fn test_rib_records_ipv6_announce() -> Result<()> {
     let prefix = IpPrefix::new(AddressFamilyId::Ipv6, vec![0x20, 0x01, 0x0d, 0xb8], 32)?;
     peer.send(&Message::Update(UpdateMessage {
         withdrawn_routes: vec![],
-        path_attributes: vec![PathAttribute::MpReachNlri(MpReachNlriPathAttribute {
+        path_attributes: with_origin_and_as_path(PathAttribute::MpReachNlri(
-            afi: Afi::IPV6,
+            MpReachNlriPathAttribute {
-            safi: Safi::UNICAST,
+                afi: Afi::IPV6,
-            next_hop: NlriNextHop::Ipv6("2001:db8::1".parse().unwrap()),
+                safi: Safi::UNICAST,
-            prefixes: vec![prefix.clone()],
+                next_hop: NlriNextHop::Ipv6("2001:db8::1".parse().unwrap()),
-        })],
+                prefixes: vec![prefix.clone()],
+            },
+        )),
         announced_routes: vec![],
     }))
     .await?;
@@ -491,12 +517,14 @@ async fn test_rib_records_ipv6_withdraw() -> Result<()> {
     // Announce first.
     peer.send(&Message::Update(UpdateMessage {
         withdrawn_routes: vec![],
-        path_attributes: vec![PathAttribute::MpReachNlri(MpReachNlriPathAttribute {
+        path_attributes: with_origin_and_as_path(PathAttribute::MpReachNlri(
-            afi: Afi::IPV6,
+            MpReachNlriPathAttribute {
-            safi: Safi::UNICAST,
+                afi: Afi::IPV6,
-            next_hop: NlriNextHop::Ipv6("2001:db8::1".parse().unwrap()),
+                safi: Safi::UNICAST,
-            prefixes: vec![prefix.clone()],
+                next_hop: NlriNextHop::Ipv6("2001:db8::1".parse().unwrap()),
-        })],
+                prefixes: vec![prefix.clone()],
+            },
+        )),
         announced_routes: vec![],
     }))
     .await?;

crates/integration-tests/tests/update_errors.rs

@@ -0,0 +1,231 @@
+//! End-to-end tests for errors in UPDATE messages (RFC 7606): most of them
+//! cost the routes in the UPDATE, not the session.
+
+use std::net::Ipv4Addr;
+use std::time::Duration;
+
+use bgp_integration_tests::test_peer::TestPeer;
+use bgp_integration_tests::test_server::TestServer;
+use bgp_packet::errors::{BgpError, UpdateMsgSubcode};
+use bgp_packet::message::{LocalPrefPathAttribute, Message, PathAttribute};
+use bgp_primitives::ip_prefix::IpPrefix;
+use bgp_rib::SnapshotEntry;
+use bgp_server::metrics::UPDATE_ERRORS;
+use eyre::{Result, bail};
+
+const LOCAL_ASN: u32 = 65000;
+const PEER_ASN: u32 = 65001;
+const TIMEOUT: Duration = Duration::from_secs(5);
+
+const WELL_KNOWN: u8 = 0x40;
+const OPTIONAL_TRANSITIVE: u8 = 0xc0;
+
+fn attr(flags: u8, type_code: u8, value: &[u8]) -> Vec<u8> {
+    let mut wire = vec![flags, type_code, value.len() as u8];
+    wire.extend_from_slice(value);
+    wire
+}
+
+fn origin() -> Vec<u8> {
+    attr(WELL_KNOWN, 1, &[0])
+}
+
+/// AS_SEQUENCE of the peer's AS, four octets wide.
+fn as_path() -> Vec<u8> {
+    attr(WELL_KNOWN, 2, &[2, 1, 0, 0, 0xfd, 0xe9])
+}
+
+fn next_hop() -> Vec<u8> {
+    attr(WELL_KNOWN, 3, &[192, 0, 2, 1])
+}
+
+/// A whole UPDATE message announcing `nlri` with `attributes`, as bytes, so
+/// that it can be wrong in ways the encoder would not write.
+fn update(attributes: &[Vec<u8>], nlri: &[u8]) -> Vec<u8> {
+    let attributes = attributes.concat();
+    let mut body = vec![0, 0];
+    body.extend_from_slice(&(attributes.len() as u16).to_be_bytes());
+    body.extend_from_slice(&attributes);
+    body.extend_from_slice(nlri);
+
+    let mut wire = vec![0xff; 16];
+    wire.extend_from_slice(&((19 + body.len()) as u16).to_be_bytes());
+    wire.push(2);
+    wire.extend_from_slice(&body);
+    wire
+}
+
+/// 10.1.0.0/16 and 10.2.0.0/16.
+const ROUTE_A: [u8; 3] = [16, 10, 1];
+const ROUTE_B: [u8; 3] = [16, 10, 2];
+
+fn prefix(s: &str) -> IpPrefix {
+    IpPrefix::try_from(s).unwrap()
+}
+
+async fn established(server: &TestServer, asn: u32) -> Result<TestPeer> {
+    let mut peer = TestPeer::connect(server.addr).await?;
+    peer.handshake(asn, Ipv4Addr::new(10, 0, 0, 1), 90).await?;
+    Ok(peer)
+}
+
+/// Waits until the server's Adj-RIB-In holds exactly `prefixes`.
+async fn wait_for_routes(server: &TestServer, prefixes: &[&str]) -> Result<Vec<SnapshotEntry>> {
+    let mut expected: Vec<IpPrefix> = prefixes.iter().map(|p| prefix(p)).collect();
+    expected.sort_by_key(|p| p.to_string());
+    Ok(tokio::time::timeout(TIMEOUT, async {
+        loop {
+            let mut routes = server.adj_rib_in.snapshot().await;
+            routes.sort_by_key(|r| r.prefix.to_string());
+            if routes.iter().map(|r| &r.prefix).eq(expected.iter()) {
+                return routes;
+            }
+            tokio::time::sleep(Duration::from_millis(10)).await;
+        }
+    })
+    .await?)
+}
+
+/// The situation RFC 7606 exists for: a route is in the table, and then an
+/// UPDATE for it arrives with a broken attribute. The route goes; the
+/// session, and the other routes learned over it, stay.
+#[tokio::test]
+async fn test_a_malformed_attribute_withdraws_the_route_and_keeps_the_session() -> Result<()> {
+    let server = TestServer::start(LOCAL_ASN).await?;
+    let mut peer = established(&server, PEER_ASN).await?;
+    let handled_before = UPDATE_ERRORS
+        .with_label_values(&["treat-as-withdraw"])
+        .get();
+
+    let good = [origin(), as_path(), next_hop()];
+    peer.send_raw(&update(&good, &[ROUTE_A, ROUTE_B].concat()))
+        .await?;
+    wait_for_routes(&server, &["10.1.0.0/16", "10.2.0.0/16"]).await?;
+
+    // ORIGIN with a value that is not defined.
+    let bad = [attr(WELL_KNOWN, 1, &[7]), as_path(), next_hop()];
+    peer.send_raw(&update(&bad, &ROUTE_A)).await?;
+    wait_for_routes(&server, &["10.2.0.0/16"]).await?;
+
+    // The session goes on: the route can be announced again.
+    peer.send_raw(&update(&good, &ROUTE_A)).await?;
+    wait_for_routes(&server, &["10.1.0.0/16", "10.2.0.0/16"]).await?;
+    assert_eq!(server.registry.list().len(), 1);
+    assert!(
+        UPDATE_ERRORS
+            .with_label_values(&["treat-as-withdraw"])
+            .get()
+            > handled_before
+    );
+    Ok(())
+}
+
+/// An announcement without a mandatory attribute is not a route we can use.
+#[tokio::test]
+async fn test_a_route_without_as_path_is_not_accepted() -> Result<()> {
+    let server = TestServer::start(LOCAL_ASN).await?;
+    let mut peer = established(&server, PEER_ASN).await?;
+
+    peer.send_raw(&update(&[origin(), next_hop()], &ROUTE_A))
+        .await?;
+    // A well-formed one after it shows that the first was dealt with.
+    peer.send_raw(&update(&[origin(), as_path(), next_hop()], &ROUTE_B))
+        .await?;
+
+    wait_for_routes(&server, &["10.2.0.0/16"]).await?;
+    assert_eq!(server.registry.list().len(), 1);
+    Ok(())
+}
+
+/// A broken attribute that plays no part in choosing a route is dropped,
+/// and the route is kept.
+#[tokio::test]
+async fn test_a_malformed_aggregator_is_dropped_and_the_route_kept() -> Result<()> {
+    let server = TestServer::start(LOCAL_ASN).await?;
+    let mut peer = established(&server, PEER_ASN).await?;
+
+    let attributes = [
+        origin(),
+        as_path(),
+        next_hop(),
+        // AGGREGATOR is eight octets on this session.
+        attr(OPTIONAL_TRANSITIVE, 7, &[0, 0, 0xfd, 0xe9, 192]),
+    ];
+    peer.send_raw(&update(&attributes, &ROUTE_A)).await?;
+
+    let routes = wait_for_routes(&server, &["10.1.0.0/16"]).await?;
+    assert_eq!(routes[0].entry.path_attributes.len(), 3);
+    assert!(
+        !routes[0]
+            .entry
+            .path_attributes
+            .iter()
+            .any(|a| matches!(a, PathAttribute::Aggregator(_)))
+    );
+    Ok(())
+}
+
+/// LOCAL_PREF is a matter between routers of one AS. From an external peer
+/// it is dropped; from an internal one it is kept.
+#[tokio::test]
+async fn test_local_pref_is_kept_only_from_an_internal_peer() -> Result<()> {
+    let local_pref = attr(WELL_KNOWN, 5, &[0, 0, 0, 200]);
+    let has_local_pref = |routes: &[SnapshotEntry]| {
+        routes[0]
+            .entry
+            .path_attributes
+            .contains(&PathAttribute::LocalPref(LocalPrefPathAttribute(200)))
+    };
+
+    let server = TestServer::start(LOCAL_ASN).await?;
+    let mut external = established(&server, PEER_ASN).await?;
+    let attributes = [origin(), as_path(), next_hop(), local_pref.clone()];
+    external.send_raw(&update(&attributes, &ROUTE_A)).await?;
+    let routes = wait_for_routes(&server, &["10.1.0.0/16"]).await?;
+    assert!(!has_local_pref(&routes));
+
+    let server = TestServer::start(LOCAL_ASN).await?;
+    let mut internal = established(&server, LOCAL_ASN).await?;
+    // An internal peer has not added its AS to the path.
+    let attributes = [origin(), attr(WELL_KNOWN, 2, &[]), next_hop(), local_pref];
+    internal.send_raw(&update(&attributes, &ROUTE_A)).await?;
+    let routes = wait_for_routes(&server, &["10.1.0.0/16"]).await?;
+    assert!(has_local_pref(&routes));
+    Ok(())
+}
+
+/// When the prefixes themselves cannot be read there is nothing to
+/// withdraw, and the session is reset as before.
+#[tokio::test]
+async fn test_an_unreadable_prefix_still_resets_the_session() -> Result<()> {
+    let server = TestServer::start(LOCAL_ASN).await?;
+    let mut peer = established(&server, PEER_ASN).await?;
+
+    // An IPv4 prefix of length 33.
+    peer.send_raw(&update(
+        &[origin(), as_path(), next_hop()],
+        &[33, 10, 1, 2, 3, 4],
+    ))
+    .await?;
+
+    loop {
+        match tokio::time::timeout(TIMEOUT, peer.recv()).await?? {
+            Message::KeepAlive => {}
+            Message::Notification(n) => {
+                assert_eq!(
+                    n.error,
+                    BgpError::UpdateMsg(UpdateMsgSubcode::InvalidNetworkField)
+                );
+                break;
+            }
+            other => bail!("expected NOTIFICATION, got {other:?}"),
+        }
+    }
+    tokio::time::timeout(TIMEOUT, async {
+        while !server.registry.list().is_empty() {
+            tokio::time::sleep(Duration::from_millis(10)).await;
+        }
+    })
+    .await?;
+    Ok(())
+}

crates/packet/src/lib.rs

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

crates/packet/src/message.rs

@@ -14,6 +14,7 @@ use serde::{Deserialize, Serialize};
 use serde_repr::{Deserialize_repr, Serialize_repr};
 use strum::EnumDiscriminants;
 
+use crate::update_decode::UpdateError;
 use bgp_primitives::constants::{AddressFamilyId, Afi, AfiSafi, Safi};
 use bgp_primitives::ip_prefix::IpPrefix;
 use bgp_primitives::parser::BgpParserError;
@@ -26,7 +27,6 @@ use crate::errors::DecodeError;
 use crate::errors::InvalidValue;
 use crate::errors::MsgHeaderSubcode;
 use crate::errors::OpenMsgSubcode;
-use crate::errors::UpdateMsgSubcode;
 use crate::open::OpenMessage;
 
 /// A sentinel AS number used to denote that the actual AS number is provided as
@@ -62,7 +62,22 @@ impl Message {
     ///
     /// A failure says which NOTIFICATION reports it, following RFC 4271 §6.1
     /// for the header and §6.2 and §6.3 for OPEN and UPDATE bodies.
+    ///
+    /// An UPDATE comes back as it is to be acted on: errors that RFC 7606
+    /// handles without resetting the session have been handled. Use
+    /// [`Message::decode_with_report`] to learn of them.
     pub fn decode(ctx: &ParserContext, buf: &[u8]) -> Result<Self, DecodeError> {
+        Self::decode_with_report(ctx, buf).map(|(msg, _)| msg)
+    }
+
+    /// As [`Message::decode`], and also returns the errors in an UPDATE
+    /// that were handled without resetting the session. The list is empty
+    /// for a well-formed UPDATE and for every other message.
+    pub fn decode_with_report(
+        ctx: &ParserContext,
+        buf: &[u8],
+    ) -> Result<(Self, Vec<UpdateError>), DecodeError> {
+        let mut update_errors = Vec::new();
         let header_error = |subcode, data: Vec<u8>, detail: String| DecodeError {
             notification: BgpError::MsgHeader(subcode),
             data,
@@ -106,7 +121,7 @@ impl Message {
             )
         };
 
-        match msg_type {
+        let message = match msg_type {
             1 => OpenMessage::from_wire(ctx, body)
                 .map(|(_, open)| Self::Open(open))
                 .map_err(|e| {
@@ -136,13 +151,11 @@ impl Message {
                         detail: format!("malformed OPEN: {}", describe(&e)),
                     }
                 }),
-            2 => UpdateMessage::from_wire(ctx, body)
+            2 => {
-                .map(|(_, update)| Self::Update(update))
+                let decoded = UpdateMessage::decode(ctx, body)?;
-                .map_err(|e| DecodeError {
+                update_errors = decoded.errors;
-                    notification: BgpError::UpdateMsg(UpdateMsgSubcode::MalformedAttrs),
+                Ok(Self::Update(decoded.update))
-                    data: vec![],
+            }
-                    detail: format!("malformed UPDATE: {}", describe(&e)),
-                }),
             3 => NotificationMessage::from_wire(ctx, body)
                 .map(|(_, notification)| Self::Notification(notification))
                 .map_err(|e| bad_length(&format!("malformed NOTIFICATION ({})", describe(&e)))),
@@ -161,7 +174,8 @@ impl Message {
                 vec![other],
                 format!("unknown BGP message type {other}"),
             )),
-        }
+        }?;
+        Ok((message, update_errors))
     }
 
     /// Parses a complete BGP message including the 19-byte header from `buf`.
@@ -269,61 +283,18 @@ pub struct UpdateMessage {
 }
 
 impl UpdateMessage {
-    /// Parses a BGP UPDATE message body per RFC 4271 §4.3. The buffer passed in
+    /// Parses a BGP UPDATE message body (RFC 4271 §4.3), without the
-    /// must contain exactly the UPDATE payload (i.e. without the 19 byte BGP
+    /// 19-octet header, in the shape of the other parsers.
-    /// message header). The NLRI section has no explicit length and is inferred
+    ///
-    /// as the remainder of the buffer, so passing extra trailing bytes will
+    /// This is [`UpdateMessage::decode`] with the report of handled errors
-    /// cause them to be (mis-)parsed as NLRI.
+    /// dropped; use `decode` when it matters what was wrong.
     pub fn from_wire<'a>(
         ctx: &ParserContext,
         buf: &'a [u8],
     ) -> IResult<&'a [u8], Self, BgpParserError<&'a [u8]>> {
-        // Withdrawn routes. Per RFC 4271 these are always IPv4 prefixes; other
+        let decoded = Self::decode(ctx, buf)
-        // address families are carried in the MP_UNREACH_NLRI path attribute.
+            .map_err(|e| Failure(BgpParserError::Malformed(e.to_string())))?;
-        let (buf, withdrawn_len) = be_u16(buf)?;
+        Ok((&[], decoded.update))
-        let (buf, withdrawn_buf) = nom::bytes::take(withdrawn_len as usize).parse(buf)?;
-        let (withdrawn_rem, withdrawn_routes) =
-            nom::multi::many0(|buf| parse_prefix(AddressFamilyId::Ipv4, buf))
-                .parse(withdrawn_buf)?;
-        if !withdrawn_rem.is_empty() {
-            return Err(Failure(BgpParserError::Malformed(format!(
-                "Leftover bytes {:x?} after parsing withdrawn routes",
-                withdrawn_rem
-            ))));
-        }
-
-        // Path attributes.
-        let (buf, total_pa_len) = be_u16(buf)?;
-        let (buf, pa_buf) = nom::bytes::take(total_pa_len as usize).parse(buf)?;
-        let (pa_rem, path_attributes) =
-            nom::multi::many0(|buf| PathAttribute::from_wire(ctx, buf)).parse(pa_buf)?;
-        if !pa_rem.is_empty() {
-            return Err(Failure(BgpParserError::Malformed(format!(
-                "Leftover bytes {:x?} after parsing path attributes",
-                pa_rem
-            ))));
-        }
-
-        // NLRI. As with withdrawn routes, these are IPv4 prefixes per RFC 4271.
-        // They occupy the remainder of the UPDATE body; if the caller has
-        // delimited `buf` correctly then `many0` will consume it entirely.
-        let (buf, announced_routes) =
-            nom::multi::many0(|buf| parse_prefix(AddressFamilyId::Ipv4, buf)).parse(buf)?;
-        if !buf.is_empty() {
-            return Err(Failure(BgpParserError::Malformed(format!(
-                "Leftover bytes {:x?} after parsing NLRI",
-                buf
-            ))));
-        }
-
-        Ok((
-            buf,
-            Self {
-                withdrawn_routes,
-                path_attributes,
-                announced_routes,
-            },
-        ))
     }
 
     pub fn to_wire(&self, ctx: &ParserContext, out: &mut BytesMut) -> Result<(), EncodeError> {
@@ -383,7 +354,7 @@ fn prefix_list_wire_len(prefixes: &[IpPrefix]) -> Result<u16, EncodeError> {
 }
 
 /// A parser failure as text for a log line.
-fn describe(e: &nom::Err<BgpParserError<&[u8]>>) -> String {
+pub(crate) fn describe(e: &nom::Err<BgpParserError<&[u8]>>) -> String {
     match e {
         nom::Err::Incomplete(_) => "truncated".into(),
         nom::Err::Error(inner) | nom::Err::Failure(inner) => match inner {
@@ -410,10 +381,10 @@ bitfield! {
     pub struct PathAttributeFlags(u8);
     impl new;
     u8;
-    optional, set_optional: 7;
+    pub optional, set_optional: 7;
-    transitive, set_transitive: 6;
+    pub transitive, set_transitive: 6;
-    partial, set_partial: 5;
+    pub partial, set_partial: 5;
-    extended_length, set_extended_length: 4;
+    pub extended_length, set_extended_length: 4;
 }
 
 impl Deref for PathAttributeFlags {
@@ -470,6 +441,38 @@ impl From<u8> for PathAttributeDiscriminants {
     }
 }
 
+/// The Optional and Transitive flags that the attribute with this type code
+/// is defined to have, for the attributes whose definition we know. A
+/// received attribute with other values is in error (RFC 7606 §3.c).
+pub(crate) fn defined_flags(type_code: u8) -> Option<(bool, bool)> {
+    const WELL_KNOWN: (bool, bool) = (false, true);
+    const OPTIONAL_TRANSITIVE: (bool, bool) = (true, true);
+    const OPTIONAL_NON_TRANSITIVE: (bool, bool) = (true, false);
+    Some(match type_code {
+        // ORIGIN, AS_PATH, NEXT_HOP, LOCAL_PREF, ATOMIC_AGGREGATE (RFC 4271).
+        1 | 2 | 3 | 5 | 6 => WELL_KNOWN,
+        // MULTI_EXIT_DISC; ORIGINATOR_ID and CLUSTER_LIST (RFC 4456);
+        // MP_REACH_NLRI and MP_UNREACH_NLRI (RFC 4760).
+        4 | 9 | 10 | 14 | 15 => OPTIONAL_NON_TRANSITIVE,
+        // AGGREGATOR, COMMUNITIES, EXTENDED COMMUNITIES, AS4_PATH,
+        // AS4_AGGREGATOR, LARGE_COMMUNITY.
+        7 | 8 | 16 | 17 | 18 | 32 => OPTIONAL_TRANSITIVE,
+        _ => return None,
+    })
+}
+
+/// 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()
+        ))))
+    }
+}
+
 impl PathAttribute {
     fn discriminant(&self) -> u8 {
         match self {
@@ -494,41 +497,14 @@ impl PathAttribute {
         }
     }
 
-    fn optional(&self) -> bool {
+    /// Whether the Optional and Transitive flags are set on this attribute.
+    fn optional_and_transitive(&self) -> (bool, bool) {
         match self {
-            PathAttribute::Origin(_) => false,
+            PathAttribute::UnknownPathAttribute { flags, .. } => {
-            PathAttribute::AsPath(_) => false,
+                (flags.optional(), flags.transitive())
-            PathAttribute::NextHop(_) => false,
+            }
-            PathAttribute::MultiExitDisc(_) => true,
+            known => defined_flags(known.discriminant())
-            PathAttribute::LocalPref(_) => false,
+                .expect("every attribute with a variant has defined flags"),
-            PathAttribute::AtomicAggregate(_) => true,
-            PathAttribute::Aggregator(_) => true,
-            PathAttribute::Communities(_) => true,
-            PathAttribute::MpReachNlri(_) => true,
-            PathAttribute::MpUnreachNlri(_) => true,
-            PathAttribute::ExtendedCommunities(_) => true,
-            PathAttribute::As4Path(_) => true,
-            PathAttribute::LargeCommunities(_) => true,
-            PathAttribute::UnknownPathAttribute { flags, .. } => flags.optional(),
-        }
-    }
-
-    fn transitive(&self) -> bool {
-        match self {
-            PathAttribute::Origin(_) => true,
-            PathAttribute::AsPath(_) => true,
-            PathAttribute::NextHop(_) => true,
-            PathAttribute::MultiExitDisc(_) => false,
-            PathAttribute::LocalPref(_) => true,
-            PathAttribute::AtomicAggregate(_) => true,
-            PathAttribute::Aggregator(_) => false,
-            PathAttribute::Communities(_) => true,
-            PathAttribute::MpReachNlri(_) => false,
-            PathAttribute::MpUnreachNlri(_) => false,
-            PathAttribute::ExtendedCommunities(_) => true,
-            PathAttribute::As4Path(_) => true,
-            PathAttribute::LargeCommunities(_) => true,
-            PathAttribute::UnknownPathAttribute { flags, .. } => flags.transitive(),
         }
     }
 
@@ -572,61 +548,73 @@ impl PathAttribute {
         length: u16,
     ) -> IResult<&'a [u8], Self, BgpParserError<&'a [u8]>> {
         let (buf, pa_buf) = nom::bytes::take(length).parse(buf)?;
+        Ok((buf, Self::parse_value(ctx, discriminant, pa_buf)?))
+    }
 
+    /// Parses the value of an attribute with a variant. `pa_buf` is exactly
+    /// the value, and all of it has to be used.
+    pub(crate) fn parse_value<'a>(
+        ctx: &ParserContext,
+        discriminant: PathAttributeDiscriminants,
+        pa_buf: &'a [u8],
+    ) -> Result<Self, nom::Err<BgpParserError<&'a [u8]>>> {
         let attr: PathAttribute = match discriminant {
             PathAttributeDiscriminants::Origin => {
-                PathAttribute::Origin(OriginPathAttribute::from_wire(ctx, pa_buf)?.1)
+                PathAttribute::Origin(whole(OriginPathAttribute::from_wire(ctx, pa_buf)?)?)
             }
             PathAttributeDiscriminants::AsPath => {
-                PathAttribute::AsPath(AsPathAttribute::from_wire(ctx, pa_buf)?.1)
+                PathAttribute::AsPath(whole(AsPathAttribute::from_wire(ctx, pa_buf)?)?)
             }
             PathAttributeDiscriminants::NextHop => {
-                PathAttribute::NextHop(NextHopPathAttribute::from_wire(ctx, pa_buf)?.1)
+                PathAttribute::NextHop(whole(NextHopPathAttribute::from_wire(ctx, pa_buf)?)?)
-            }
-            PathAttributeDiscriminants::MultiExitDisc => {
-                PathAttribute::MultiExitDisc(MultiExitDiscPathAttribute::from_wire(ctx, pa_buf)?.1)
             }
+            PathAttributeDiscriminants::MultiExitDisc => PathAttribute::MultiExitDisc(whole(
+                MultiExitDiscPathAttribute::from_wire(ctx, pa_buf)?,
+            )?),
             PathAttributeDiscriminants::LocalPref => {
-                PathAttribute::LocalPref(LocalPrefPathAttribute::from_wire(ctx, pa_buf)?.1)
+                PathAttribute::LocalPref(whole(LocalPrefPathAttribute::from_wire(ctx, pa_buf)?)?)
             }
-            PathAttributeDiscriminants::AtomicAggregate => PathAttribute::AtomicAggregate(
+            PathAttributeDiscriminants::AtomicAggregate => PathAttribute::AtomicAggregate(whole(
-                AtomicAggregatePathAttribute::from_wire(ctx, pa_buf)?.1,
+                AtomicAggregatePathAttribute::from_wire(ctx, pa_buf)?,
-            ),
+            )?),
             PathAttributeDiscriminants::Aggregator => {
-                PathAttribute::Aggregator(AggregatorPathAttribute::from_wire(ctx, pa_buf)?.1)
+                PathAttribute::Aggregator(whole(AggregatorPathAttribute::from_wire(ctx, pa_buf)?)?)
-            }
-            PathAttributeDiscriminants::Communities => {
-                PathAttribute::Communities(CommunitiesPathAttribute::from_wire(ctx, pa_buf)?.1)
-            }
-            PathAttributeDiscriminants::MpReachNlri => {
-                PathAttribute::MpReachNlri(MpReachNlriPathAttribute::from_wire(ctx, pa_buf)?.1)
-            }
-            PathAttributeDiscriminants::MpUnreachNlri => {
-                PathAttribute::MpUnreachNlri(MpUnreachNlriPathAttribute::from_wire(ctx, pa_buf)?.1)
             }
+            PathAttributeDiscriminants::Communities => PathAttribute::Communities(whole(
+                CommunitiesPathAttribute::from_wire(ctx, pa_buf)?,
+            )?),
+            PathAttributeDiscriminants::MpReachNlri => PathAttribute::MpReachNlri(whole(
+                MpReachNlriPathAttribute::from_wire(ctx, pa_buf)?,
+            )?),
+            PathAttributeDiscriminants::MpUnreachNlri => PathAttribute::MpUnreachNlri(whole(
+                MpUnreachNlriPathAttribute::from_wire(ctx, pa_buf)?,
+            )?),
             PathAttributeDiscriminants::ExtendedCommunities => PathAttribute::ExtendedCommunities(
-                ExtendedCommunitiesPathAttribute::from_wire(ctx, pa_buf)?.1,
+                whole(ExtendedCommunitiesPathAttribute::from_wire(ctx, pa_buf)?)?,
             ),
             PathAttributeDiscriminants::As4Path => {
-                PathAttribute::As4Path(AsPathAttribute::from_wire_with_width(true, pa_buf)?.1)
+                PathAttribute::As4Path(whole(AsPathAttribute::from_wire_with_width(true, pa_buf)?)?)
+            }
+            PathAttributeDiscriminants::LargeCommunities => PathAttribute::LargeCommunities(whole(
+                LargeCommunitiesPathAttribute::from_wire(ctx, pa_buf)?,
+            )?),
+            PathAttributeDiscriminants::UnknownPathAttribute => {
+                return Err(Failure(BgpParserError::Malformed(
+                    "an unknown attribute has no value parser".into(),
+                )));
             }
-            PathAttributeDiscriminants::LargeCommunities => PathAttribute::LargeCommunities(
-                LargeCommunitiesPathAttribute::from_wire(ctx, pa_buf)?.1,
-            ),
-            PathAttributeDiscriminants::UnknownPathAttribute => unreachable!(
-                "parse_known_path_attribute must never be called with an unknown attribute"
-            ),
         };
-        Ok((buf, attr))
+        Ok(attr)
     }
 
     pub fn to_wire(&self, ctx: &ParserContext, out: &mut BytesMut) -> Result<(), EncodeError> {
         macro_rules! write_path_attribute {
             ($out: expr, $attribute: expr) => {
                 let wire_len = $attribute.wire_len(ctx)?;
+                let (optional, transitive) = self.optional_and_transitive();
                 let attr_flags = PathAttributeFlags::new(
-                    self.optional(),
+                    optional,
-                    self.transitive(),
+                    transitive,
                     /*partial=*/ false,
                     wire_len > 255,
                 );
@@ -865,6 +853,19 @@ impl AsPathAttribute {
         let parse_segment = |buf| -> IResult<&[u8], AsPathSegment, BgpParserError<&[u8]>> {
             let (buf, typ) = be_u8(buf)?;
             let (buf, len) = be_u8(buf)?;
+            // AS_SET and AS_SEQUENCE. The confederation segment types of
+            // RFC 5065 are an error from a peer outside a confederation,
+            // and this speaker is never inside one.
+            if typ != 1 && typ != 2 {
+                return Err(Failure(BgpParserError::Malformed(format!(
+                    "unrecognised AS path segment type {typ}"
+                ))));
+            }
+            if len == 0 {
+                return Err(Failure(BgpParserError::Malformed(
+                    "AS path segment with no AS numbers".into(),
+                )));
+            }
             let count = usize::from(len);
             let (buf, path): (_, Vec<u32>) = if four_octet {
                 nom::multi::many_m_n(count, count, be_u32).parse(buf)?
@@ -882,7 +883,15 @@ impl AsPathAttribute {
             ))
         };
 
-        let (buf, segments) = nom::multi::many0(parse_segment).parse(buf)?;
+        // 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 }))
     }
 
@@ -1101,7 +1110,8 @@ impl CommunitiesPathAttribute {
         _: &ParserContext,
         buf: &'a [u8],
     ) -> IResult<&'a [u8], Self, BgpParserError<&'a [u8]>> {
-        let (buf, values) = nom::multi::many0(|i| (be_u16, be_u16).parse(i)).parse(buf)?;
+        // At least one: an empty COMMUNITIES is malformed (RFC 7606 §7.8).
+        let (buf, values) = nom::multi::many1(|i| (be_u16, be_u16).parse(i)).parse(buf)?;
         Ok((buf, CommunitiesPathAttribute(values)))
     }
 
@@ -1455,7 +1465,7 @@ impl NlriNextHop {
 }
 
 // parse_prefix is a helper function that implements an NLRI parser for the given AFI.
-fn parse_prefix(
+pub(crate) fn parse_prefix(
     afi: AddressFamilyId,
     buf: &[u8],
 ) -> IResult<&[u8], IpPrefix, BgpParserError<&[u8]>> {
@@ -1729,7 +1739,7 @@ mod tests {
     fn default_v6_context() -> ParserContext {
         ParserContext {
             four_octet_asn: Some(true),
-            address_family: Some(AddressFamilyId::Ipv6),
+            ibgp: false,
         }
     }
 
@@ -1812,7 +1822,7 @@ mod tests {
     // Atomic Aggregate Path Attribute.
     test_path_attribute_roundtrip!(
         test_atomic_aggregate,
-        &[0xc0, 0x06, 0x00],
+        &[0x40, 0x06, 0x00],
         PathAttribute::AtomicAggregate(AtomicAggregatePathAttribute {})
     );
 
@@ -1820,7 +1830,7 @@ mod tests {
     test_path_attribute_roundtrip!(
         test_aggregator,
         &[
-            0x80, 0x07, 0x08, 0x00, 0x00, 0xfd, 0xe8, 0xc0, 0xa8, 0x01, 0x01
+            0xc0, 0x07, 0x08, 0x00, 0x00, 0xfd, 0xe8, 0xc0, 0xa8, 0x01, 0x01
         ],
         PathAttribute::Aggregator(AggregatorPathAttribute {
             asn: 65000,
@@ -2106,11 +2116,12 @@ mod tests {
     #[test]
     fn bits_past_the_prefix_length_are_cleared() {
         let ctx = default_v6_context();
-        // 10.255.0.0/12 as sent, with the low four bits of the second octet set.
+        // 10.255.0.0/12 withdrawn, with the low four bits of the second
-        let body = [0, 0, 0, 0, 12, 10, 0xff];
+        // octet set.
+        let body = [0, 3, 12, 10, 0xff, 0, 0];
         let (_, update) = UpdateMessage::from_wire(&ctx, &body).unwrap();
-        assert_eq!(update.announced_routes, vec![v4_prefix(12, &[10, 0xf0])]);
+        assert_eq!(update.withdrawn_routes, vec![v4_prefix(12, &[10, 0xf0])]);
-        assert_eq!(update.announced_routes[0].to_string(), "10.240.0.0/12");
+        assert_eq!(update.withdrawn_routes[0].to_string(), "10.240.0.0/12");
     }
 
     /// An extended community is eight octets whatever its type. The opaque
@@ -2154,7 +2165,7 @@ mod tests {
     fn aggregator_is_six_octets_on_a_two_octet_session() {
         let two_octet = ParserContext {
             four_octet_asn: Some(false),
-            address_family: None,
+            ibgp: false,
         };
         let wire = [0xfd, 0xe8, 192, 0, 2, 1];
         let (rest, parsed) = AggregatorPathAttribute::from_wire(&two_octet, &wire).unwrap();
@@ -2231,11 +2242,11 @@ mod tests {
         };
         let two_octet = ParserContext {
             four_octet_asn: Some(false),
-            address_family: None,
+            ibgp: false,
         };
         let four_octet = ParserContext {
             four_octet_asn: Some(true),
-            address_family: None,
+            ibgp: false,
         };
         let wire = |attribute: &PathAttribute, ctx: &ParserContext| {
             let mut out = BytesMut::new();

crates/packet/src/open.rs

@@ -904,7 +904,7 @@ mod tests {
     };
 
     use bgp_primitives::{
-        constants::{AddressFamilyId, Afi, Safi},
+        constants::{Afi, Safi},
         parser::ParserContext,
     };
 
@@ -928,7 +928,7 @@ mod tests {
     fn default_v6_context() -> ParserContext {
         ParserContext {
             four_octet_asn: Some(true),
-            address_family: Some(AddressFamilyId::Ipv6),
+            ibgp: false,
         }
     }
 

crates/packet/src/update_decode.rs

@@ -0,0 +1,944 @@
+//! Decoding of UPDATE messages with the error handling of RFC 7606.
+//!
+//! RFC 4271 answered every malformed UPDATE by resetting the session, which
+//! withdraws every route learned over it. One bad attribute, often
+//! originated by a router several ASes away and passed on faithfully by a
+//! peer that did not look at it, could take down a session carrying a full
+//! table. RFC 7606 keeps the session where that is safe:
+//!
+//! - **Treat-as-withdraw.** The routes the UPDATE announces are handled as
+//!   if it had withdrawn them. Wrong attributes must not be used, and
+//!   keeping the routes' previous attributes would be wrong too.
+//! - **Attribute discard.** The attribute is dropped and the routes are
+//!   kept, for the few attributes that have no effect on route selection.
+//! - **Session reset** remains for errors that leave the prefixes
+//!   themselves in doubt: if we cannot tell which routes a message is
+//!   about, we cannot withdraw them.
+//!
+//! [`UpdateMessage::decode`] applies this. It returns the UPDATE as it is to
+//! be acted on, with treat-as-withdraw and attribute discard already carried
+//! out, together with a report of what was wrong, for the log.
+
+use std::collections::HashSet;
+
+use bgp_primitives::constants::AddressFamilyId;
+use bgp_primitives::ip_prefix::IpPrefix;
+use bgp_primitives::parser::{BgpParserError, ParserContext};
+
+use crate::errors::{BgpError, DecodeError, UpdateMsgSubcode};
+use crate::message::{
+    MpUnreachNlriPathAttribute, PathAttribute, PathAttributeDiscriminants, PathAttributeFlags,
+    UpdateMessage, defined_flags, describe, parse_prefix,
+};
+
+/// How an error in an UPDATE was handled, short of resetting the session.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum Handling {
+    /// The routes the UPDATE announced were withdrawn instead.
+    TreatAsWithdraw,
+    /// The attribute was dropped; the routes were kept.
+    AttributeDiscard,
+}
+
+impl Handling {
+    pub fn as_str(self) -> &'static str {
+        match self {
+            Self::TreatAsWithdraw => "treat-as-withdraw",
+            Self::AttributeDiscard => "attribute-discard",
+        }
+    }
+}
+
+/// Something wrong in an UPDATE that the session survived.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct UpdateError {
+    pub handling: Handling,
+    /// Type code of the attribute at fault, when it is one attribute.
+    pub attribute: Option<u8>,
+    pub detail: String,
+}
+
+/// An UPDATE as it is to be acted on, and what was wrong with it as sent.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct DecodedUpdate {
+    /// The UPDATE after treat-as-withdraw and attribute discard.
+    pub update: UpdateMessage,
+    /// Empty when the UPDATE was well formed.
+    pub errors: Vec<UpdateError>,
+}
+
+const ORIGIN: u8 = 1;
+const AS_PATH: u8 = 2;
+const NEXT_HOP: u8 = 3;
+const LOCAL_PREF: u8 = 5;
+const ATOMIC_AGGREGATE: u8 = 6;
+const AGGREGATOR: u8 = 7;
+const ORIGINATOR_ID: u8 = 9;
+const CLUSTER_LIST: u8 = 10;
+const MP_REACH_NLRI: u8 = 14;
+const MP_UNREACH_NLRI: u8 = 15;
+const AS4_PATH: u8 = 17;
+const AS4_AGGREGATOR: u8 = 18;
+
+/// How a malformed value of the attribute with this type code is handled
+/// (RFC 7606 §7, and RFC 6793 §6 for the AS4 attributes). `None` means the
+/// session is reset: an error inside MP_REACH_NLRI or MP_UNREACH_NLRI leaves
+/// it unknown which routes the UPDATE is about.
+fn handling_of_malformed(type_code: u8) -> Option<Handling> {
+    match type_code {
+        MP_REACH_NLRI | MP_UNREACH_NLRI => None,
+        // These do not affect which route is selected.
+        ATOMIC_AGGREGATE | AGGREGATOR | AS4_PATH | AS4_AGGREGATOR => {
+            Some(Handling::AttributeDiscard)
+        }
+        _ => Some(Handling::TreatAsWithdraw),
+    }
+}
+
+fn reset(subcode: UpdateMsgSubcode, data: Vec<u8>, detail: String) -> DecodeError {
+    DecodeError {
+        notification: BgpError::UpdateMsg(subcode),
+        data,
+        detail,
+    }
+}
+
+/// Parses a field that is nothing but prefixes. An error here cannot be
+/// recovered from: the prefixes are what treat-as-withdraw would act on.
+fn prefixes(field: &str, mut buf: &[u8]) -> Result<Vec<IpPrefix>, DecodeError> {
+    let mut prefixes = Vec::new();
+    while !buf.is_empty() {
+        let (rest, prefix) = parse_prefix(AddressFamilyId::Ipv4, buf).map_err(|e| {
+            reset(
+                UpdateMsgSubcode::InvalidNetworkField,
+                vec![],
+                format!("malformed {field}: {}", describe(&e)),
+            )
+        })?;
+        prefixes.push(prefix);
+        buf = rest;
+    }
+    Ok(prefixes)
+}
+
+impl UpdateMessage {
+    /// Decodes the body of an UPDATE message (what follows the 19-octet
+    /// header) with the error handling of RFC 7606.
+    ///
+    /// `Err` is an error that resets the session, with the NOTIFICATION to
+    /// send. Every other error is handled here and listed in the result.
+    pub fn decode(ctx: &ParserContext, body: &[u8]) -> Result<DecodedUpdate, DecodeError> {
+        // The two length fields frame the message. If they do not fit, there
+        // is no telling where anything is (RFC 7606 §3.e).
+        let framing = |detail: &str| {
+            reset(
+                UpdateMsgSubcode::MalformedAttrs,
+                vec![],
+                format!("malformed UPDATE: {detail}"),
+            )
+        };
+        let split_field = |buf: &'_ [u8], what: &str| -> Result<(usize, usize), DecodeError> {
+            let length = buf
+                .get(..2)
+                .map(|l| usize::from(u16::from_be_bytes([l[0], l[1]])))
+                .ok_or_else(|| framing(&format!("no room for the {what} length")))?;
+            if buf.len() - 2 < length {
+                return Err(framing(&format!(
+                    "{what} length {length} exceeds the message"
+                )));
+            }
+            Ok((2, 2 + length))
+        };
+        let (start, end) = split_field(body, "withdrawn routes")?;
+        let withdrawn_buf = &body[start..end];
+        let body = &body[end..];
+        let (start, end) = split_field(body, "path attribute")?;
+        let attributes_buf = &body[start..end];
+        let nlri_buf = &body[end..];
+
+        let withdrawn_routes = prefixes("withdrawn routes", withdrawn_buf)?;
+        let announced_routes = prefixes("NLRI", nlri_buf)?;
+
+        let mut errors = Vec::new();
+        let path_attributes = attributes(ctx, attributes_buf, &mut errors)?;
+        let mut update = UpdateMessage {
+            withdrawn_routes,
+            path_attributes,
+            announced_routes,
+        };
+
+        if !errors
+            .iter()
+            .any(|e| e.handling == Handling::TreatAsWithdraw)
+            && let Some(missing) = missing_mandatory(ctx, &update)
+        {
+            errors.push(UpdateError {
+                handling: Handling::TreatAsWithdraw,
+                attribute: Some(missing),
+                detail: format!("mandatory attribute {missing} is missing"),
+            });
+        }
+        if errors
+            .iter()
+            .any(|e| e.handling == Handling::TreatAsWithdraw)
+        {
+            update = as_withdrawal(update);
+        }
+        Ok(DecodedUpdate { update, errors })
+    }
+}
+
+/// Reads the path attributes, leaving out the ones that are discarded.
+fn attributes(
+    ctx: &ParserContext,
+    mut buf: &[u8],
+    errors: &mut Vec<UpdateError>,
+) -> Result<Vec<PathAttribute>, DecodeError> {
+    let mut attributes = Vec::new();
+    let mut seen = HashSet::new();
+    let mut error = |handling, attribute: Option<u8>, detail: String| {
+        errors.push(UpdateError {
+            handling,
+            attribute,
+            detail,
+        })
+    };
+
+    while !buf.is_empty() {
+        // An attribute that does not fit in what is left of the attributes
+        // (RFC 7606 §4). Where the next one would start is unknown, so the
+        // rest is not read. Routes found so far are withdrawn.
+        let header = match buf {
+            [flags, type_code, rest @ ..] => {
+                let flags = PathAttributeFlags(*flags);
+                match (flags.extended_length(), rest) {
+                    (true, [hi, lo, rest @ ..]) => Some((
+                        flags,
+                        *type_code,
+                        usize::from(u16::from_be_bytes([*hi, *lo])),
+                        rest,
+                    )),
+                    (false, [length, rest @ ..]) => {
+                        Some((flags, *type_code, usize::from(*length), rest))
+                    }
+                    _ => None,
+                }
+            }
+            _ => None,
+        };
+        let Some((flags, type_code, length, rest)) = header else {
+            error(
+                Handling::TreatAsWithdraw,
+                None,
+                format!("{} octets are too few for an attribute", buf.len()),
+            );
+            break;
+        };
+        if rest.len() < length {
+            error(
+                Handling::TreatAsWithdraw,
+                Some(type_code),
+                format!(
+                    "attribute {type_code} has length {length} but only {} octets remain",
+                    rest.len()
+                ),
+            );
+            break;
+        }
+        let (value, rest) = rest.split_at(length);
+        let attribute_wire = &buf[..buf.len() - rest.len()];
+        buf = rest;
+
+        // RFC 7606 §3.g: a second MP_REACH_NLRI or MP_UNREACH_NLRI makes it
+        // unclear which routes are meant; a second anything else is ignored.
+        if !seen.insert(type_code) {
+            if matches!(type_code, MP_REACH_NLRI | MP_UNREACH_NLRI) {
+                return Err(reset(
+                    UpdateMsgSubcode::MalformedAttrs,
+                    vec![],
+                    format!("attribute {type_code} appears more than once"),
+                ));
+            }
+            error(
+                Handling::AttributeDiscard,
+                Some(type_code),
+                format!("attribute {type_code} appears more than once; kept the first"),
+            );
+            continue;
+        }
+
+        let Some((optional, transitive)) = defined_flags(type_code) else {
+            // An attribute we have no definition for. One marked well-known
+            // is one every speaker is required to understand (RFC 4271
+            // §6.3); an optional one is carried along as it is.
+            if !flags.optional() {
+                return Err(reset(
+                    UpdateMsgSubcode::UnrecognizedWellKnownAttr,
+                    attribute_wire.to_vec(),
+                    format!("unrecognised well-known attribute {type_code}"),
+                ));
+            }
+            attributes.push(PathAttribute::UnknownPathAttribute {
+                flags,
+                type_code,
+                payload: value.to_vec(),
+            });
+            continue;
+        };
+        let flags_are_wrong = flags.optional() != optional || flags.transitive() != transitive;
+        if flags_are_wrong {
+            // RFC 7606 §3.c. The value is still read below if it is one that
+            // names routes, since those have to be withdrawn.
+            error(
+                Handling::TreatAsWithdraw,
+                Some(type_code),
+                format!(
+                    "attribute {type_code} has flags {:#04x} that conflict with its definition",
+                    flags.0
+                ),
+            );
+            if !matches!(type_code, MP_REACH_NLRI | MP_UNREACH_NLRI) {
+                continue;
+            }
+        }
+
+        // Attributes that only mean something between internal peers are
+        // dropped when an external one sends them (RFC 7606 §7.5, §7.9,
+        // §7.10), well formed or not.
+        if !ctx.ibgp && matches!(type_code, LOCAL_PREF | ORIGINATOR_ID | CLUSTER_LIST) {
+            error(
+                Handling::AttributeDiscard,
+                Some(type_code),
+                format!("attribute {type_code} from an external peer"),
+            );
+            continue;
+        }
+        // A speaker with four-octet AS numbers has no use for AS4_PATH and
+        // AS4_AGGREGATOR and must not send them to another (RFC 6793 §6).
+        if ctx.four_octet_asn == Some(true) && matches!(type_code, AS4_PATH | AS4_AGGREGATOR) {
+            error(
+                Handling::AttributeDiscard,
+                Some(type_code),
+                format!("attribute {type_code} on a session with four-octet AS numbers"),
+            );
+            continue;
+        }
+
+        let parsed = match type_code {
+            // Known by their definition but not modelled: kept as bytes,
+            // once their length has been checked.
+            ORIGINATOR_ID | CLUSTER_LIST | AS4_AGGREGATOR => {
+                let well_formed = match type_code {
+                    ORIGINATOR_ID => value.len() == 4,
+                    CLUSTER_LIST => !value.is_empty() && value.len().is_multiple_of(4),
+                    _ => value.len() == 8,
+                };
+                if well_formed {
+                    Ok(PathAttribute::UnknownPathAttribute {
+                        flags,
+                        type_code,
+                        payload: value.to_vec(),
+                    })
+                } else {
+                    Err(format!("a length of {} is not valid", value.len()))
+                }
+            }
+            _ => {
+                PathAttribute::parse_value(ctx, PathAttributeDiscriminants::from(type_code), value)
+                    .map_err(|e: nom::Err<BgpParserError<&[u8]>>| describe(&e))
+            }
+        };
+        match parsed {
+            Ok(attribute) => attributes.push(attribute),
+            Err(detail) => match handling_of_malformed(type_code) {
+                Some(handling) => error(
+                    handling,
+                    Some(type_code),
+                    format!("malformed attribute {type_code}: {detail}"),
+                ),
+                None => {
+                    return Err(reset(
+                        UpdateMsgSubcode::OptionalAttribute,
+                        attribute_wire.to_vec(),
+                        format!("malformed attribute {type_code}: {detail}"),
+                    ));
+                }
+            },
+        }
+    }
+    Ok(attributes)
+}
+
+/// The type code of a mandatory attribute that an UPDATE announcing routes
+/// lacks (RFC 4271 §5, RFC 4760 §3, RFC 7606 §3.d). An UPDATE that only
+/// withdraws needs no attributes.
+fn missing_mandatory(ctx: &ParserContext, update: &UpdateMessage) -> Option<u8> {
+    let has = |type_code: u8| {
+        update.path_attributes.iter().any(|a| {
+            matches!(
+                (type_code, a),
+                (ORIGIN, PathAttribute::Origin(_))
+                    | (AS_PATH, PathAttribute::AsPath(_))
+                    | (NEXT_HOP, PathAttribute::NextHop(_))
+                    | (LOCAL_PREF, PathAttribute::LocalPref(_))
+            )
+        })
+    };
+    let in_body = !update.announced_routes.is_empty();
+    let in_mp_reach = update
+        .path_attributes
+        .iter()
+        .any(|a| matches!(a, PathAttribute::MpReachNlri(mp) if !mp.prefixes.is_empty()));
+    if !in_body && !in_mp_reach {
+        return None;
+    }
+    let mut required = vec![ORIGIN, AS_PATH];
+    if in_body {
+        // Routes in MP_REACH_NLRI carry their next hop there.
+        required.push(NEXT_HOP);
+    }
+    if ctx.ibgp {
+        required.push(LOCAL_PREF);
+    }
+    required.into_iter().find(|type_code| !has(*type_code))
+}
+
+/// `update` with every route it announces withdrawn instead, and no other
+/// attributes: they are not to be trusted, and a withdrawal has none.
+fn as_withdrawal(update: UpdateMessage) -> UpdateMessage {
+    let mut withdrawn_routes = update.withdrawn_routes;
+    withdrawn_routes.extend(update.announced_routes);
+
+    let mut unreach: Vec<MpUnreachNlriPathAttribute> = Vec::new();
+    let mut withdraw = |afi, safi, prefixes: Vec<IpPrefix>| match unreach
+        .iter_mut()
+        .find(|u| u.afi == afi && u.safi == safi)
+    {
+        Some(existing) => existing.prefixes.extend(prefixes),
+        None => unreach.push(MpUnreachNlriPathAttribute {
+            afi,
+            safi,
+            prefixes,
+        }),
+    };
+    for attribute in update.path_attributes {
+        match attribute {
+            PathAttribute::MpUnreachNlri(mp) => withdraw(mp.afi, mp.safi, mp.prefixes),
+            PathAttribute::MpReachNlri(mp) => withdraw(mp.afi, mp.safi, mp.prefixes),
+            _ => {}
+        }
+    }
+    UpdateMessage {
+        withdrawn_routes,
+        path_attributes: unreach
+            .into_iter()
+            .map(PathAttribute::MpUnreachNlri)
+            .collect(),
+        announced_routes: vec![],
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use std::net::Ipv4Addr;
+
+    use bgp_primitives::constants::{Afi, AfiSafi, Safi};
+
+    use super::*;
+    use crate::message::{
+        AsPathAttribute, LocalPrefPathAttribute, MultiExitDiscPathAttribute, NextHopPathAttribute,
+        OriginPathAttribute,
+    };
+
+    const MED: u8 = 4;
+    const COMMUNITIES: u8 = 8;
+    const EXTENDED_COMMUNITIES: u8 = 16;
+    const LARGE_COMMUNITY: u8 = 32;
+
+    const WELL_KNOWN: u8 = 0x40;
+    const OPTIONAL: u8 = 0x80;
+    const OPTIONAL_TRANSITIVE: u8 = 0xc0;
+
+    fn ebgp() -> ParserContext {
+        ParserContext {
+            four_octet_asn: Some(true),
+            ibgp: false,
+        }
+    }
+
+    fn ibgp() -> ParserContext {
+        ParserContext {
+            four_octet_asn: Some(true),
+            ibgp: true,
+        }
+    }
+
+    fn attr(flags: u8, type_code: u8, value: &[u8]) -> Vec<u8> {
+        let mut wire = vec![flags, type_code, value.len() as u8];
+        wire.extend_from_slice(value);
+        wire
+    }
+
+    fn origin() -> Vec<u8> {
+        attr(WELL_KNOWN, ORIGIN, &[0])
+    }
+
+    /// AS_SEQUENCE of AS65001, four octets wide.
+    fn as_path() -> Vec<u8> {
+        attr(WELL_KNOWN, AS_PATH, &[2, 1, 0, 0, 0xfd, 0xe9])
+    }
+
+    fn next_hop() -> Vec<u8> {
+        attr(WELL_KNOWN, NEXT_HOP, &[192, 0, 2, 1])
+    }
+
+    fn local_pref() -> Vec<u8> {
+        attr(WELL_KNOWN, LOCAL_PREF, &[0, 0, 0, 100])
+    }
+
+    /// The three attributes every IPv4 announcement needs.
+    fn mandatory() -> Vec<Vec<u8>> {
+        vec![origin(), as_path(), next_hop()]
+    }
+
+    /// 10.1.2.0/24
+    const NLRI: [u8; 4] = [24, 10, 1, 2];
+
+    fn route() -> IpPrefix {
+        IpPrefix::try_from("10.1.2.0/24").unwrap()
+    }
+
+    fn body(withdrawn: &[u8], attributes: &[Vec<u8>], nlri: &[u8]) -> Vec<u8> {
+        let attributes = attributes.concat();
+        let mut body = (withdrawn.len() as u16).to_be_bytes().to_vec();
+        body.extend_from_slice(withdrawn);
+        body.extend_from_slice(&(attributes.len() as u16).to_be_bytes());
+        body.extend_from_slice(&attributes);
+        body.extend_from_slice(nlri);
+        body
+    }
+
+    /// An announcement of 10.1.2.0/24 with the mandatory attributes and
+    /// `extra`.
+    fn announce_with(ctx: &ParserContext, extra: Vec<u8>) -> DecodedUpdate {
+        let mut attributes = mandatory();
+        attributes.push(extra);
+        UpdateMessage::decode(ctx, &body(&[], &attributes, &NLRI)).unwrap()
+    }
+
+    fn handled(decoded: &DecodedUpdate) -> Vec<(Handling, Option<u8>)> {
+        decoded
+            .errors
+            .iter()
+            .map(|e| (e.handling, e.attribute))
+            .collect()
+    }
+
+    #[track_caller]
+    fn assert_withdrawn(decoded: &DecodedUpdate, attribute: Option<u8>, case: &str) {
+        assert_eq!(
+            handled(decoded),
+            [(Handling::TreatAsWithdraw, attribute)],
+            "{case}"
+        );
+        assert_eq!(decoded.update.withdrawn_routes, [route()], "{case}");
+        assert!(decoded.update.announced_routes.is_empty(), "{case}");
+        assert!(decoded.update.path_attributes.is_empty(), "{case}");
+    }
+
+    #[test]
+    fn a_well_formed_update_is_left_alone() {
+        let decoded = announce_with(&ebgp(), attr(OPTIONAL, MED, &[0, 0, 0, 50]));
+        assert!(decoded.errors.is_empty());
+        assert_eq!(decoded.update.announced_routes, [route()]);
+        assert_eq!(
+            decoded.update.path_attributes,
+            [
+                PathAttribute::Origin(OriginPathAttribute::IGP),
+                AsPathAttribute::from_asns(vec![65001]),
+                PathAttribute::NextHop(NextHopPathAttribute(Ipv4Addr::new(192, 0, 2, 1))),
+                PathAttribute::MultiExitDisc(MultiExitDiscPathAttribute(50)),
+            ]
+        );
+    }
+
+    #[test]
+    fn an_update_that_announces_nothing_needs_no_attributes() {
+        // End-of-RIB, and a plain withdrawal.
+        for wire in [body(&[], &[], &[]), body(&NLRI, &[], &[])] {
+            let decoded = UpdateMessage::decode(&ebgp(), &wire).unwrap();
+            assert!(decoded.errors.is_empty());
+        }
+    }
+
+    #[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])),
+            (
+                "COMMUNITIES with nothing in it",
+                COMMUNITIES,
+                attr(OPTIONAL_TRANSITIVE, COMMUNITIES, &[]),
+            ),
+            (
+                "COMMUNITIES that is not a multiple of four",
+                COMMUNITIES,
+                attr(OPTIONAL_TRANSITIVE, COMMUNITIES, &[0xfd, 0xe8, 0, 100, 1]),
+            ),
+            (
+                "EXTENDED COMMUNITIES of seven octets",
+                EXTENDED_COMMUNITIES,
+                attr(OPTIONAL_TRANSITIVE, EXTENDED_COMMUNITIES, &[0; 7]),
+            ),
+            (
+                "LARGE_COMMUNITY of eleven octets",
+                LARGE_COMMUNITY,
+                attr(OPTIONAL_TRANSITIVE, LARGE_COMMUNITY, &[0; 11]),
+            ),
+            (
+                "MED marked transitive",
+                MED,
+                attr(OPTIONAL_TRANSITIVE, MED, &[0, 0, 0, 50]),
+            ),
+        ];
+        for (case, type_code, extra) in cases {
+            let decoded = announce_with(&ebgp(), extra);
+            assert_withdrawn(&decoded, Some(type_code), case);
+        }
+    }
+
+    #[test]
+    fn a_malformed_mandatory_attribute_withdraws_the_routes() {
+        // Each replaces the good attribute of its type.
+        let cases: Vec<(&str, u8, Vec<u8>)> = vec![
+            ("ORIGIN of 5", ORIGIN, attr(WELL_KNOWN, ORIGIN, &[5])),
+            (
+                "ORIGIN of two octets",
+                ORIGIN,
+                attr(WELL_KNOWN, ORIGIN, &[0, 0]),
+            ),
+            (
+                "ORIGIN marked optional",
+                ORIGIN,
+                attr(OPTIONAL_TRANSITIVE, ORIGIN, &[0]),
+            ),
+            (
+                "AS_PATH with a confederation segment",
+                AS_PATH,
+                attr(WELL_KNOWN, AS_PATH, &[3, 1, 0, 0, 0xfd, 0xe9]),
+            ),
+            (
+                "AS_PATH with an empty segment",
+                AS_PATH,
+                attr(WELL_KNOWN, AS_PATH, &[2, 0]),
+            ),
+            (
+                "AS_PATH cut short",
+                AS_PATH,
+                attr(WELL_KNOWN, AS_PATH, &[2, 2, 0, 0, 0xfd, 0xe9]),
+            ),
+            (
+                "NEXT_HOP of three octets",
+                NEXT_HOP,
+                attr(WELL_KNOWN, NEXT_HOP, &[192, 0, 2]),
+            ),
+        ];
+        for (case, type_code, bad) in cases {
+            let attributes: Vec<Vec<u8>> = mandatory()
+                .into_iter()
+                .map(|good| {
+                    if good[1] == type_code {
+                        bad.clone()
+                    } else {
+                        good
+                    }
+                })
+                .collect();
+            let decoded = UpdateMessage::decode(&ebgp(), &body(&[], &attributes, &NLRI)).unwrap();
+            assert_withdrawn(&decoded, Some(type_code), case);
+        }
+    }
+
+    #[test]
+    fn a_missing_mandatory_attribute_withdraws_the_routes() {
+        for missing in [ORIGIN, AS_PATH, NEXT_HOP] {
+            let attributes: Vec<Vec<u8>> = mandatory()
+                .into_iter()
+                .filter(|a| a[1] != missing)
+                .collect();
+            let decoded = UpdateMessage::decode(&ebgp(), &body(&[], &attributes, &NLRI)).unwrap();
+            assert_withdrawn(&decoded, Some(missing), &format!("without {missing}"));
+        }
+    }
+
+    #[test]
+    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]);
+        let decoded = UpdateMessage::decode(&ebgp(), &body(&[], &attributes, &NLRI)).unwrap();
+        assert_withdrawn(&decoded, Some(MED), "overrun");
+
+        // Two stray octets after the last attribute: not even a header.
+        let mut attributes = mandatory();
+        attributes.push(vec![OPTIONAL, MED]);
+        let decoded = UpdateMessage::decode(&ebgp(), &body(&[], &attributes, &NLRI)).unwrap();
+        assert_withdrawn(&decoded, None, "stray octets");
+    }
+
+    #[test]
+    fn attributes_that_do_not_affect_selection_are_dropped_and_the_route_kept() {
+        let cases: Vec<(&str, u8, Vec<u8>)> = vec![
+            (
+                "ATOMIC_AGGREGATE with a value",
+                ATOMIC_AGGREGATE,
+                attr(WELL_KNOWN, ATOMIC_AGGREGATE, &[1]),
+            ),
+            (
+                "AGGREGATOR of five octets",
+                AGGREGATOR,
+                attr(OPTIONAL_TRANSITIVE, AGGREGATOR, &[0; 5]),
+            ),
+            ("LOCAL_PREF from an external peer", LOCAL_PREF, local_pref()),
+            (
+                "ORIGINATOR_ID from an external peer",
+                ORIGINATOR_ID,
+                attr(OPTIONAL, ORIGINATOR_ID, &[10, 0, 0, 1]),
+            ),
+            (
+                "CLUSTER_LIST from an external peer",
+                CLUSTER_LIST,
+                attr(OPTIONAL, CLUSTER_LIST, &[10, 0, 0, 1]),
+            ),
+            (
+                "AS4_PATH between four-octet speakers",
+                AS4_PATH,
+                attr(OPTIONAL_TRANSITIVE, AS4_PATH, &[2, 1, 0, 0, 0xfd, 0xe9]),
+            ),
+            ("a second ORIGIN", ORIGIN, origin()),
+        ];
+        for (case, type_code, extra) in cases {
+            let decoded = announce_with(&ebgp(), extra);
+            assert_eq!(
+                handled(&decoded),
+                [(Handling::AttributeDiscard, Some(type_code))],
+                "{case}"
+            );
+            assert_eq!(decoded.update.announced_routes, [route()], "{case}");
+            assert_eq!(decoded.update.path_attributes.len(), 3, "{case}");
+        }
+    }
+
+    #[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, MED, &[0, 0, 0, 99]));
+        let decoded = UpdateMessage::decode(&ebgp(), &body(&[], &attributes, &NLRI)).unwrap();
+        assert_eq!(handled(&decoded), [(Handling::AttributeDiscard, Some(MED))]);
+        assert!(
+            decoded
+                .update
+                .path_attributes
+                .contains(&PathAttribute::MultiExitDisc(MultiExitDiscPathAttribute(
+                    50
+                )))
+        );
+    }
+
+    #[test]
+    fn internal_peers_must_send_local_pref_and_may_send_reflection_attributes() {
+        let kept = announce_with(&ibgp(), local_pref());
+        assert!(kept.errors.is_empty());
+        assert!(
+            kept.update
+                .path_attributes
+                .contains(&PathAttribute::LocalPref(LocalPrefPathAttribute(100)))
+        );
+
+        let missing = UpdateMessage::decode(&ibgp(), &body(&[], &mandatory(), &NLRI)).unwrap();
+        assert_withdrawn(&missing, Some(LOCAL_PREF), "no LOCAL_PREF");
+
+        let short = announce_with(&ibgp(), attr(WELL_KNOWN, LOCAL_PREF, &[0, 0, 100]));
+        assert_withdrawn(&short, Some(LOCAL_PREF), "short LOCAL_PREF");
+
+        let mut attributes = mandatory();
+        attributes.push(local_pref());
+        attributes.push(attr(OPTIONAL, ORIGINATOR_ID, &[10, 0, 0, 1]));
+        attributes.push(attr(OPTIONAL, CLUSTER_LIST, &[10, 0, 0, 2, 10, 0, 0, 3]));
+        let reflected = UpdateMessage::decode(&ibgp(), &body(&[], &attributes, &NLRI)).unwrap();
+        assert!(reflected.errors.is_empty());
+        assert_eq!(reflected.update.path_attributes.len(), 6);
+
+        let mut attributes = mandatory();
+        attributes.push(local_pref());
+        attributes.push(attr(OPTIONAL, ORIGINATOR_ID, &[10, 0, 1]));
+        let bad = UpdateMessage::decode(&ibgp(), &body(&[], &attributes, &NLRI)).unwrap();
+        assert_withdrawn(&bad, Some(ORIGINATOR_ID), "short ORIGINATOR_ID");
+    }
+
+    #[test]
+    fn as4_path_is_read_from_a_two_octet_peer_and_dropped_if_malformed() {
+        let ctx = ParserContext {
+            four_octet_asn: Some(false),
+            ibgp: false,
+        };
+        // On this session AS_PATH is two octets wide.
+        let attributes = |as4_path: Vec<u8>| {
+            vec![
+                origin(),
+                attr(WELL_KNOWN, AS_PATH, &[2, 1, 0x5b, 0xa0]),
+                next_hop(),
+                as4_path,
+            ]
+        };
+        let good = attr(
+            OPTIONAL_TRANSITIVE,
+            AS4_PATH,
+            &[2, 1, 0xfa, 0x56, 0xea, 0x01],
+        );
+        let decoded = UpdateMessage::decode(&ctx, &body(&[], &attributes(good), &NLRI)).unwrap();
+        assert!(decoded.errors.is_empty());
+        assert!(
+            decoded
+                .update
+                .path_attributes
+                .iter()
+                .any(|a| matches!(a, PathAttribute::As4Path(_)))
+        );
+
+        let bad = attr(OPTIONAL_TRANSITIVE, AS4_PATH, &[2, 1, 0xfa, 0x56]);
+        let decoded = UpdateMessage::decode(&ctx, &body(&[], &attributes(bad), &NLRI)).unwrap();
+        assert_eq!(
+            handled(&decoded),
+            [(Handling::AttributeDiscard, Some(AS4_PATH))]
+        );
+        assert_eq!(decoded.update.announced_routes, [route()]);
+    }
+
+    /// 2001:db8:1::/48 via 2001:db8::1.
+    fn mp_reach_v6() -> Vec<u8> {
+        let mut value = vec![0, 2, 1, 16];
+        value.extend_from_slice(&[0x20, 0x01, 0x0d, 0xb8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]);
+        value.extend_from_slice(&[0, 48, 0x20, 0x01, 0x0d, 0xb8, 0x00, 0x01]);
+        attr(OPTIONAL, MP_REACH_NLRI, &value)
+    }
+
+    #[test]
+    fn routes_in_mp_reach_are_withdrawn_too() {
+        let v6 = IpPrefix::try_from("2001:db8:1::/48").unwrap();
+        let withdrawn = |attributes: Vec<Vec<u8>>, case: &str| {
+            let decoded = UpdateMessage::decode(&ebgp(), &body(&[], &attributes, &[])).unwrap();
+            assert!(
+                decoded
+                    .errors
+                    .iter()
+                    .any(|e| e.handling == Handling::TreatAsWithdraw),
+                "{case}"
+            );
+            assert_eq!(
+                decoded.update.path_attributes,
+                [PathAttribute::MpUnreachNlri(MpUnreachNlriPathAttribute {
+                    afi: Afi::IPV6,
+                    safi: Safi::UNICAST,
+                    prefixes: vec![v6.clone()],
+                })],
+                "{case}"
+            );
+        };
+
+        // A well-formed one first: no NEXT_HOP attribute is needed.
+        let good = vec![mp_reach_v6(), origin(), as_path()];
+        let decoded = UpdateMessage::decode(&ebgp(), &body(&[], &good, &[])).unwrap();
+        assert!(decoded.errors.is_empty());
+        assert!(matches!(
+            &decoded.update.path_attributes[0],
+            PathAttribute::MpReachNlri(mp) if mp.family() == AfiSafi::IPV6_UNICAST
+        ));
+
+        withdrawn(
+            vec![mp_reach_v6(), attr(WELL_KNOWN, ORIGIN, &[9]), as_path()],
+            "bad ORIGIN",
+        );
+        withdrawn(vec![mp_reach_v6(), origin()], "no AS_PATH");
+        // Wrong flags on MP_REACH_NLRI itself: its routes are still found.
+        let mut misflagged = mp_reach_v6();
+        misflagged[0] = OPTIONAL_TRANSITIVE;
+        withdrawn(vec![misflagged, origin(), as_path()], "misflagged");
+    }
+
+    #[test]
+    fn errors_that_leave_the_routes_in_doubt_reset_the_session() {
+        let subcode = |wire: Vec<u8>| match UpdateMessage::decode(&ebgp(), &wire) {
+            Err(e) => (e.notification, e.data),
+            Ok(decoded) => panic!("expected a reset, got {decoded:?}"),
+        };
+        let update_error = |subcode| BgpError::UpdateMsg(subcode);
+
+        // The lengths that frame the message do not fit it.
+        assert_eq!(
+            subcode(vec![0, 9, 24, 10, 1, 2, 0, 0]).0,
+            update_error(UpdateMsgSubcode::MalformedAttrs)
+        );
+        assert_eq!(
+            subcode(vec![0, 0, 0, 9, 0x40, 1, 1, 0]).0,
+            update_error(UpdateMsgSubcode::MalformedAttrs)
+        );
+        assert_eq!(
+            subcode(vec![0]).0,
+            update_error(UpdateMsgSubcode::MalformedAttrs)
+        );
+
+        // A prefix that cannot be one.
+        assert_eq!(
+            subcode(body(&[], &mandatory(), &[33, 10, 1, 2, 3, 4])).0,
+            update_error(UpdateMsgSubcode::InvalidNetworkField)
+        );
+        assert_eq!(
+            subcode(body(&[24, 10, 1], &[], &[])).0,
+            update_error(UpdateMsgSubcode::InvalidNetworkField)
+        );
+
+        // Two MP_REACH_NLRI: which routes are meant?
+        assert_eq!(
+            subcode(body(
+                &[],
+                &[mp_reach_v6(), mp_reach_v6(), origin(), as_path()],
+                &[]
+            ))
+            .0,
+            update_error(UpdateMsgSubcode::MalformedAttrs)
+        );
+
+        // MP_REACH_NLRI with a next hop of the wrong length.
+        let bad_mp_reach = attr(OPTIONAL, MP_REACH_NLRI, &[0, 2, 1, 3, 1, 2, 3, 0]);
+        let (error, data) = subcode(body(&[], &[bad_mp_reach.clone(), origin(), as_path()], &[]));
+        assert_eq!(error, update_error(UpdateMsgSubcode::OptionalAttribute));
+        assert_eq!(data, bad_mp_reach);
+
+        // An attribute nobody has defined, marked as one everybody must know.
+        let unknown = attr(WELL_KNOWN, 99, &[1, 2]);
+        let mut attributes = mandatory();
+        attributes.push(unknown.clone());
+        let (error, data) = subcode(body(&[], &attributes, &NLRI));
+        assert_eq!(
+            error,
+            update_error(UpdateMsgSubcode::UnrecognizedWellKnownAttr)
+        );
+        assert_eq!(data, unknown);
+    }
+
+    #[test]
+    fn an_unknown_optional_attribute_is_carried_as_it_came() {
+        let decoded = announce_with(&ebgp(), attr(0xe0, 99, &[1, 2, 3]));
+        assert!(decoded.errors.is_empty());
+        assert_eq!(
+            decoded.update.path_attributes[3],
+            PathAttribute::UnknownPathAttribute {
+                flags: PathAttributeFlags(0xe0),
+                type_code: 99,
+                payload: vec![1, 2, 3],
+            }
+        );
+    }
+}

crates/packet/tests/properties.rs

@@ -36,13 +36,29 @@ use bytes::BytesMut;
 use proptest::collection::vec;
 use proptest::prelude::*;
 
-fn ctx(four_octet_asn: bool) -> ParserContext {
+/// What a session settled that the codec depends on.
-    ParserContext {
+#[derive(Debug, Clone, Copy)]
-        four_octet_asn: Some(four_octet_asn),
+struct Session {
-        address_family: None,
+    four_octet_asn: bool,
+    ibgp: bool,
+}
+
+impl Session {
+    fn ctx(self) -> ParserContext {
+        ParserContext {
+            four_octet_asn: Some(self.four_octet_asn),
+            ibgp: self.ibgp,
+        }
     }
 }
 
+fn session() -> impl Strategy<Value = Session> {
+    any::<(bool, bool)>().prop_map(|(four_octet_asn, ibgp)| Session {
+        four_octet_asn,
+        ibgp,
+    })
+}
+
 // ---------------------------------------------------------------------------
 // Generators
 // ---------------------------------------------------------------------------
@@ -76,7 +92,8 @@ fn asn(four_octet_asn: bool) -> BoxedStrategy<u32> {
 }
 
 fn as_path(asn: BoxedStrategy<u32>) -> impl Strategy<Value = AsPathAttribute> {
-    vec((any::<bool>(), vec(asn, 0..6)), 0..4).prop_map(|segments| AsPathAttribute {
+    // A segment has at least one AS number.
+    vec((any::<bool>(), vec(asn, 1..6)), 0..4).prop_map(|segments| AsPathAttribute {
         segments: segments
             .into_iter()
             .map(|(ordered, path)| AsPathSegment { ordered, path })
@@ -88,7 +105,7 @@ fn as_path(asn: BoxedStrategy<u32>) -> impl Strategy<Value = AsPathAttribute> {
 /// they are generated from the eight octets they are on the wire.
 fn extended_community() -> impl Strategy<Value = ExtendedCommunity> {
     any::<[u8; 8]>().prop_map(|bytes| {
-        ExtendedCommunity::from_wire(&ctx(true), &bytes)
+        ExtendedCommunity::from_wire(&ParserContext::default(), &bytes)
             .expect("any eight octets are an extended community")
             .1
     })
@@ -141,68 +158,99 @@ fn mp_unreach() -> impl Strategy<Value = MpUnreachNlriPathAttribute> {
     prop_oneof![of(AddressFamilyId::Ipv4), of(AddressFamilyId::Ipv6)]
 }
 
-/// Type codes that have their own variant; an unknown attribute has none of
+/// Type codes whose definition the decoder knows; an unknown attribute has
-/// these.
+/// none of these.
-const KNOWN_ATTRIBUTES: [u8; 13] = [1, 2, 3, 4, 5, 6, 7, 8, 14, 15, 16, 17, 32];
+const DEFINED_ATTRIBUTES: [u8; 16] = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 14, 15, 16, 17, 18, 32];
 
-fn unknown_attribute() -> impl Strategy<Value = PathAttribute> {
+/// Unknown attributes, each with a type code of its own: a repeated
-    (
+/// attribute is dropped by the decoder.
-        any::<u8>().prop_filter("a type code without a variant", |code| {
+fn unknown_attributes() -> impl Strategy<Value = Vec<PathAttribute>> {
-            !KNOWN_ATTRIBUTES.contains(code)
+    proptest::collection::btree_map(
+        any::<u8>().prop_filter("a type code without a definition", |code| {
+            !DEFINED_ATTRIBUTES.contains(code)
         }),
-        any::<u8>(),
+        (any::<u8>(), vec(any::<u8>(), 0..300)),
-        vec(any::<u8>(), 0..300),
+        0..3,
     )
-        .prop_map(|(type_code, flags, payload)| {
+    .prop_map(|attributes| {
-            // Bit 4 selects the two-octet length, which a payload this
+        attributes
-            // long needs.
+            .into_iter()
-            let flags = PathAttributeFlags(if payload.len() > 255 {
+            .map(|(type_code, (flags, payload))| {
-                flags | 0x10
+                // Optional (bit 7): an unknown well-known attribute is an
-            } else {
+                // error. Bit 4 selects the two-octet length, which a payload
-                flags
+                // this long needs.
-            });
+                let mut flags = flags | 0x80;
-            PathAttribute::UnknownPathAttribute {
+                if payload.len() > 255 {
-                flags,
+                    flags |= 0x10;
-                type_code,
+                }
-                payload,
+                PathAttribute::UnknownPathAttribute {
-            }
+                    flags: PathAttributeFlags(flags),
-        })
+                    type_code,
+                    payload,
+                }
+            })
+            .collect()
+    })
 }
 
-fn path_attribute(four_octet_asn: bool) -> impl Strategy<Value = PathAttribute> {
+/// The attributes of a well-formed UPDATE on `session`: the mandatory ones,
-    prop_oneof![
+/// and any of the others at most once.
+fn path_attributes(session: Session) -> impl Strategy<Value = Vec<PathAttribute>> {
+    let mandatory = (
         prop_oneof![
             Just(OriginPathAttribute::IGP),
             Just(OriginPathAttribute::EGP),
             Just(OriginPathAttribute::INCOMPLETE)
-        ]
+        ],
-        .prop_map(PathAttribute::Origin),
+        as_path(asn(session.four_octet_asn)),
-        as_path(asn(four_octet_asn)).prop_map(PathAttribute::AsPath),
+        any::<[u8; 4]>(),
-        any::<[u8; 4]>()
+        any::<u32>(),
-            .prop_map(|a| PathAttribute::NextHop(NextHopPathAttribute(Ipv4Addr::from(a)))),
+    )
-        any::<u32>().prop_map(|v| PathAttribute::MultiExitDisc(MultiExitDiscPathAttribute(v))),
+        .prop_map(move |(origin, as_path, next_hop, local_pref)| {
-        any::<u32>().prop_map(|v| PathAttribute::LocalPref(LocalPrefPathAttribute(v))),
+            let mut attributes = vec![
-        Just(PathAttribute::AtomicAggregate(
+                PathAttribute::Origin(origin),
-            AtomicAggregatePathAttribute {}
+                PathAttribute::AsPath(as_path),
+                PathAttribute::NextHop(NextHopPathAttribute(Ipv4Addr::from(next_hop))),
+            ];
+            // Mandatory between internal peers, dropped from external ones.
+            if session.ibgp {
+                attributes.push(PathAttribute::LocalPref(LocalPrefPathAttribute(local_pref)));
+            }
+            attributes
+        });
+    let optional = (
+        proptest::option::of(
+            any::<u32>().prop_map(|v| PathAttribute::MultiExitDisc(MultiExitDiscPathAttribute(v))),
+        ),
+        proptest::option::of(Just(PathAttribute::AtomicAggregate(
+            AtomicAggregatePathAttribute {},
+        ))),
+        proptest::option::of((asn(session.four_octet_asn), any::<[u8; 4]>()).prop_map(
+            |(asn, ip)| {
+                PathAttribute::Aggregator(AggregatorPathAttribute {
+                    asn,
+                    ip: Ipv4Addr::from(ip),
+                })
+            },
         )),
-        (asn(four_octet_asn), any::<[u8; 4]>()).prop_map(|(asn, ip)| {
+        proptest::option::of(
-            PathAttribute::Aggregator(AggregatorPathAttribute {
+            vec(any::<(u16, u16)>(), 1..8)
-                asn,
+                .prop_map(|c| PathAttribute::Communities(CommunitiesPathAttribute(c))),
-                ip: Ipv4Addr::from(ip),
+        ),
-            })
+        proptest::option::of(mp_reach().prop_map(PathAttribute::MpReachNlri)),
-        }),
+        proptest::option::of(mp_unreach().prop_map(PathAttribute::MpUnreachNlri)),
-        vec(any::<(u16, u16)>(), 0..8)
+        proptest::option::of(
-            .prop_map(|c| PathAttribute::Communities(CommunitiesPathAttribute(c))),
+            vec(extended_community(), 1..6).prop_map(|extended_communities| {
-        mp_reach().prop_map(PathAttribute::MpReachNlri),
+                PathAttribute::ExtendedCommunities(ExtendedCommunitiesPathAttribute {
-        mp_unreach().prop_map(PathAttribute::MpUnreachNlri),
+                    extended_communities,
-        vec(extended_community(), 1..6).prop_map(|extended_communities| {
+                })
-            PathAttribute::ExtendedCommunities(ExtendedCommunitiesPathAttribute {
+            }),
-                extended_communities,
+        ),
-            })
+        // AS4_PATH is four octets wide, and only a two-octet speaker sends
-        }),
+        // it.
-        // AS4_PATH is four octets wide on every session.
+        proptest::option::of(as_path(any::<u32>().boxed()).prop_map(PathAttribute::As4Path))
-        as_path(any::<u32>().boxed()).prop_map(PathAttribute::As4Path),
+            .prop_map(move |as4_path| as4_path.filter(|_| !session.four_octet_asn)),
-        vec(any::<(u32, u32, u32)>(), 1..6).prop_map(|communities| {
+        proptest::option::of(vec(any::<(u32, u32, u32)>(), 1..6).prop_map(|communities| {
             PathAttribute::LargeCommunities(LargeCommunitiesPathAttribute {
                 communities: communities
                     .into_iter()
@@ -213,15 +261,36 @@ fn path_attribute(four_octet_asn: bool) -> impl Strategy<Value = PathAttribute>
                     })
                     .collect(),
             })
-        }),
+        })),
-        unknown_attribute(),
+        unknown_attributes(),
-    ]
+    );
+    (mandatory, optional).prop_map(|(mut attributes, optional)| {
+        let (med, atomic, aggregator, communities, reach, unreach, extended, as4, large, unknown) =
+            optional;
+        attributes.extend(
+            [
+                med,
+                atomic,
+                aggregator,
+                communities,
+                reach,
+                unreach,
+                extended,
+                as4,
+                large,
+            ]
+            .into_iter()
+            .flatten(),
+        );
+        attributes.extend(unknown);
+        attributes
+    })
 }
 
-fn update(four_octet_asn: bool) -> impl Strategy<Value = UpdateMessage> {
+fn update(session: Session) -> impl Strategy<Value = UpdateMessage> {
     (
         vec(prefix(AddressFamilyId::Ipv4), 0..5),
-        vec(path_attribute(four_octet_asn), 0..8),
+        path_attributes(session),
         vec(prefix(AddressFamilyId::Ipv4), 0..5),
     )
         .prop_map(
@@ -360,10 +429,10 @@ fn notification() -> impl Strategy<Value = NotificationMessage> {
     )
 }
 
-fn message(four_octet_asn: bool) -> impl Strategy<Value = Message> {
+fn message(session: Session) -> impl Strategy<Value = Message> {
     prop_oneof![
         open().prop_map(Message::Open),
-        update(four_octet_asn).prop_map(Message::Update),
+        update(session).prop_map(Message::Update),
         notification().prop_map(Message::Notification),
         Just(Message::KeepAlive),
         (afi(), any::<u8>(), safi()).prop_map(|(afi, subtype, safi)| {
@@ -372,21 +441,25 @@ fn message(four_octet_asn: bool) -> impl Strategy<Value = Message> {
     ]
 }
 
-/// A session's AS number width, and a message that fits it.
+/// A session, and a well-formed message that fits it.
-fn session_and_message() -> impl Strategy<Value = (bool, Message)> {
+fn session_and_message() -> impl Strategy<Value = (Session, Message)> {
-    any::<bool>().prop_flat_map(|four_octet_asn| (Just(four_octet_asn), message(four_octet_asn)))
+    session().prop_flat_map(|session| (Just(session), message(session)))
 }
 
 // ---------------------------------------------------------------------------
 // Properties
 // ---------------------------------------------------------------------------
 
-/// What must hold for anything the decoder accepts: it can be shown,
+/// Decodes `wire` and checks what must hold for anything the decoder
-/// written, and read back as itself.
+/// accepts: it can be shown and written, and unless the decoder had to
-fn check_accepted(ctx: &ParserContext, msg: &Message) -> Result<(), TestCaseError> {
+/// repair it, it reads back as itself.
+fn check_decoding(ctx: &ParserContext, wire: &[u8]) -> Result<(), TestCaseError> {
+    let Ok((msg, repairs)) = Message::decode_with_report(ctx, wire) else {
+        return Ok(());
+    };
     // Display of what a peer sent must not be able to panic: it is logged.
-    let _ = format!("{msg:?}");
+    let _ = format!("{msg:?} {repairs:?}");
-    if let Message::Update(update) = msg {
+    if let Message::Update(update) = &msg {
         for prefix in update
             .withdrawn_routes
             .iter()
@@ -407,13 +480,25 @@ fn check_accepted(ctx: &ParserContext, msg: &Message) -> Result<(), TestCaseErro
         }
     }
 
-    let mut wire = BytesMut::new();
+    let mut rewritten = BytesMut::new();
-    msg.to_wire(ctx, &mut wire)
+    match msg.to_wire(ctx, &mut rewritten) {
-        .map_err(|e| TestCaseError::fail(format!("accepted but cannot be written: {e}")))?;
+        Ok(()) => {}
-    prop_assert_eq!(usize::from(msg.wire_len(ctx).unwrap()), wire.len());
+        // Withdrawing what a full-size UPDATE announced can need more room
-    let again = Message::decode(ctx, &wire)
+        // than one message has. The result is acted on, never sent.
-        .map_err(|e| TestCaseError::fail(format!("written but cannot be read: {e}")))?;
+        Err(EncodeError::TooLong { .. }) if !repairs.is_empty() => return Ok(()),
-    prop_assert_eq!(&again, msg);
+        Err(e) => {
+            return Err(TestCaseError::fail(format!(
+                "accepted but cannot be written: {e}"
+            )));
+        }
+    }
+    prop_assert_eq!(usize::from(msg.wire_len(ctx).unwrap()), rewritten.len());
+    if repairs.is_empty() {
+        let (again, repairs) = Message::decode_with_report(ctx, &rewritten)
+            .map_err(|e| TestCaseError::fail(format!("written but cannot be read: {e}")))?;
+        prop_assert!(repairs.is_empty(), "{:?}", repairs);
+        prop_assert_eq!(again, msg);
+    }
     Ok(())
 }
 
@@ -503,65 +588,59 @@ proptest! {
     #![proptest_config(config())]
 
     #[test]
-    fn a_message_that_can_be_written_reads_back_equal((four_octet_asn, msg) in session_and_message()) {
+    fn a_message_that_can_be_written_reads_back_equal((session, msg) in session_and_message()) {
-        let ctx = ctx(four_octet_asn);
+        let ctx = session.ctx();
         let Some(wire) = written(&ctx, &msg)? else {
             return Ok(());
         };
         prop_assert_eq!(usize::from(msg.wire_len(&ctx).unwrap()), wire.len());
 
-        let decoded = Message::decode(&ctx, &wire)
+        let (decoded, repairs) = Message::decode_with_report(&ctx, &wire)
             .map_err(|e| TestCaseError::fail(format!("{e}")))?;
+        // What we write is well formed: nothing for the decoder to repair.
+        prop_assert!(repairs.is_empty(), "{:?}", repairs);
         prop_assert_eq!(&decoded, &msg);
         let (rest, _) = Message::from_wire(&ctx, &wire).unwrap();
         prop_assert!(rest.is_empty());
     }
 
     #[test]
-    fn noise_is_never_more_than_an_error(four_octet_asn: bool, bytes in vec(any::<u8>(), 0..200)) {
+    fn noise_is_never_more_than_an_error(session in session(), bytes in vec(any::<u8>(), 0..200)) {
-        let ctx = ctx(four_octet_asn);
+        check_decoding(&session.ctx(), &bytes)?;
-        if let Ok(msg) = Message::decode(&ctx, &bytes) {
-            check_accepted(&ctx, &msg)?;
-        }
     }
 
     #[test]
     fn noise_behind_a_valid_header_is_never_more_than_an_error(
-        four_octet_asn: bool,
+        session in session(),
         msg_type in 0u8..7,
         body in vec(any::<u8>(), 0..300),
     ) {
-        let ctx = ctx(four_octet_asn);
+        check_decoding(&session.ctx(), &framed(msg_type, &body))?;
-        if let Ok(msg) = Message::decode(&ctx, &framed(msg_type, &body)) {
-            check_accepted(&ctx, &msg)?;
-        }
     }
 
     #[test]
     fn a_damaged_message_is_never_more_than_an_error(
-        (four_octet_asn, msg) in session_and_message(),
+        (session, msg) in session_and_message(),
         damage in vec(damage(), 1..4),
     ) {
-        let ctx = ctx(four_octet_asn);
+        let ctx = session.ctx();
         let Some(wire) = written(&ctx, &msg)? else {
             return Ok(());
         };
-        if let Ok(msg) = Message::decode(&ctx, &damaged(&wire, &damage)) {
+        check_decoding(&ctx, &damaged(&wire, &damage))?;
-            check_accepted(&ctx, &msg)?;
-        }
     }
 
     /// The parsers below the message level are public too.
     #[test]
     fn noise_is_never_more_than_an_error_for_the_parts(
-        four_octet_asn: bool,
+        session in session(),
         bytes in vec(any::<u8>(), 0..120),
     ) {
-        let ctx = ctx(four_octet_asn);
+        let ctx = session.ctx();
         let _ = OpenMessage::from_wire(&ctx, &bytes);
         let _ = OpenOption::from_wire(&ctx, &bytes);
         let _ = Capability::from_wire(&ctx, &bytes);
-        let _ = UpdateMessage::from_wire(&ctx, &bytes);
+        let _ = UpdateMessage::decode(&ctx, &bytes);
         let _ = PathAttribute::from_wire(&ctx, &bytes);
         let _ = NotificationMessage::from_wire(&ctx, &bytes);
     }

crates/primitives/src/parser.rs

@@ -1,14 +1,16 @@
 use nom::error::{ErrorKind, ParseError};
 use thiserror::Error;
 
-use crate::constants::AddressFamilyId;
+/// What a parser has to know about the session a message belongs to.
-
 #[derive(Debug, Default)]
 pub struct ParserContext {
-    /// Whether thi parser is being run with a peer that is RFC6793 compliant.
+    /// Whether AS numbers are four octets wide on this session (RFC 6793).
+    /// `None` until the OPENs have been exchanged.
     pub four_octet_asn: Option<bool>,
-    /// Which address family should be parsed by default with this parser.
+    /// Whether the peer is in our own AS. Some attributes mean something
-    pub address_family: Option<AddressFamilyId>,
+    /// only between internal peers and are dropped when an external one
+    /// sends them (RFC 7606 §7).
+    pub ibgp: bool,
 }
 
 /// Why a value could not be written to the wire.

crates/server/src/advertise.rs

@@ -290,7 +290,7 @@ mod tests {
         // Serialise and parse back to make sure the wire encoding is sound.
         let ctx = ParserContext {
             four_octet_asn: Some(true),
-            address_family: None,
+            ibgp: false,
         };
         let msg = Message::Update(update.clone());
         let mut buf = BytesMut::new();
@@ -348,7 +348,7 @@ mod tests {
         // What the peer reads from the wire.
         let ctx = ParserContext {
             four_octet_asn: Some(false),
-            address_family: None,
+            ibgp: false,
         };
         let mut buf = BytesMut::new();
         Message::Update(update).to_wire(&ctx, &mut buf).unwrap();

crates/server/src/connection.rs

@@ -14,6 +14,7 @@ use std::sync::Arc;
 
 use bgp_packet::errors::{BgpError, DecodeError, MsgHeaderSubcode};
 use bgp_packet::message::{BGP_HEADER_LEN, BGP_MAX_MSG_LEN, Message, PathAttribute, UpdateMessage};
+use bgp_packet::update_decode::UpdateError;
 use bgp_primitives::parser::ParserContext;
 use bgp_rib::{AdjRibIn, PeerId, ReceivedUpdate};
 use bytes::BytesMut;
@@ -29,7 +30,7 @@ use crate::error::SessionError;
 use crate::family::FamilySet;
 use crate::fsm::{Action, Event, Fsm, FsmConfig, Negotiated};
 use crate::metrics::{
-    MESSAGES_RECEIVED, MESSAGES_SENT, SESSIONS_ESTABLISHED_TOTAL, message_type_name,
+    MESSAGES_RECEIVED, MESSAGES_SENT, SESSIONS_ESTABLISHED_TOTAL, UPDATE_ERRORS, message_type_name,
 };
 use crate::session::{SessionGuard, SessionHandle, SessionRegistry};
 
@@ -157,7 +158,10 @@ impl<R: AsyncRead + Unpin> Session<R> {
     async fn next_event(&mut self) -> Result<Event, SessionError> {
         tokio::select! {
             frame = self.frames.next(&self.ctx) => match frame {
-                Ok(msg) => Ok(Event::Message(msg)),
+                Ok((msg, update_errors)) => {
+                    self.report(&update_errors);
+                    Ok(Event::Message(msg))
+                }
                 Err(SessionError::Decode(e)) => Ok(Event::DecodeFailed(e)),
                 Err(e) => Err(e),
             },
@@ -168,6 +172,24 @@ impl<R: AsyncRead + Unpin> Session<R> {
         }
     }
 
+    /// Records errors in an UPDATE that the session survived. They are the
+    /// peer's fault, or that of a router behind it, and worth an operator's
+    /// attention: routes were withdrawn or attributes dropped because of
+    /// them.
+    fn report(&self, update_errors: &[UpdateError]) {
+        for error in update_errors {
+            UPDATE_ERRORS
+                .with_label_values(&[error.handling.as_str()])
+                .inc();
+            warn!(
+                peer_addr = %self.peer_addr,
+                handling = error.handling.as_str(),
+                detail = %error.detail,
+                "Error in received UPDATE"
+            );
+        }
+    }
+
     /// Carries out `actions` in order. Returns the session's result once it
     /// is over.
     async fn execute(&mut self, actions: Vec<Action>) -> Option<Result<(), SessionError>> {
@@ -197,6 +219,7 @@ impl<R: AsyncRead + Unpin> Session<R> {
                 }
                 Action::Admit(negotiated) => {
                     self.ctx.four_octet_asn = Some(negotiated.four_octet_asn);
+                    self.ctx.ibgp = negotiated.peer_asn == self.config.local_asn;
                     match self.config.collisions.admit(
                         self.config.router_id,
                         negotiated.peer_identifier,
@@ -369,17 +392,24 @@ impl<R: AsyncRead + Unpin> FrameReader<R> {
     }
 
     /// The next message from the peer. Cancel-safe.
-    async fn next(&mut self, ctx: &ParserContext) -> Result<Message, SessionError> {
+    ///
+    /// With an UPDATE come the errors in it that were handled without
+    /// resetting the session (RFC 7606); the UPDATE returned is the one to
+    /// act on, with their handling already applied.
+    async fn next(
+        &mut self,
+        ctx: &ParserContext,
+    ) -> Result<(Message, Vec<UpdateError>), SessionError> {
         loop {
             if let Some(length) = frame_length(&self.buf)?
                 && self.buf.len() >= length
             {
                 let frame = self.buf.split_to(length);
-                let msg = Message::decode(ctx, &frame)?;
+                let (msg, update_errors) = Message::decode_with_report(ctx, &frame)?;
                 MESSAGES_RECEIVED
                     .with_label_values(&[message_type_name(&msg)])
                     .inc();
-                return Ok(msg);
+                return Ok((msg, update_errors));
             }
             if self.reader.read_buf(&mut self.buf).await? == 0 {
                 return Err(io::Error::new(
@@ -456,10 +486,19 @@ mod tests {
             bytes.extend_from_slice(&KEEPALIVE);
         }
         peer.write_all(&bytes[..47]).await.unwrap();
-        assert!(matches!(frames.next(&ctx).await, Ok(Message::KeepAlive)));
+        assert!(matches!(
-        assert!(matches!(frames.next(&ctx).await, Ok(Message::KeepAlive)));
+            frames.next(&ctx).await,
+            Ok((Message::KeepAlive, _))
+        ));
+        assert!(matches!(
+            frames.next(&ctx).await,
+            Ok((Message::KeepAlive, _))
+        ));
         peer.write_all(&bytes[47..]).await.unwrap();
-        assert!(matches!(frames.next(&ctx).await, Ok(Message::KeepAlive)));
+        assert!(matches!(
+            frames.next(&ctx).await,
+            Ok((Message::KeepAlive, _))
+        ));
 
         drop(peer);
         assert!(matches!(
@@ -483,7 +522,10 @@ mod tests {
         assert!(abandoned.is_err(), "half a message is not a message");
 
         peer.write_all(&KEEPALIVE[10..]).await.unwrap();
-        assert!(matches!(frames.next(&ctx).await, Ok(Message::KeepAlive)));
+        assert!(matches!(
+            frames.next(&ctx).await,
+            Ok((Message::KeepAlive, _))
+        ));
     }
 
     proptest! {
@@ -531,7 +573,7 @@ mod tests {
                 };
                 let reader = async {
                     let mut received = Vec::new();
-                    while let Ok(msg) = frames.next(&ctx).await {
+                    while let Ok((msg, _)) = frames.next(&ctx).await {
                         received.push(msg);
                     }
                     received

crates/server/src/metrics.rs

@@ -26,6 +26,15 @@ pub static MESSAGES_RECEIVED: LazyLock<IntCounterVec> = LazyLock::new(|| {
     .expect("metric registration")
 });
 
+pub static UPDATE_ERRORS: LazyLock<IntCounterVec> = LazyLock::new(|| {
+    register_int_counter_vec!(
+        "bgp_update_errors_total",
+        "Errors in received UPDATE messages that did not reset the session, by how they were handled (RFC 7606)",
+        &["handling"]
+    )
+    .expect("metric registration")
+});
+
 pub static SESSIONS_ESTABLISHED_TOTAL: LazyLock<IntCounter> = LazyLock::new(|| {
     register_int_counter!(
         "bgp_sessions_established_total",
@@ -69,6 +78,7 @@ pub static ADJ_RIB_IN_ROUTES: LazyLock<IntGauge> = LazyLock::new(|| {
 pub fn init() {
     LazyLock::force(&MESSAGES_SENT);
     LazyLock::force(&MESSAGES_RECEIVED);
+    LazyLock::force(&UPDATE_ERRORS);
     LazyLock::force(&SESSIONS_ESTABLISHED_TOTAL);
     LazyLock::force(&SESSIONS_ESTABLISHED);
     LazyLock::force(&COLLISIONS);

fuzz/fuzz_targets/message.rs

@@ -1,5 +1,5 @@
 //! Raw bytes, as they come off a TCP connection. The first byte picks the
-//! session's AS number width.
+//! session's AS number width and whether the peer is internal.
 #![no_main]
 
 use libfuzzer_sys::fuzz_target;
@@ -8,5 +8,5 @@ fuzz_target!(|data: &[u8]| {
     let Some((selector, wire)) = data.split_first() else {
         return;
     };
-    bgp_fuzz::decode_and_check(selector & 1 == 1, wire);
+    bgp_fuzz::decode_and_check(selector & 1 == 1, selector & 0x80 != 0, wire);
 });

fuzz/fuzz_targets/message_body.rs

@@ -1,7 +1,7 @@
 //! A message body behind a correct header, so that the fuzzer spends its
 //! time in the OPEN, UPDATE and NOTIFICATION parsers and not on guessing
-//! the marker and length. The first byte picks the AS number width and the
+//! the marker and length. The first byte picks the AS number width, the
-//! message type.
+//! message type and whether the peer is internal.
 #![no_main]
 
 use libfuzzer_sys::fuzz_target;
@@ -20,5 +20,5 @@ fuzz_target!(|data: &[u8]| {
     wire.extend_from_slice(&((19 + body.len()) as u16).to_be_bytes());
     wire.push(msg_type);
     wire.extend_from_slice(body);
-    bgp_fuzz::decode_and_check(selector & 1 == 1, &wire);
+    bgp_fuzz::decode_and_check(selector & 1 == 1, selector & 0x80 != 0, &wire);
 });

fuzz/src/lib.rs

@@ -1,33 +1,44 @@
 //! What the fuzz targets have in common.
 
 use bgp_packet::message::Message;
-use bgp_primitives::parser::ParserContext;
+use bgp_primitives::parser::{EncodeError, ParserContext};
 use bytes::BytesMut;
 
 /// Decodes `wire` as a peer's message on a session with the given AS number
-/// width. Whatever the bytes, this must return: a panic is a way for a peer
+/// width and kind of peer. Whatever the bytes, this must return: a panic is a way for a peer
 /// to take a session down.
 ///
 /// A message that is accepted must also be one we can show, write, and read
 /// back as itself. Anything else means the decoder built a value the rest
 /// of the program cannot rely on.
-pub fn decode_and_check(four_octet_asn: bool, wire: &[u8]) {
+pub fn decode_and_check(four_octet_asn: bool, ibgp: bool, wire: &[u8]) {
     let ctx = ParserContext {
         four_octet_asn: Some(four_octet_asn),
-        address_family: None,
+        ibgp,
     };
-    let Ok(msg) = Message::decode(&ctx, wire) else {
+    let Ok((msg, repairs)) = Message::decode_with_report(&ctx, wire) else {
         return;
     };
-    let _ = format!("{msg:?}");
+    let _ = format!("{msg:?} {repairs:?}");
 
     let mut rewritten = BytesMut::new();
-    msg.to_wire(&ctx, &mut rewritten)
+    match msg.to_wire(&ctx, &mut rewritten) {
-        .expect("an accepted message can be written");
+        Ok(()) => {}
+        // Withdrawing what a full-size UPDATE announced can need more room
+        // than one message has. The result is acted on, never sent.
+        Err(EncodeError::TooLong { .. }) if !repairs.is_empty() => return,
+        Err(e) => panic!("an accepted message cannot be written: {e}"),
+    }
     assert_eq!(
         usize::from(msg.wire_len(&ctx).expect("and has a length")),
         rewritten.len()
     );
-    let again = Message::decode(&ctx, &rewritten).expect("and read back");
+    // An UPDATE the decoder had to repair (RFC 7606) is not what was on the
-    assert_eq!(again, msg);
+    // wire, and need not read back as itself.
+    if repairs.is_empty() {
+        let (again, repairs) =
+            Message::decode_with_report(&ctx, &rewritten).expect("and read back");
+        assert!(repairs.is_empty(), "{repairs:?}");
+        assert_eq!(again, msg);
+    }
 }