From 613c41cc813363cc2e81a50ef5850dbef54334a3 Mon Sep 17 00:00:00 2001 From: Quentin De Coninck Date: Thu, 29 Feb 2024 16:54:14 +0100 Subject: [PATCH] fully integrated explicit path ID --- apps/src/bin/quiche-server.rs | 20 ++- apps/src/client.rs | 17 ++- quiche/src/cid.rs | 238 +++++++++++++++++++++++------- quiche/src/lib.rs | 265 +++++++++++++++++++--------------- quiche/src/packet.rs | 8 +- quiche/src/path.rs | 75 ++++++---- 6 files changed, 417 insertions(+), 206 deletions(-) diff --git a/apps/src/bin/quiche-server.rs b/apps/src/bin/quiche-server.rs index c96f58ef2c..9ebbdd2732 100644 --- a/apps/src/bin/quiche-server.rs +++ b/apps/src/bin/quiche-server.rs @@ -512,14 +512,20 @@ fn main() { clients_ids.remove(&retired_scid); } - // Provides as many CIDs as possible. - while client.conn.scids_left() > 0 { - let (scid, reset_token) = generate_cid_and_reset_token(&rng); - if client.conn.new_scid(&scid, reset_token, false).is_err() { - break; - } + for path_id in client.conn.path_ids() { + // Provides as many CIDs as possible. + while client.conn.scids_left_on_path(path_id) > 0 { + let (scid, reset_token) = generate_cid_and_reset_token(&rng); + if client + .conn + .new_scid_on_path(path_id, &scid, reset_token, false) + .is_err() + { + break; + } - clients_ids.insert(scid, client.client_id); + clients_ids.insert(scid, client.client_id); + } } } diff --git a/apps/src/client.rs b/apps/src/client.rs index 7c5c6b2eab..afdaed5a4f 100644 --- a/apps/src/client.rs +++ b/apps/src/client.rs @@ -472,14 +472,19 @@ pub fn connect( } // Provides as many CIDs as possible. - while conn.scids_left() > 0 { - let (scid, reset_token) = generate_cid_and_reset_token(&rng); + for path_id in conn.path_ids() { + while conn.scids_left_on_path(path_id) > 0 { + let (scid, reset_token) = generate_cid_and_reset_token(&rng); - if conn.new_scid(&scid, reset_token, false).is_err() { - break; - } + if conn + .new_scid_on_path(path_id, &scid, reset_token, false) + .is_err() + { + break; + } - scid_sent = true; + scid_sent = true; + } } if conn_args.initial_max_paths.is_some() && diff --git a/quiche/src/cid.rs b/quiche/src/cid.rs index 5057bfa579..4809af160a 100644 --- a/quiche/src/cid.rs +++ b/quiche/src/cid.rs @@ -24,6 +24,8 @@ // NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS // SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +use smallvec::SmallVec; + use crate::CIDSeq; use crate::Error; use crate::FourTuple; @@ -162,8 +164,12 @@ impl BoundedNonEmptyVecDeque { /// `None` if it was not and nothing was modified. /// /// [`OutOfIdentifiers`]: enum.Error.html#OutOfIdentifiers - fn remove(&mut self, id: u64) -> Result> { + fn remove(&mut self, id: u64, allow_empty: bool) -> Result> { if self.inner.len() <= 1 { + // If we are closing the path, delay the removal of this ID. + if allow_empty { + return Ok(None); + } return Err(Error::OutOfIdentifiers); } @@ -236,6 +242,9 @@ pub struct PathConnectionIdentifiers { /// The maximum number of source Connection IDs our peer allows us. source_conn_id_limit: usize, + /// Are we in closing phase of the Path ID? + closing: bool, + /// Has initialized destination Connection ID? initialized_destination_connection_id: bool, @@ -294,24 +303,24 @@ impl PathConnectionIdentifiers { initialized_source_connection_id ); - let reset_token = initial_scid.as_ref().and_then(|_| reset_token); + let reset_token = initial_scid.as_ref().and(reset_token); // We need to track up to (2 * source_conn_id_limit - 1) source // Connection IDs when the host wants to force their renewal. let scids = BoundedNonEmptyVecDeque::new( 2 * source_conn_id_limit - 1, ConnectionIdEntry { - cid: initial_scid.unwrap_or(ConnectionId::default()), + cid: initial_scid.unwrap_or_default(), seq: 0, reset_token, network_path, }, ); - let reset_token = initial_dcid.as_ref().and_then(|_| reset_token); + let reset_token = initial_dcid.as_ref().and(reset_token); let dcids = BoundedNonEmptyVecDeque::new( destination_conn_id_limit, ConnectionIdEntry { - cid: initial_dcid.unwrap_or(ConnectionId::default()), + cid: initial_dcid.unwrap_or_default(), seq: 0, reset_token, network_path, @@ -387,6 +396,11 @@ impl PathConnectionIdentifiers { &mut self, cid: ConnectionId<'static>, reset_token: Option, network_path: Option, retire_if_needed: bool, ) -> Result<(bool, u64)> { + if !self.initialized_source_connection_id { + self.set_initial_scid(cid, reset_token, network_path); + return Ok((true, 0)); + } + // Check whether the number of source Connection IDs does not exceed the // limit. If the host agrees to retire old CIDs, it can store up to // (2 * source_active_conn_id - 1) source CIDs. This limit is enforced @@ -444,11 +458,6 @@ impl PathConnectionIdentifiers { .ok_or(Error::InvalidState) } - #[inline] - fn initialized_destination_connection_id(&self) -> bool { - self.initialized_destination_connection_id - } - /// Sets the initial destination identifier. fn set_initial_dcid( &mut self, cid: ConnectionId<'static>, seq: CIDSeq, @@ -584,7 +593,7 @@ impl PathConnectionIdentifiers { return Err(Error::InvalidState); } - let entry = if let Some(e) = self.scids.remove(seq)? { + let entry = if let Some(e) = self.scids.remove(seq, self.closing)? { if e.cid == *pkt_dcid { return Err(Error::InvalidState); } @@ -615,9 +624,11 @@ impl PathConnectionIdentifiers { /// [`OutOfIdentifiers`]: enum.Error.html#OutOfIdentifiers /// [`InvalidState`]: enum.Error.html#InvalidState pub fn retire_dcid(&mut self, seq: CIDSeq) -> Result> { - let e = self.dcids.remove(seq)?.ok_or(Error::InvalidState)?; - - Ok(e.network_path) + match self.dcids.remove(seq, self.closing)? { + Some(e) => Ok(e.network_path), + None if self.closing => Ok(None), + None => Err(Error::InvalidState), + } } /// Gets the lowest Destination Connection ID sequence number that is not @@ -643,6 +654,43 @@ impl Identifiable for PathConnectionIdentifiers { } } +/// An iterator over QUIC streams. +#[derive(Default)] +pub struct PathIdIter { + path_ids: SmallVec<[u64; 8]>, + index: usize, +} + +impl PathIdIter { + #[inline] + fn from(max_path_id: PathId, exclude: &[PathId]) -> Self { + PathIdIter { + path_ids: (0..=max_path_id) + .filter(|p| p % 2 == 0 && !exclude.contains(p)) + .collect(), + index: 0, + } + } +} + +impl Iterator for PathIdIter { + type Item = u64; + + #[inline] + fn next(&mut self) -> Option { + let v = self.path_ids.get(self.index)?; + self.index += 1; + Some(*v) + } +} + +impl ExactSizeIterator for PathIdIter { + #[inline] + fn len(&self) -> usize { + self.path_ids.len() - self.index + } +} + #[derive(Default)] pub struct ConnectionIdentifiers { /// All the Destination Connection IDs provided by our peer. @@ -665,17 +713,23 @@ pub struct ConnectionIdentifiers { destination_conn_id_limit: usize, /// Closed path Ids. - closed_path_id: VecDeque, + closed_path_id: Vec, /// Available path IDs to use -- for client use only!!! spare_path_ids: BinaryHeap>, - /// Maximum path ID that can be used on this connection. - max_path_id: PathId, + /// Maximum path ID that we allow on this connection. + local_max_path_id: PathId, + + /// Maximum path ID that our peer allows on this connection. + remote_max_path_id: PathId, /// Largest path ID seen on this connection. largest_path_id: PathId, + /// Whether we should advertise the local max path id. + advertise_local_max_path_id: bool, + /// Does the host use zero-length source Connection ID. zero_length_scid: bool, @@ -711,7 +765,7 @@ impl ConnectionIdentifiers { PathConnectionIdentifiers::new( destination_conn_id_limit, source_conn_id_limit, - &initial_scid, + initial_scid, Some(network_path), reset_token, ), @@ -729,9 +783,7 @@ impl ConnectionIdentifiers { #[inline] fn get_pcids(&self, path_id: PathId) -> Option<&PathConnectionIdentifiers> { - self.pcids - .iter() - .find_map(|pcid| (pcid.path_id == path_id).then_some(pcid)) + self.pcids.iter().find(|pcid| pcid.path_id == path_id) } #[inline] @@ -757,13 +809,29 @@ impl ConnectionIdentifiers { } } - /// Sets the maximum number of the path ID we could have. - pub fn set_max_path_id(&mut self, v: PathId) { - // FIXME: we may rather limit locally the number of tracked Path IDs. - if v > self.max_path_id { - self.max_path_id = v; - // Assumption: only client-initiated paths. - self.pcids.resize((1 + v / 2) as usize); + /// When the max path ID change. + fn maybe_resize_pcids(&mut self) { + // Assumption: only client-initiated paths. + self.pcids.resize((1 + self.max_path_id() / 2) as usize); + } + + /// Sets the maximum path ID we allow, and specify if we should advertise + /// this new value. + pub fn set_local_max_path_id(&mut self, v: PathId, advertise: bool) { + if v > self.local_max_path_id { + self.local_max_path_id = v; + if advertise { + self.advertise_local_max_path_id = true; + } + self.maybe_resize_pcids(); + } + } + + /// Sets the maximum path ID our peer allows. + pub fn set_remote_max_path_id(&mut self, v: PathId) { + if v > self.remote_max_path_id { + self.remote_max_path_id = v; + self.maybe_resize_pcids(); } } @@ -970,13 +1038,13 @@ impl ConnectionIdentifiers { pub fn retire_scid( &mut self, path_id: PathId, seq: CIDSeq, pkt_dcid: &ConnectionId, ) -> Result> { - // If the path ID does not exist anymore, returns an error. - let entry = self - .get_pcids_mut(path_id) - .ok_or(Error::OutOfPathId) - .and_then(|pcid| pcid.retire_scid(seq, pkt_dcid))?; + // If the path ID does not exist anymore, ignore the request. + let pcid = match self.get_pcids_mut(path_id) { + Some(p) => p, + None => return Ok(None), + }; - match entry { + match pcid.retire_scid(seq, pkt_dcid)? { Some(e) => { // Notifies the application. self.retired_scids.push_back((path_id, e.cid)); @@ -1005,6 +1073,10 @@ impl ConnectionIdentifiers { if self.zero_length_dcid { return Err(Error::InvalidState); } + // If we later receive a retire DCID for a closed path ID, ignore. + if self.closed_path_id.contains(&path_id) { + return Ok(None); + } let network_path = self .get_pcids_mut(path_id) .ok_or(Error::OutOfPathId) @@ -1095,13 +1167,11 @@ impl ConnectionIdentifiers { #[inline] pub fn available_scids_on_path(&self, path_id: PathId) -> usize { self.get_pcids(path_id) - .and_then(|pcid| { - Some( - pcid.scids - .iter() - .filter(|e| e.network_path.is_none()) - .count(), - ) + .map(|pcid| { + pcid.scids + .iter() + .filter(|e| e.network_path.is_none()) + .count() }) .unwrap_or(0) } @@ -1127,13 +1197,11 @@ impl ConnectionIdentifiers { return 0; } self.get_pcids(path_id) - .and_then(|pcid| { - Some( - pcid.dcids - .iter() - .filter(|e| e.network_path.is_none()) - .count(), - ) + .map(|pcid| { + pcid.dcids + .iter() + .filter(|e| e.network_path.is_none()) + .count() }) .unwrap_or(0) } @@ -1144,6 +1212,22 @@ impl ConnectionIdentifiers { self.pcids.get_oldest().scids.get_oldest() } + /// Returns the oldest active source Connection ID on a given path ID. + #[inline] + pub fn oldest_scid_on_path( + &self, path_id: PathId, + ) -> Option<&ConnectionIdEntry> { + self.get_pcids(path_id).map(|pcid| pcid.scids.get_oldest()) + } + + /// Returns the oldest active destination Connection ID on a given path ID. + #[inline] + pub fn oldest_dcid_on_path( + &self, path_id: PathId, + ) -> Option<&ConnectionIdEntry> { + self.get_pcids(path_id).map(|pcid| pcid.dcids.get_oldest()) + } + /// Returns the oldest known active destination Connection ID of this /// connection. /// @@ -1300,7 +1384,25 @@ impl ConnectionIdentifiers { /// Returns the max path ID authorized on this connection. #[inline] pub fn max_path_id(&self) -> PathId { - self.max_path_id + self.local_max_path_id.min(self.remote_max_path_id) + } + + /// Returns the local max path ID we allow on this connection. + #[inline] + pub fn local_max_paths_id(&self) -> PathId { + self.local_max_path_id + } + + /// Whether we should send a MAX_PATHS frame. + #[inline] + pub fn should_send_max_paths(&self) -> bool { + self.advertise_local_max_path_id + } + + /// When a MAX_PATHS frame has been sent. + #[inline] + pub fn on_max_paths_sent(&mut self) { + self.advertise_local_max_path_id = false; } /// Returns the largest path ID number seen. @@ -1309,7 +1411,45 @@ impl ConnectionIdentifiers { self.largest_path_id } + pub fn closing_path_id(&mut self, path_id: PathId) { + if let Some(pcid) = self.get_pcids_mut(path_id) { + pcid.closing = true; + } + } + + /// Remove a path identifier from the Connection IDs. + pub fn remove_path_id(&mut self, path_id: PathId) -> Result<()> { + if self.closed_path_id.contains(&path_id) { + warn!( + "retiring CIDs of an already closed path id {}; ignoring", + path_id + ); + return Ok(()); + } + if path_id > self.max_path_id() { + return Err(Error::MultiPathViolation); + } + // We may close a path ID for which we have no state. + if let Some(mut pcid) = self.pcids.remove(path_id, false)? { + for e in pcid.dcids.inner.drain(..) { + self.retire_dcid_seqs.push_back((path_id, e.seq)); + } + for e in pcid.scids.inner.drain(..) { + self.retired_scids.push_back((path_id, e.cid)); + } + }; + // Record that the path ID was closed. + self.closed_path_id.push(path_id); + Ok(()) + } + + /// An iterator over valid path IDs. + pub fn path_ids(&self) -> PathIdIter { + PathIdIter::from(self.max_path_id(), &self.closed_path_id) + } + /// Helper function for tests. + #[allow(dead_code)] pub fn next_scid_seq_on_path_id(&self, path_id: PathId) -> CIDSeq { self.get_pcids(path_id) .map(|p| p.next_scid_seq()) diff --git a/quiche/src/lib.rs b/quiche/src/lib.rs index 62b81817fd..88c5dbcd37 100644 --- a/quiche/src/lib.rs +++ b/quiche/src/lib.rs @@ -382,6 +382,7 @@ #[macro_use] extern crate log; +use cid::PathIdIter; #[cfg(feature = "qlog")] use qlog::events::connectivity::TransportOwner; #[cfg(feature = "qlog")] @@ -1157,7 +1158,7 @@ impl Config { /// number of concurrent paths. pub fn set_initial_max_paths(&mut self, v: u64) { if v < 2_u64.pow(32) { - self.local_transport_params.initial_max_paths = Some(v.into()); + self.local_transport_params.initial_max_paths = Some(v); } } @@ -3087,27 +3088,21 @@ impl Connection { } for path_id in self.path_id_to_abandon.drain(..) { - // The path might be already abandoned. - todo!("Fix abandon path"); - // if let Ok(e) = self.ids.get_dcid(dcid_seq) { - // if let Some(pid) = e.network_path { - // let (lost_packets, lost_bytes) = close_path( - // &mut self.ids, - // &mut self.pkt_num_spaces, - // &mut self.paths, - // pid, - // now, - // &self.trace_id, - // )?; - // self.lost_count += lost_packets; - // self.lost_bytes += lost_bytes as u64; - // } - // } + let (lost_packets, lost_bytes) = close_path( + &mut self.ids, + &mut self.pkt_num_spaces, + &mut self.paths, + path_id, + now, + &self.trace_id, + )?; + self.lost_count += lost_packets; + self.lost_bytes += lost_bytes as u64; } // Now that we processed all the frames, if there is a path that has no // Destination CID, try to allocate one. - for (pid, p) in self + for (_, p) in self .paths .iter_mut() .filter(|(_, p)| p.active_dcid_seq.is_none()) @@ -4175,19 +4170,20 @@ impl Connection { // Create RETIRE_CONNECTION_ID frames as needed. while let Some((path_id, seq_num)) = self.ids.next_retire_dcid_seq() { - if path_id != 0 { - todo!("retire connection id path_id != 0"); - } // 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. let dcid_seq = path.active_dcid_seq.ok_or(Error::InvalidState)?; - if seq_num == dcid_seq { + if path.path_id() == 0 && seq_num == dcid_seq { continue; } - let frame = frame::Frame::RetireConnectionId { seq_num }; + let frame = if path_id != 0 { + frame::Frame::MpRetireConnectionId { path_id, seq_num } + } else { + frame::Frame::RetireConnectionId { seq_num } + }; if push_frame_to_pkt!(b, frames, frame, left) { self.ids.mark_retire_dcid_seq(path_id, seq_num, false); @@ -4212,6 +4208,9 @@ impl Connection { }; if push_frame_to_pkt!(b, frames, frame, left) { self.paths.on_path_abandon_sent(pid, now)?; + // We should also keep in mind that we may retire the last CID + // related to Path Id. + self.ids.closing_path_id(path_id); ack_eliciting = true; in_flight = true; @@ -4220,6 +4219,19 @@ impl Connection { } } + // Create MAX_PATHS frames as needed. + if self.ids.should_send_max_paths() { + let frame = frame::Frame::MaxPaths { + max_path_id: self.ids.local_max_paths_id(), + }; + if push_frame_to_pkt!(b, frames, frame, left) { + self.ids.on_max_paths_sent(); + + ack_eliciting = true; + in_flight = true; + } + } + // Create PATH_AVAILABLE/PATH_STANDBY frames as needed. while let Some((path_id, seq_num, available)) = self.paths.path_status() @@ -6017,14 +6029,8 @@ impl Connection { if let Some(timer) = p.closing_timer() { if timer <= now { trace!("{} path closing timeout expired", self.trace_id); - todo!("Proper path closing timeout"); - // if let Some(dcid_seq) = p.active_dcid_seq { - // self.ids.retire_dcid(dcid_seq).ok(); - // self.pkt_num_spaces - // .spaces - // .update_lowest_active_tx_id(self.ids. - // min_dcid_seq()); } - // p.on_closing_timeout(); + self.path_id_to_abandon.push_back(p.path_id()); + p.on_closing_timeout(); } } if let Some(timer) = p.recovery.loss_detection_timer() { @@ -6050,6 +6056,24 @@ impl Connection { } } + // If some path has faced closing timeout, close them now. + for path_id in self.path_id_to_abandon.drain(..) { + match close_path( + &mut self.ids, + &mut self.pkt_num_spaces, + &mut self.paths, + path_id, + now, + &self.trace_id, + ) { + Ok((lc, lb)) => { + self.lost_count += lc; + self.lost_bytes += lb as u64; + }, + Err(e) => error!("Got an error when trying to close path: {e:?}"), + }; + } + // Notify timeout events to the application. self.paths.notify_failed_validations(); self.paths.notify_closed_paths(); @@ -6390,7 +6414,7 @@ impl Connection { /// Returns the number of source Connection IDs active over a given PathId. /// This is only useful when multipath is enabled. pub fn active_scids_on_path(&self, path_id: PathId) -> usize { - todo!("active_scids per path") + self.ids.active_source_cids_on_path(path_id) } /// Returns the number of source Connection IDs that are active. This is @@ -6418,9 +6442,7 @@ impl Connection { self.local_transport_params.active_conn_id_limit, ) as usize; - todo!("pif"); - - max_active_source_cids - self.active_scids() + max_active_source_cids - self.active_scids_on_path(path_id) } /// Returns the number of source Connection IDs that should be provided @@ -6783,9 +6805,19 @@ impl Connection { self.session.as_deref() } + /// Returns the source connection ID on the given Path ID. + /// + /// When there are multiple IDs, and if there is an active path, the ID used + /// on that path is returned. Otherwise the oldest ID is returned. If the + /// Path ID does not exist, return `None`. + /// + /// Note that the value returned can change throughout the connection's + /// lifetime. #[inline] pub fn source_id_on_path(&self, path_id: PathId) -> Option { - todo!() + self.ids + .oldest_scid_on_path(path_id) + .map(|e| ConnectionId::from_ref(e.cid.as_ref())) } /// Returns the source connection ID. @@ -6819,11 +6851,21 @@ impl Connection { self.ids.scids_iter() } + /// Returns the destination connection ID on the given Path ID. + /// + /// When there are multiple IDs, and if there is an active path, the ID used + /// on that path is returned. Otherwise the oldest ID is returned. If the + /// Path ID does not exist, return `None`. + /// + /// Note that the value returned can change throughout the connection's + /// lifetime. #[inline] pub fn destination_id_on_path( &self, path_id: PathId, ) -> Option { - todo!() + self.ids + .oldest_dcid_on_path(path_id) + .map(|e| ConnectionId::from_ref(e.cid.as_ref())) } /// Returns the destination connection ID. @@ -6845,6 +6887,26 @@ impl Connection { ConnectionId::from_ref(e.cid.as_ref()) } + /// Set the maximum Path ID we could allow on this connection. + /// Only meaningful if multipath is enabled. + #[inline] + pub fn set_max_paths_id(&mut self, v: PathId) { + self.ids.set_local_max_path_id(v, true); + } + + /// The maximum path ID enabled on this connection. Only meaningful + /// if multipath is enabled. + #[inline] + pub fn max_paths_id(&self) -> PathId { + self.ids.max_path_id() + } + + /// An iterator over valid Path IDs. + #[inline] + pub fn path_ids(&self) -> PathIdIter { + self.ids.path_ids() + } + /// Returns true if the connection handshake is complete. #[inline] pub fn is_established(&self) -> bool { @@ -7092,7 +7154,8 @@ impl Connection { peer_params.initial_max_paths, ) { self.paths.set_multipath(true); - self.ids.set_max_path_id(local.min(peer)); + self.ids.set_local_max_path_id(local, false); + self.ids.set_remote_max_path_id(peer); } self.peer_transport_params = peer_params; @@ -7274,6 +7337,7 @@ impl Connection { self.streams.has_stopped() || self.ids.has_new_scids() || self.ids.has_retire_dcids() || + self.ids.should_send_max_paths() || self.paths.has_path_abandon() || self.paths.has_path_status() || send_path.needs_ack_eliciting || @@ -7732,32 +7796,11 @@ impl Connection { return Err(Error::InvalidState); } - if let Some(network_path) = - self.ids.retire_scid(0, seq_num, &hdr.dcid)? - { + if self.ids.retire_scid(0, seq_num, &hdr.dcid)?.is_some() { if let Some(pid) = self.paths.pid_from_path_id(0) { - // TODO: this should be the explicit PathId!!! let path = self.paths.get_mut(pid)?; - // If we are closing the path and retiring the SCID before - // receiving the ACK(_MP), it won't be possible to - // associate the space ID with the - // path, and so get the acknowledgment - // for the PATH_ABANDON. We thus assume that in such case, - // the PATH_ABANDON got acknowledged and the path is fully - // closed at this point. - if path.is_closing() { - let (lost_packets, lost_bytes) = close_path( - &mut self.ids, - &mut self.pkt_num_spaces, - &mut self.paths, - 0, - now, - &self.trace_id, - )?; - self.lost_count += lost_packets; - self.lost_bytes += lost_bytes as u64; - } else if path.active_scid_seq == Some(seq_num) { + if path.active_scid_seq == Some(seq_num) { // Maybe we already linked a new SCID to that path. // XXX: We do not remove unused paths now, we instead @@ -7913,47 +7956,27 @@ impl Connection { if !self.use_path_pkt_num_space(epoch) { return Err(Error::MultiPathViolation); } - todo!("handle path abandon with explicit path id"); - // let abandon_pid = match self - // .ids - // .get_dcid(dcid_seq_num)? - // .path_id - // .ok_or(Error::InvalidFrame) - // { - // Ok(ap) => ap, - // Err(_) => return Ok(()), - // }; - // self.paths.on_path_abandon_received( - // abandon_pid, - // error_code, - // reason, - // )?; + + if path_id > self.ids.largest_path_id() { + return Err(Error::InvalidFrame); + } + + self.paths + .on_path_abandon_received(path_id, error_code, reason)?; }, frame::Frame::PathStandby { path_id, seq_num } => { if !self.use_path_pkt_num_space(epoch) { return Err(Error::MultiPathViolation); } - todo!("handle path standby with explicit path id"); - // let pid = self - // .ids - // .get_dcid(dcid_seq_num)? - // .path_id - // .ok_or(Error::InvalidFrame)?; - // self.paths.on_path_status_received(pid, seq_num, false); + self.paths.on_path_status_received(path_id, seq_num, false); }, frame::Frame::PathAvailable { path_id, seq_num } => { if !self.use_path_pkt_num_space(epoch) { return Err(Error::MultiPathViolation); } - todo!("handle path available with explicit path id"); - // let pid = self - // .ids - // .get_dcid(dcid_seq_num)? - // .path_id - // .ok_or(Error::InvalidFrame)?; - // self.paths.on_path_status_received(pid, seq_num, true); + self.paths.on_path_status_received(path_id, seq_num, true); }, frame::Frame::MpNewConnectionId { @@ -8007,9 +8030,30 @@ impl Connection { } }, - frame::Frame::MpRetireConnectionId { path_id, seq_num } => - todo!("MpRetireConnectionId"), - frame::Frame::MaxPaths { max_path_id } => todo!("MaxPaths"), + frame::Frame::MpRetireConnectionId { path_id, seq_num } => { + if self.ids.zero_length_scid() { + return Err(Error::InvalidState); + } + + if self.ids.retire_scid(0, seq_num, &hdr.dcid)?.is_some() { + if let Some(pid) = self.paths.pid_from_path_id(path_id) { + let path = self.paths.get_mut(pid)?; + + if path.active_scid_seq == Some(seq_num) { + // Maybe we already linked a new SCID to that path. + + // XXX: We do not remove unused paths now, we instead + // wait until we need to maintain more paths than the + // host is willing to. + path.active_scid_seq = None; + } + } + } + }, + + frame::Frame::MaxPaths { max_path_id } => { + self.ids.set_remote_max_path_id(max_path_id); + }, }; Ok(()) } @@ -8446,11 +8490,10 @@ impl Connection { // Do we have a spare DCID? If we are using zero-length DCID, just use // the default having sequence 0 (note that if we exceed our local CID - // limit, the `insert_path()` call will raise an error. + // limit, the `insert_path()` call will raise an error). let dcid_seq = if self.ids.zero_length_dcid() { 0 } else { - // TODO: following line is wrong. self.ids .lowest_available_dcid_seq(path_id) .ok_or(Error::OutOfIdentifiers)? @@ -8582,25 +8625,17 @@ fn close_path( pkt_num_spaces: &mut packet::PktNumSpaceMap, paths: &mut path::PathMap, path_id: PathId, now: time::Instant, trace_id: &str, ) -> Result<(usize, usize)> { - todo!("close path") - // // If the path had a active DCID, remove it. - // if let Ok(p) = paths.get(pid) { - // if let Some(dcid_seq) = p.active_dcid_seq { - // let _ = ids.retire_dcid(dcid_seq); - // } - // pkt_num_spaces - // .spaces - // .update_lowest_active_rx_id(ids.min_scid_seq()); - // pkt_num_spaces - // .spaces - // .update_lowest_active_tx_id(ids.min_dcid_seq()); - // } - // paths.on_path_abandon_acknowledged(pid); - // debug!("Closing path with pid {}; cleaning recovery", pid); - // let abandon_path = paths.get_mut(pid)?; - // Ok(abandon_path - // .recovery - // .mark_all_inflight_as_lost(now, trace_id)) + // Let do not expect anymore packets from now. We may need to optimize + // this a bit for some PTO, but start being simple. + debug!( + "{}: Closing path with pid {}; cleaning recovery", + trace_id, path_id + ); + ids.remove_path_id(path_id)?; + pkt_num_spaces + .spaces + .remove_application_data_space_id(path_id)?; + Ok(paths.on_path_abandon_acknowledged(path_id, now, trace_id)) } struct AddrTupleFmt(SocketAddr, SocketAddr); @@ -18003,6 +18038,8 @@ mod tests { assert!(path_s2c_0.recovery.bytes_sent >= DATA_BYTES / 2); assert!(path_s2c_1.recovery.bytes_sent >= DATA_BYTES / 2); + // TODO: need to test MP_RETIRE_CONNECTION_ID. + // Now close the initial path. assert_eq!( pipe.client.abandon_path( diff --git a/quiche/src/packet.rs b/quiche/src/packet.rs index 8cf5574e2d..2c925c68d5 100644 --- a/quiche/src/packet.rs +++ b/quiche/src/packet.rs @@ -1031,7 +1031,13 @@ impl PktNumSpaceImplMap { } pub fn remove_application_data_space_id(&mut self, id: PathId) -> Result<()> { - todo!("remove application data space id") + if self.application_pkt_num_spaces.len() <= 1 { + return Err(Error::OutOfPathId); + } + if self.application_pkt_num_spaces.remove(&id).is_none() { + warn!("Trying to remove non-present pkt num space with id {}; continuing", id); + } + Ok(()) } pub fn application_data_space_ids( diff --git a/quiche/src/path.rs b/quiche/src/path.rs index d47fa202e1..84ef54fbc0 100644 --- a/quiche/src/path.rs +++ b/quiche/src/path.rs @@ -506,9 +506,6 @@ impl Path { pub fn on_closing_timeout(&mut self) { self.closing_timer = None; - if let PathState::Closing(e, r) = &mut self.state { - self.state = PathState::Closed(*e, std::mem::take(r)); - } } pub fn closing_error_code_and_reason(&self) -> Result<(u64, Vec)> { @@ -905,9 +902,7 @@ impl PathMap { /// it returns [`Done`]. /// /// [`Done`]: enum.Error.html#variant.Done - pub fn insert_path( - &mut self, mut path: Path, is_server: bool, - ) -> Result { + pub fn insert_path(&mut self, path: Path, is_server: bool) -> Result { self.make_room_for_new_path()?; let path_id = path.path_id; @@ -1019,34 +1014,52 @@ impl PathMap { } /// Handles acknowledged PATH_ABANDONs. - pub fn on_path_abandon_acknowledged(&mut self, abandon_path_id: usize) { - if let Ok(path) = self.get_mut(abandon_path_id) { - let path_id = path.path_id; - let local_addr = path.local_addr; - let peer_addr = path.peer_addr; - let to_notify = if let PathState::Closing(e, r) = &mut path.state { - let to_notify = Some((*e, r.clone())); - path.state = PathState::Closed(*e, std::mem::take(r)); - to_notify - } else { - None - }; - if let Some((e, r)) = to_notify { - self.notify_event( - path_id, - PathEvent::Closed(local_addr, peer_addr, e, r), - ); - } + pub fn on_path_abandon_acknowledged( + &mut self, abandon_path_id: PathId, now: time::Instant, trace_id: &str, + ) -> (usize, usize) { + let abandon_pid = match self.pid_from_path_id(abandon_path_id) { + Some(pid) => pid, + None => return (0, 0), + }; + let path = match self.get_mut(abandon_pid) { + Ok(p) => p, + Err(_) => return (0, 0), + }; + + let path_id = path.path_id; + let local_addr = path.local_addr; + let peer_addr = path.peer_addr; + let to_notify = if let PathState::Closing(e, r) = &mut path.state { + let to_notify = Some((*e, r.clone())); + path.state = PathState::Closed(*e, std::mem::take(r)); + to_notify + } else { + None + }; + let lost_info = path.recovery.mark_all_inflight_as_lost(now, trace_id); + if let Some((e, r)) = to_notify { + self.notify_event( + path_id, + PathEvent::Closed(local_addr, peer_addr, e, r), + ); } + lost_info } /// Handles incoming PATH_ABANDONs. pub fn on_path_abandon_received( - &mut self, abandon_path_id: usize, error_code: u64, reason: Vec, + &mut self, abandon_path_id: PathId, error_code: u64, reason: Vec, ) -> Result<()> { let is_server = self.is_server; let nb_paths = self.paths.len(); - let abandon_path = self.get_mut(abandon_path_id)?; + let abandon_pid = match self.pid_from_path_id(abandon_path_id) { + Some(pid) => pid, + None => { + warn!("Received path abandon for path_id {}, but it is unknown; ingore PathAbandon", abandon_path_id); + return Ok(()); + }, + }; + let abandon_path = self.get_mut(abandon_pid)?; // If we are the server, and receiving a PATH_ABANDON for the only // active path, request a connection closure. if is_server && nb_paths == 1 { @@ -1060,7 +1073,7 @@ impl PathMap { abandon_path.set_state(PathState::Closing(error_code, reason))?; abandon_path.on_abandon_received(); if !was_closing { - self.mark_path_abandon(abandon_path_id, true); + self.mark_path_abandon(abandon_pid, true); } Ok(()) } @@ -1197,9 +1210,13 @@ impl PathMap { /// Handles the reception of PATH_STANDBY/PATH_AVAILABLE. pub fn on_path_status_received( - &mut self, path_id: usize, seq_num: u64, available: bool, + &mut self, path_id: PathId, seq_num: u64, available: bool, ) { - if let Ok(p) = self.get_mut(path_id) { + let pid = match self.pid_from_path_id(path_id) { + Some(p) => p, + None => return, + }; + if let Ok(p) = self.get_mut(pid) { if seq_num >= p.expected_path_status_seq_num { p.expected_path_status_seq_num = seq_num.saturating_add(1); let path_id = p.path_id();