fix: Accept or reject incoming frames based on self.local_max_path_id instead of Connection::max_path_id

quinn-proto/src/connection/mod.rs

@@ -4163,20 +4163,20 @@ impl Connection {
                     }
                 }
                 Frame::NewConnectionId(frame) => {
-                    let path_id = match (frame.path_id, self.max_path_id()) {
-                        (Some(path_id), Some(current_max)) if path_id <= current_max => path_id,
-                        (Some(_large_path_id), Some(_current_max)) => {
+                    let path_id = if let Some(path_id) = frame.path_id {
+                        if !self.is_multipath_negotiated() {
                             return Err(TransportError::PROTOCOL_VIOLATION(
-                                "PATH_NEW_CONNECTION_ID contains path_id exceeding current max",
+                                "received PATH_NEW_CONNECTION_ID frame when multipath was not negotiated",
                             ));
                         }
-                        (Some(_path_id), None) => {
+                        if path_id > self.local_max_path_id {
                             return Err(TransportError::PROTOCOL_VIOLATION(
-                                "received PATH_NEW_CONNECTION_ID frame when multipath was not negotiated",
+                                "PATH_NEW_CONNECTION_ID contains path_id exceeding current max",
                             ));
                         }
-
-                        (None, _) => PathId::ZERO,
+                        path_id
+                    } else {
+                        PathId::ZERO
                     };
 
                     if self.abandoned_paths.contains(&path_id) {
@@ -4422,17 +4422,16 @@ impl Connection {
                 }
                 Frame::MaxPathId(frame::MaxPathId(path_id)) => {
                     span.record("path", tracing::field::debug(&path_id));
-                    if let Some(current_max) = self.max_path_id() {
-                        // frames that do not increase the path id are ignored
-                        self.remote_max_path_id = self.remote_max_path_id.max(path_id);
-                        if self.max_path_id() != Some(current_max) {
-                            self.issue_first_path_cids(now);
-                        }
-                    } else {
+                    if !self.is_multipath_negotiated() {
                         return Err(TransportError::PROTOCOL_VIOLATION(
                             "received MAX_PATH_ID frame when multipath was not negotiated",
                         ));
                     }
+                    // frames that do not increase the path id are ignored
+                    if path_id > self.remote_max_path_id {
+                        self.remote_max_path_id = path_id;
+                        self.issue_first_path_cids(now);
+                    }
                 }
                 Frame::PathsBlocked(frame::PathsBlocked(max_path_id)) => {
                     // Receipt of a value of Maximum Path Identifier or Path Identifier that is higher than the local maximum value MUST
@@ -5911,10 +5910,14 @@ impl Connection {
         );
     }
 
-    /// Returns the maximum [`PathId`] to be used in this connection.
+    /// Returns the maximum [`PathId`] to be used for sending in this connection.
     ///
     /// This is calculated as minimum between the local and remote's maximums when multipath is
     /// enabled, or `None` when disabled.
+    ///
+    /// For data that's received, we should use [`self.local_max_path_id`] instead.
+    /// The reasoning is that the remote might already have updated to its own newer
+    /// [`Self::max_path_id`] after sending out a `MAX_PATH_ID` frame, but it got re-ordered.
     fn max_path_id(&self) -> Option<PathId> {
         if self.is_multipath_negotiated() {
             Some(self.remote_max_path_id.min(self.local_max_path_id))

quinn-proto/src/tests/multipath.rs

@@ -363,6 +363,78 @@ fn issue_max_path_id() {
     assert_eq!(stats.frame_rx.path_new_connection_id, client_path_new_cids);
 }
 
+/// A copy of [`issue_max_path_id`], but reordering the `MAX_PATH_ID` frame
+/// that's sent from the server to the client, so that some `NEW_CONNECTION_ID`
+/// frames arrive with higher path IDs than the most recently received
+/// `MAX_PATH_ID` frame on the client side.
+#[test]
+fn issue_max_path_id_reordered() {
+    let _guard = subscribe();
+
+    // We enable multipath but initially do not allow any paths to be opened.
+    let multipath_transport_cfg = Arc::new(TransportConfig {
+        max_concurrent_multipath_paths: NonZeroU32::new(1),
+        ..TransportConfig::default()
+    });
+    let server_cfg = Arc::new(ServerConfig {
+        transport: multipath_transport_cfg.clone(),
+        ..server_config()
+    });
+    let server = Endpoint::new(Default::default(), Some(server_cfg), true, None);
+    let client = Endpoint::new(Default::default(), None, true, None);
+
+    let mut pair = Pair::new_from_endpoint(client, server);
+
+    // The client is allowed to create more paths immediately.
+    let client_multipath_transport_cfg = Arc::new(TransportConfig {
+        max_concurrent_multipath_paths: NonZeroU32::new(MAX_PATHS),
+        ..TransportConfig::default()
+    });
+    let client_cfg = ClientConfig {
+        transport: client_multipath_transport_cfg,
+        ..client_config()
+    };
+    let (_client_ch, server_ch) = pair.connect_with(client_cfg);
+    pair.drive();
+    info!("connected");
+
+    // Server should only have sent NEW_CONNECTION_ID frames for now.
+    let server_new_cids = CidQueue::LEN as u64 - 1;
+    let mut server_path_new_cids = 0;
+    let stats = pair.server_conn_mut(server_ch).stats();
+    assert_eq!(stats.frame_tx.max_path_id, 0);
+    assert_eq!(stats.frame_tx.new_connection_id, server_new_cids);
+    assert_eq!(stats.frame_tx.path_new_connection_id, server_path_new_cids);
+
+    // Client should have sent PATH_NEW_CONNECTION_ID frames for PathId::ZERO.
+    let client_new_cids = 0;
+    let mut client_path_new_cids = CidQueue::LEN as u64;
+    assert_eq!(stats.frame_rx.new_connection_id, client_new_cids);
+    assert_eq!(stats.frame_rx.path_new_connection_id, client_path_new_cids);
+
+    // Server increases MAX_PATH_ID, but we reorder the frame
+    pair.server_conn_mut(server_ch)
+        .set_max_concurrent_paths(Instant::now(), NonZeroU32::new(MAX_PATHS).unwrap())
+        .unwrap();
+    pair.drive_server();
+    // reorder the frames on the incoming side
+    let p = pair.client.inbound.pop_front().unwrap();
+    pair.client.inbound.push_back(p);
+    pair.drive();
+    let stats = pair.server_conn_mut(server_ch).stats();
+
+    // Server should have sent MAX_PATH_ID and new CIDs
+    server_path_new_cids += (MAX_PATHS as u64 - 1) * CidQueue::LEN as u64;
+    assert_eq!(stats.frame_tx.max_path_id, 1);
+    assert_eq!(stats.frame_tx.new_connection_id, server_new_cids);
+    assert_eq!(stats.frame_tx.path_new_connection_id, server_path_new_cids);
+
+    // Client should have sent CIDs for new paths
+    client_path_new_cids += (MAX_PATHS as u64 - 1) * CidQueue::LEN as u64;
+    assert_eq!(stats.frame_rx.new_connection_id, client_new_cids);
+    assert_eq!(stats.frame_rx.path_new_connection_id, client_path_new_cids);
+}
+
 #[test]
 fn open_path() {
     let _guard = subscribe();