diff --git a/neqo-common/Cargo.toml b/neqo-common/Cargo.toml index 699c799b44..7bbe3daf7d 100644 --- a/neqo-common/Cargo.toml +++ b/neqo-common/Cargo.toml @@ -8,7 +8,7 @@ license = "MIT/Apache-2.0" build = "build.rs" [dependencies] -log = {version = "0.4.0", default-features = false} +log = {version = "=0.4.17", default-features = false} env_logger = {version = "0.9", default-features = false} lazy_static = "1.3.0" qlog = "0.4.0" diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 95f24f24b9..eece932337 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -1468,6 +1468,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[..]); @@ -1492,7 +1503,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))?; } } @@ -2560,6 +2578,7 @@ impl Connection { packet_version: Version, packet_type: PacketType, frame: Frame, + next_pn: PacketNumber, now: Instant, ) -> Res<()> { if !frame.is_allowed(packet_type) { @@ -2594,9 +2613,17 @@ impl Connection { first_ack_range, ack_ranges, } => { + // 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, ack_delay, now); + self.handle_ack(space, ranges, ack_delay, now); } Frame::Crypto { offset, data } => { qtrace!( @@ -2771,7 +2798,6 @@ impl Connection { fn handle_ack( &mut self, space: PacketNumberSpace, - largest_acknowledged: u64, ack_ranges: R, ack_delay: u64, now: Instant, @@ -2784,11 +2810,11 @@ impl Connection { let (acked_packets, lost_packets) = self.loss_recovery.on_ack_received( &self.paths.primary(), space, - largest_acknowledged, ack_ranges, 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 { @@ -2812,7 +2838,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/neqo-transport/src/connection/tests/recovery.rs b/neqo-transport/src/connection/tests/recovery.rs index 0e3ee412f5..13b2d8d5f8 100644 --- a/neqo-transport/src/connection/tests/recovery.rs +++ b/neqo-transport/src/connection/tests/recovery.rs @@ -808,3 +808,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/neqo-transport/src/recovery.rs b/neqo-transport/src/recovery.rs index 51becae19d..f997428254 100644 --- a/neqo-transport/src/recovery.rs +++ b/neqo-transport/src/recovery.rs @@ -652,7 +652,6 @@ impl LossRecovery { &mut self, primary_path: &PathRef, pn_space: PacketNumberSpace, - largest_acked: u64, acked_ranges: R, ack_delay: Duration, now: Instant, @@ -661,17 +660,7 @@ impl LossRecovery { R: IntoIterator>, R::IntoIter: ExactSizeIterator, { - qdebug!( - [self], - "ACK for {} - largest_acked={}.", - pn_space, - largest_acked - ); - - let space = self.spaces.get_mut(pn_space); - let space = if let Some(sp) = space { - sp - } else { + let Some(space) = self.spaces.get_mut(pn_space) else { qinfo!("ACK on discarded space"); return (Vec::new(), Vec::new()); }; @@ -685,8 +674,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) @@ -702,6 +691,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 @@ -1035,19 +1031,12 @@ mod tests { pub fn on_ack_received( &mut self, pn_space: PacketNumberSpace, - largest_acked: u64, - acked_ranges: Vec>, + acked_ranges: Vec>, ack_delay: Duration, now: Instant, ) -> (Vec, Vec) { - self.lr.on_ack_received( - &self.path, - pn_space, - largest_acked, - acked_ranges, - ack_delay, - now, - ) + self.lr + .on_ack_received(&self.path, pn_space, acked_ranges, ack_delay, now) } pub fn on_packet_sent(&mut self, sent_packet: SentPacket) { @@ -1191,7 +1180,6 @@ mod tests { fn ack(lr: &mut Fixture, pn: u64, delay: Duration) { lr.on_ack_received( PacketNumberSpace::ApplicationData, - pn, vec![pn..=pn], ACK_DELAY, pn_time(pn) + delay, @@ -1338,7 +1326,6 @@ mod tests { )); let (_, lost) = lr.on_ack_received( PacketNumberSpace::ApplicationData, - 1, vec![1..=1], ACK_DELAY, pn_time(0) + (TEST_RTT * 5 / 4), @@ -1361,7 +1348,6 @@ mod tests { let (_, lost) = lr.on_ack_received( PacketNumberSpace::ApplicationData, - 2, vec![2..=2], ACK_DELAY, pn2_ack_time, @@ -1390,7 +1376,6 @@ mod tests { assert_eq!(super::PACKET_THRESHOLD, 3); let (_, lost) = lr.on_ack_received( PacketNumberSpace::ApplicationData, - 4, vec![2..=4], ACK_DELAY, pn_time(4), @@ -1418,7 +1403,6 @@ mod tests { lr.discard(PacketNumberSpace::Initial, now()); let (acked, lost) = lr.on_ack_received( PacketNumberSpace::Initial, - 0, vec![], Duration::from_millis(0), pn_time(0), @@ -1464,7 +1448,7 @@ mod tests { let sent_pkt = SentPacket::new(*sp, 1, pn_time(3), true, Vec::new(), ON_SENT_SIZE); let pn_space = PacketNumberSpace::from(sent_pkt.pt); lr.on_packet_sent(sent_pkt); - lr.on_ack_received(pn_space, 1, vec![1..=1], Duration::from_secs(0), pn_time(3)); + lr.on_ack_received(pn_space, vec![1..=1], Duration::from_secs(0), pn_time(3)); let mut lost = Vec::new(); lr.spaces.get_mut(pn_space).unwrap().detect_lost_packets( pn_time(3), @@ -1510,7 +1494,6 @@ mod tests { let rtt = lr.path.borrow().rtt().estimate(); lr.on_ack_received( PacketNumberSpace::Initial, - 0, vec![0..=0], Duration::new(0, 0), now() + rtt,