Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cancel the PTO timer when all Handshake packets are acknowledged #3231

Merged
merged 1 commit into from Jul 15, 2021

Conversation

marten-seemann
Copy link
Member

@marten-seemann marten-seemann commented Jul 13, 2021

This can lead to busy-looping if the following contrived situation occurs:

  1. The server receives the first Initial from the client. It continues the handshake by sending its first flight.
  2. The RTT is longer than 200ms. The server retransmit the Initial packets it sent after 200ms.
  3. We never receive an ACK for the Initial packets (not exactly sure why this is necessary though).
  4. The RTT is longer than 400ms. The retransmits the Handshake packets it sent after another 200ms.
  5. None of these packets is actually lost. The server receives an ACK for all Handshake packets sent. Now the following happens:
    1. First the Initial keys are dropped because we decrypted the first Handshake packet. Crucially, this happens before any packet contents are processed. This triggers a timer reset, which sets a PTO timer based on the last Handshake packet sent.
    2. Now the ACK is processed, acknowledging all Handshake packets. There are no outstanding packets now, except for the 1-RTT packet sent in the first flight. We don't retransmit that one though until the handshake completes. At this point we need to make sure to cancel the timer set in the step before.

@codecov
Copy link

codecov bot commented Jul 13, 2021

Codecov Report

Merging #3231 (61748d4) into master (746358c) will decrease coverage by 0.01%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3231      +/-   ##
==========================================
- Coverage   85.47%   85.46%   -0.01%     
==========================================
  Files         133      133              
  Lines        9794     9799       +5     
==========================================
+ Hits         8371     8374       +3     
- Misses       1051     1052       +1     
- Partials      372      373       +1     
Impacted Files Coverage Δ
internal/ackhandler/sent_packet_handler.go 78.07% <60.00%> (-0.22%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 746358c...61748d4. Read the comment docs.

@marten-seemann
Copy link
Member Author

Merging without review. We need to get this fixed asap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant