Skip to content

Commit

Permalink
Merge pull request #31 from mxinden/fix-ack-for-unsent-pn
Browse files Browse the repository at this point in the history 
Do not use untrusted largest_ack
  • Loading branch information
@larseggert
larseggert authored Oct 7, 2024
2 parents 3b87021 + 6551357 commit 11b8b17
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 47 deletions.
34 changes: 13 additions & 21 deletions neqo-transport/src/connection/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2872,17 +2872,17 @@ impl Connection {
ack_ranges,
ecn_count,
} => {
// Ensure that the largest acknowledged packet number was actually sent.
// (If we ever start using non-contigous 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,
next_pn,
largest_acknowledged,
ranges,
ecn_count,
ack_delay,
now,
)?;
self.handle_ack(space, ranges, ecn_count, ack_delay, now)?;
}
Frame::Crypto { offset, data } => {
qtrace!(
Expand Down Expand Up @@ -3071,8 +3071,6 @@ impl Connection {
fn handle_ack<R>(
&mut self,
space: PacketNumberSpace,
next_pn: PacketNumber,
largest_acknowledged: PacketNumber,
ack_ranges: R,
ack_ecn: Option<EcnCount>,
ack_delay: u64,
Expand All @@ -3087,23 +3085,15 @@ impl Connection {
return Ok(());
};

// Ensure that the largest acknowledged packet number was actually sent.
// (If we ever start using non-contigous 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 (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 {
Expand All @@ -3127,7 +3117,9 @@ impl Connection {
qlog::packets_lost(&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(la) = largest_acknowledged {
stats.largest_acknowledged = max(stats.largest_acknowledged, la);
}
Ok(())
}

Expand Down
38 changes: 12 additions & 26 deletions neqo-transport/src/recovery/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,6 @@ impl LossRecovery {
&mut self,
primary_path: &PathRef,
pn_space: PacketNumberSpace,
largest_acked: PacketNumber,
acked_ranges: R,
ack_ecn: Option<EcnCount>,
ack_delay: Duration,
Expand All @@ -588,12 +587,7 @@ impl LossRecovery {
R: IntoIterator<Item = RangeInclusive<PacketNumber>>,
R::IntoIter: ExactSizeIterator,
{
qdebug!(
[self],
"ACK for {} - largest_acked={}.",
pn_space,
largest_acked
);
qdebug!([self], "ACK for {}.", pn_space,);

let Some(space) = self.spaces.get_mut(pn_space) else {
qinfo!("ACK on discarded space");
Expand All @@ -609,8 +603,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)
Expand All @@ -625,6 +619,13 @@ impl LossRecovery {
}
}

qdebug!(
[self],
"ACK pn_space={} 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
Expand Down Expand Up @@ -978,21 +979,13 @@ mod tests {
pub fn on_ack_received(
&mut self,
pn_space: PacketNumberSpace,
largest_acked: PacketNumber,
acked_ranges: Vec<RangeInclusive<PacketNumber>>,
ack_ecn: Option<EcnCount>,
ack_delay: Duration,
now: Instant,
) -> (Vec<SentPacket>, Vec<SentPacket>) {
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) {
Expand Down Expand Up @@ -1145,7 +1138,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,
Expand Down Expand Up @@ -1299,7 +1291,6 @@ mod tests {
));
let (_, lost) = lr.on_ack_received(
PacketNumberSpace::ApplicationData,
1,
vec![1..=1],
None,
ACK_DELAY,
Expand All @@ -1323,7 +1314,6 @@ mod tests {

let (_, lost) = lr.on_ack_received(
PacketNumberSpace::ApplicationData,
2,
vec![2..=2],
None,
ACK_DELAY,
Expand Down Expand Up @@ -1353,7 +1343,6 @@ mod tests {
assert_eq!(super::PACKET_THRESHOLD, 3);
let (_, lost) = lr.on_ack_received(
PacketNumberSpace::ApplicationData,
4,
vec![2..=4],
None,
ACK_DELAY,
Expand All @@ -1375,7 +1364,6 @@ mod tests {
lr.discard(PacketNumberSpace::Initial, now());
let (acked, lost) = lr.on_ack_received(
PacketNumberSpace::Initial,
0,
vec![],
None,
Duration::from_millis(0),
Expand Down Expand Up @@ -1435,7 +1423,6 @@ mod tests {
lr.on_packet_sent(sent_pkt);
lr.on_ack_received(
pn_space,
1,
vec![1..=1],
None,
Duration::from_secs(0),
Expand Down Expand Up @@ -1488,7 +1475,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),
Expand Down

0 comments on commit 11b8b17

Please sign in to comment.