lib: improve connection ID retirement

This change improves sender side handling of RETIRE_CONNECTION_ID
frames, especially when there may be multiple paths available to use.

Author: LPardue

quiche/src/cid.rs

@@ -71,10 +71,6 @@ impl BoundedConnectionIdSeqSet {
         self.inner.remove(e)
     }
 
-    fn front(&self) -> Option<u64> {
-        self.inner.iter().next().copied()
-    }
-
     fn is_empty(&self) -> bool {
         self.inner.is_empty()
     }
@@ -757,14 +753,15 @@ impl ConnectionIdentifiers {
         self.advertise_new_scid_seqs.front().copied()
     }
 
-    /// Gets a destination Connection IDs's sequence number that need to send
-    /// RETIRE_CONNECTION_ID frames.
+    /// Returns a copy of the set of destination Connection IDs's sequence
+    /// numbers to send RETIRE_CONNECTION_ID frames.
     ///
-    /// If `Some`, it always returns the same value until it has been removed
-    /// using `mark_retire_dcid_seq`.
+    /// Note that the set includes sequence numbers at the time the copy was
+    /// created. To account for newly inserted or removed sequence numbers, a
+    /// new copy needs to be created.
     #[inline]
-    pub fn next_retire_dcid_seq(&self) -> Option<u64> {
-        self.retire_dcid_seqs.front()
+    pub fn retire_dcid_seqs(&self) -> HashSet<u64> {
+        self.retire_dcid_seqs.inner.clone()
     }
 
     /// Returns true if there are new source Connection IDs to advertise.
@@ -927,12 +924,12 @@ mod tests {
         assert_eq!(ids.available_dcids(), 1);
         assert_eq!(ids.dcids.len(), 2);
         assert!(ids.has_retire_dcids());
-        assert_eq!(ids.next_retire_dcid_seq(), Some(0));
+        assert_eq!(ids.retire_dcid_seqs().iter().next(), Some(&0));
 
         // Fake RETIRE_CONNECTION_ID sending.
         let _ = ids.mark_retire_dcid_seq(0, false);
         assert!(!ids.has_retire_dcids());
-        assert_eq!(ids.next_retire_dcid_seq(), None);
+        assert_eq!(ids.retire_dcid_seqs().iter().next(), None);
 
         // Now tries to experience CID retirement. If the server tries to remove
         // non-existing DCIDs, it fails.
@@ -947,13 +944,13 @@ mod tests {
         ids.link_dcid_to_path_id(2, 0).unwrap();
         assert_eq!(ids.available_dcids(), 0);
         assert!(ids.has_retire_dcids());
-        assert_eq!(ids.next_retire_dcid_seq(), Some(1));
+        assert_eq!(ids.retire_dcid_seqs().iter().next(), Some(&1));
         assert_eq!(ids.dcids.len(), 1);
 
         // Fake RETIRE_CONNECTION_ID sending.
         let _ = ids.mark_retire_dcid_seq(1, false);
         assert!(!ids.has_retire_dcids());
-        assert_eq!(ids.next_retire_dcid_seq(), None);
+        assert_eq!(ids.retire_dcid_seqs().iter().next(), None);
 
         // Trying to remove the last DCID triggers an error.
         assert_eq!(ids.retire_dcid(2), Err(Error::OutOfIdentifiers));

quiche/src/lib.rs

@@ -4564,7 +4564,9 @@ impl<F: BufFactory> Connection<F> {
             }
 
             // Create RETIRE_CONNECTION_ID frames as needed.
-            while let Some(seq_num) = self.ids.next_retire_dcid_seq() {
+            let retire_dcid_seqs = self.ids.retire_dcid_seqs();
+
+            for seq_num in retire_dcid_seqs {
                 // The sequence number specified in a RETIRE_CONNECTION_ID frame
                 // MUST NOT refer to the Destination Connection ID field of the
                 // packet in which the frame is contained.

quiche/src/tests.rs

@@ -7955,6 +7955,90 @@ fn connection_id_retire_limit(
     );
 }
 
+#[rstest]
+fn connection_id_retire_exotic_sequence(
+    #[values("cubic", "bbr2", "bbr2_gcongestion")] cc_algorithm_name: &str,
+) {
+    let mut buf = [0; 65535];
+
+    let mut config = Config::new(PROTOCOL_VERSION).unwrap();
+    assert_eq!(config.set_cc_algorithm_name(cc_algorithm_name), Ok(()));
+    config
+        .load_cert_chain_from_pem_file("examples/cert.crt")
+        .unwrap();
+    config
+        .load_priv_key_from_pem_file("examples/cert.key")
+        .unwrap();
+    config
+        .set_application_protos(&[b"proto1", b"proto2"])
+        .unwrap();
+    config.verify_peer(false);
+    config.set_active_connection_id_limit(2);
+    config.set_initial_max_data(30);
+    config.set_initial_max_stream_data_bidi_local(15);
+    config.set_initial_max_stream_data_bidi_remote(15);
+    config.set_initial_max_stream_data_uni(10);
+    config.set_initial_max_streams_uni(3);
+    config.set_initial_max_streams_bidi(3);
+
+    let mut pipe = test_utils::Pipe::with_config(&mut config).unwrap();
+    assert_eq!(pipe.handshake(), Ok(()));
+
+    // Inject an exotic sequence of NEW_CONNECTION_ID frames, unbeknowst to
+    // quiche client connection object.
+    let frames = [
+        frame::Frame::NewConnectionId {
+            seq_num: 8,
+            retire_prior_to: 1,
+            conn_id: vec![0],
+            reset_token: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1],
+        },
+        frame::Frame::NewConnectionId {
+            seq_num: 1,
+            retire_prior_to: 0,
+            conn_id: vec![02],
+            reset_token: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2],
+        },
+        frame::Frame::NewConnectionId {
+            seq_num: 6,
+            retire_prior_to: 6,
+            conn_id: vec![0x15],
+            reset_token: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3],
+        },
+        frame::Frame::NewConnectionId {
+            seq_num: 8,
+            retire_prior_to: 1,
+            conn_id: vec![0],
+            reset_token: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4],
+        },
+        frame::Frame::NewConnectionId {
+            seq_num: 48,
+            retire_prior_to: 8,
+            conn_id: vec![1],
+            reset_token: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 5],
+        },
+    ];
+
+    let pkt_type = Type::Short;
+    pipe.send_pkt_to_server(pkt_type, &frames, &mut buf)
+        .unwrap();
+
+    // Ensure operations continue to be allowed.
+    assert_eq!(pipe.client.stream_send(0, b"data", true), Ok(4));
+    assert_eq!(pipe.server.stream_send(1, b"data", true), Ok(4));
+    assert_eq!(pipe.client.stream_send(2, b"data", true), Ok(4));
+    assert_eq!(pipe.server.stream_send(3, b"data", true), Ok(4));
+
+    assert_eq!(pipe.advance(), Ok(()));
+
+    let mut b = [0; 15];
+    assert_eq!(pipe.server.stream_recv(0, &mut b), Ok((4, true)));
+    assert_eq!(pipe.server.stream_recv(2, &mut b), Ok((4, true)));
+
+    // The exotic sequence insertion messes with the client object's
+    // worldview so we can't check its side of things.
+}
+
 // Utility function.
 fn pipe_with_exchanged_cids(
     config: &mut Config, client_scid_len: usize, server_scid_len: usize,