diff --git a/Cargo.lock b/Cargo.lock index 3d988de4b188..eeeb9c33f8cd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3962,7 +3962,6 @@ dependencies = [ [[package]] name = "neqo-bin" version = "0.7.9" -source = "git+https://github.com/mozilla/neqo?tag=v0.7.9#121fe683ae4b39a5b694f671abfd397cbd9b4322" dependencies = [ "clap", "clap-verbosity-flag", @@ -3984,7 +3983,6 @@ dependencies = [ [[package]] name = "neqo-common" version = "0.7.9" -source = "git+https://github.com/mozilla/neqo?tag=v0.7.9#121fe683ae4b39a5b694f671abfd397cbd9b4322" dependencies = [ "enum-map", "env_logger", @@ -3997,7 +3995,6 @@ dependencies = [ [[package]] name = "neqo-crypto" version = "0.7.9" -source = "git+https://github.com/mozilla/neqo?tag=v0.7.9#121fe683ae4b39a5b694f671abfd397cbd9b4322" dependencies = [ "bindgen 0.69.4", "log", @@ -4012,7 +4009,6 @@ dependencies = [ [[package]] name = "neqo-http3" version = "0.7.9" -source = "git+https://github.com/mozilla/neqo?tag=v0.7.9#121fe683ae4b39a5b694f671abfd397cbd9b4322" dependencies = [ "enumset", "log", @@ -4029,7 +4025,6 @@ dependencies = [ [[package]] name = "neqo-qpack" version = "0.7.9" -source = "git+https://github.com/mozilla/neqo?tag=v0.7.9#121fe683ae4b39a5b694f671abfd397cbd9b4322" dependencies = [ "log", "neqo-common", @@ -4042,7 +4037,6 @@ dependencies = [ [[package]] name = "neqo-transport" version = "0.7.9" -source = "git+https://github.com/mozilla/neqo?tag=v0.7.9#121fe683ae4b39a5b694f671abfd397cbd9b4322" dependencies = [ "enum-map", "indexmap 2.2.6", diff --git a/Cargo.toml b/Cargo.toml index bfca80a98b43..a4248187bef4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -236,3 +236,11 @@ mio_0_8 = { package = "mio", git = "https://github.com/glandium/mio", rev = "9a2 # Patch `gpu-descriptor` 0.3.0 to remove unnecessary `allocator-api2` dep.: # Still waiting for the now-merged to be released. gpu-descriptor = { git = "https://github.com/zakarumych/gpu-descriptor", rev = "7b71a4e47c81903ad75e2c53deb5ab1310f6ff4d" } + +[patch."https://github.com/mozilla/neqo"] +neqo-bin = { path = "third_party/rust/neqo-bin" } +neqo-common = { path = "third_party/rust/neqo-common" } +neqo-crypto = { path = "third_party/rust/neqo-crypto" } +neqo-http3 = { path = "third_party/rust/neqo-http3" } +neqo-qpack = { path = "third_party/rust/neqo-qpack" } +neqo-transport = { path = "third_party/rust/neqo-transport" } diff --git a/third_party/rust/neqo-transport/src/connection/mod.rs b/third_party/rust/neqo-transport/src/connection/mod.rs index 77c39abff300..1c3a6f1071a6 100644 --- a/third_party/rust/neqo-transport/src/connection/mod.rs +++ b/third_party/rust/neqo-transport/src/connection/mod.rs @@ -1600,6 +1600,17 @@ impl Connection { // on the assert for doesn't exist. // OK, we have a valid packet. + // Get the next packet number we'll send, for ACK verification. + // TODO: Once PR #2118 lands, this can move to `input_frame`. For now, it needs to be here, + // because we can drop packet number spaces as we parse throught the packet, and if an ACK + // frame follows a CRYPTO frame that makes us drop a space, we need to know this + // packet number to verify the ACK against. + let next_pn = self + .crypto + .states + .select_tx(self.version, PacketNumberSpace::from(packet.packet_type())) + .map_or(0, |(_, tx)| tx.next_pn()); + let mut ack_eliciting = false; let mut probing = true; let mut d = Decoder::from(&packet[..]); @@ -1612,7 +1623,14 @@ impl Connection { ack_eliciting |= f.ack_eliciting(); probing &= f.path_probing(); let t = f.get_type(); - if let Err(e) = self.input_frame(path, packet.version(), packet.packet_type(), f, now) { + if let Err(e) = self.input_frame( + path, + packet.version(), + packet.packet_type(), + f, + next_pn, + now, + ) { self.capture_error(Some(Rc::clone(path)), now, t, Err(e))?; } } @@ -2712,6 +2730,7 @@ impl Connection { packet_version: Version, packet_type: PacketType, frame: Frame, + next_pn: PacketNumber, now: Instant, ) -> Res<()> { if !frame.is_allowed(packet_type) { @@ -2746,16 +2765,17 @@ impl Connection { ack_ranges, ecn_count, } => { + // Ensure that the largest acknowledged packet number was actually sent. + // (If we ever start using non-contiguous packet numbers, we need to check all the + // packet numbers in the ACKed ranges.) + if largest_acknowledged >= next_pn { + qwarn!("Largest ACKed {} was never sent", largest_acknowledged); + return Err(Error::AckedUnsentPacket); + } + let ranges = Frame::decode_ack_frame(largest_acknowledged, first_ack_range, &ack_ranges)?; - self.handle_ack( - space, - largest_acknowledged, - ranges, - ecn_count, - ack_delay, - now, - ); + self.handle_ack(space, ranges, ecn_count, ack_delay, now); } Frame::Crypto { offset, data } => { qtrace!( @@ -2929,7 +2949,6 @@ impl Connection { fn handle_ack( &mut self, space: PacketNumberSpace, - largest_acknowledged: PacketNumber, ack_ranges: R, ack_ecn: Option, ack_delay: u64, @@ -2946,12 +2965,12 @@ impl Connection { let (acked_packets, lost_packets) = self.loss_recovery.on_ack_received( &path, space, - largest_acknowledged, ack_ranges, ack_ecn, self.decode_ack_delay(ack_delay), now, ); + let largest_acknowledged = acked_packets.first().map(SentPacket::pn); for acked in acked_packets { for token in acked.tokens() { match token { @@ -2975,7 +2994,9 @@ impl Connection { qlog::packets_lost(&mut self.qlog, &lost_packets); let stats = &mut self.stats.borrow_mut().frame_rx; stats.ack += 1; - stats.largest_acknowledged = max(stats.largest_acknowledged, largest_acknowledged); + if let Some(largest_acknowledged) = largest_acknowledged { + stats.largest_acknowledged = max(stats.largest_acknowledged, largest_acknowledged); + } } /// When the server rejects 0-RTT we need to drop a bunch of stuff. diff --git a/third_party/rust/neqo-transport/src/connection/tests/recovery.rs b/third_party/rust/neqo-transport/src/connection/tests/recovery.rs index 0f12d0310759..01bb9abbeadd 100644 --- a/third_party/rust/neqo-transport/src/connection/tests/recovery.rs +++ b/third_party/rust/neqo-transport/src/connection/tests/recovery.rs @@ -802,3 +802,38 @@ fn fast_pto_persistent_congestion() { client.process_input(&ack.unwrap(), now); assert_eq!(cwnd(&client), CWND_MIN); } + +/// Receiving an ACK frame for a packet number that was never sent is an error. +#[test] +fn ack_for_unsent() { + /// This inserts an ACK frame into packets that ACKs a packet that was never sent. + struct AckforUnsentWriter {} + + impl FrameWriter for AckforUnsentWriter { + fn write_frames(&mut self, builder: &mut PacketBuilder) { + builder.encode_varint(FRAME_TYPE_ACK); + builder.encode_varint(666u16); // Largest ACKed + builder.encode_varint(0u8); // ACK delay + builder.encode_varint(0u8); // ACK block count + builder.encode_varint(0u8); // ACK block length + } + } + + let mut client = default_client(); + let mut server = default_server(); + connect_force_idle(&mut client, &mut server); + + server.test_frame_writer = Some(Box::new(AckforUnsentWriter {})); + let spoofed = server.process_output(now()).dgram().unwrap(); + server.test_frame_writer = None; + + // Now deliver the packet with the spoofed ACK frame + client.process_input(&spoofed, now()); + assert!(matches!( + client.state(), + State::Closing { + error: CloseReason::Transport(Error::AckedUnsentPacket), + .. + } + )); +} diff --git a/third_party/rust/neqo-transport/src/recovery/mod.rs b/third_party/rust/neqo-transport/src/recovery/mod.rs index b181bd88a0f0..ce699d6072ed 100644 --- a/third_party/rust/neqo-transport/src/recovery/mod.rs +++ b/third_party/rust/neqo-transport/src/recovery/mod.rs @@ -587,7 +587,6 @@ impl LossRecovery { &mut self, primary_path: &PathRef, pn_space: PacketNumberSpace, - largest_acked: PacketNumber, acked_ranges: R, ack_ecn: Option, ack_delay: Duration, @@ -597,13 +596,6 @@ impl LossRecovery { R: IntoIterator>, R::IntoIter: ExactSizeIterator, { - qdebug!( - [self], - "ACK for {} - largest_acked={}.", - pn_space, - largest_acked - ); - let Some(space) = self.spaces.get_mut(pn_space) else { qinfo!("ACK on discarded space"); return (Vec::new(), Vec::new()); @@ -618,8 +610,8 @@ impl LossRecovery { // Track largest PN acked per space let prev_largest_acked = space.largest_acked_sent_time; - if Some(largest_acked) > space.largest_acked { - space.largest_acked = Some(largest_acked); + if Some(largest_acked_pkt.pn()) > space.largest_acked { + space.largest_acked = Some(largest_acked_pkt.pn()); // If the largest acknowledged is newly acked and any newly acked // packet was ack-eliciting, update the RTT. (-recovery 5.1) @@ -634,6 +626,13 @@ impl LossRecovery { } } + qdebug!( + [self], + "ACK for {} - largest_acked={}", + pn_space, + largest_acked_pkt.pn() + ); + // Perform loss detection. // PTO is used to remove lost packets from in-flight accounting. // We need to ensure that we have sent any PTO probes before they are removed @@ -978,21 +977,13 @@ mod tests { pub fn on_ack_received( &mut self, pn_space: PacketNumberSpace, - largest_acked: PacketNumber, acked_ranges: Vec>, ack_ecn: Option, ack_delay: Duration, now: Instant, ) -> (Vec, Vec) { - self.lr.on_ack_received( - &self.path, - pn_space, - largest_acked, - acked_ranges, - ack_ecn, - ack_delay, - now, - ) + self.lr + .on_ack_received(&self.path, pn_space, acked_ranges, ack_ecn, ack_delay, now) } pub fn on_packet_sent(&mut self, sent_packet: SentPacket) { @@ -1144,7 +1135,6 @@ mod tests { fn ack(lr: &mut Fixture, pn: u64, delay: Duration) { lr.on_ack_received( PacketNumberSpace::ApplicationData, - pn, vec![pn..=pn], None, ACK_DELAY, @@ -1298,7 +1288,6 @@ mod tests { )); let (_, lost) = lr.on_ack_received( PacketNumberSpace::ApplicationData, - 1, vec![1..=1], None, ACK_DELAY, @@ -1322,7 +1311,6 @@ mod tests { let (_, lost) = lr.on_ack_received( PacketNumberSpace::ApplicationData, - 2, vec![2..=2], None, ACK_DELAY, @@ -1352,7 +1340,6 @@ mod tests { assert_eq!(super::PACKET_THRESHOLD, 3); let (_, lost) = lr.on_ack_received( PacketNumberSpace::ApplicationData, - 4, vec![2..=4], None, ACK_DELAY, @@ -1381,7 +1368,6 @@ mod tests { lr.discard(PacketNumberSpace::Initial, now()); let (acked, lost) = lr.on_ack_received( PacketNumberSpace::Initial, - 0, vec![], None, Duration::from_millis(0), @@ -1441,7 +1427,6 @@ mod tests { lr.on_packet_sent(sent_pkt); lr.on_ack_received( pn_space, - 1, vec![1..=1], None, Duration::from_secs(0), @@ -1494,7 +1479,6 @@ mod tests { let rtt = lr.path.borrow().rtt().estimate(); lr.on_ack_received( PacketNumberSpace::Initial, - 0, vec![0..=0], None, Duration::new(0, 0),