fix(s2n-quic-transport) Ensures packet number about to be skipped is not the first packet #2619
Conversation
| @@ -166,7 +166,7 @@ impl<Config: endpoint::Config> ApplicationSpace<Config> { | |||
| // necessary) and record only if a packet is transmitted. | |||
| let mut skipped_packet_number = SkippedPacketNumber::default(); | |||
|
|
|||
| if self.recovery_manager.requires_probe() { | |||
| if self.recovery_manager.requires_probe() && packet_number.as_u64() != 0 { | |||
There was a problem hiding this comment.
are we missing the PacketSkipped event here and in 180-198 too? or is it getting emitted somewhere else?
There was a problem hiding this comment.
Looks like the PacketSkipped event actually gets emitted in the on_packet_sent function on line 272. Seems like we didn't want to record a packet number as skipped if the packet wasn't actually sent.
There was a problem hiding this comment.
This question did make me consider, it does look like there's actually two reasons a packet number could be skipped in the application space, pto probing and optimistic ack protection. I wondered if our optimistic ack protection could cause packet number 0 to be skipped. However, looking at the Optimistic Ack code I think that the minimum skip counter is 30, so, we would never skip pn 0.
Release Summary:
Resolved issues:
resolves #2599
Description of changes:
It's technically possible for the PTO Timer to cause s2n-quic to skip the first packet number when transitioning to a new packet space. This change checks that the packet number is not zero before skipping the number, as we do not want this behavior.
Call-outs:
The PTO module doesn't really have any knowledge of which packet number we're on. Maybe it's better to add this logic inside of the requires_probe() function. But then I would have to pass in the packet number so I really didn't think it was worth it.
Testing:
slow_tls test now passes with the fips libcrypto.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.