Repository navigation
Fix client hang when server closes during handshake - #182
Open
josephnoir wants to merge 3 commits into
Open
josephnoir wants to merge 3 commits into
josephnoir wants to merge 3 commits into
Conversation
Collaborator
|
Thanks for finding this and fixing @josephnoir! |
A server that closes with an application error before it has processed the client's Finished still has key state .handshake. It then sends APPLICATION_CLOSE (0x1d) in a Handshake packet, which RFC 9000 §10.2.3 forbids. Since apple#157 the client rejects that frame but drops the resulting close, so it hangs until the idle timeout. - Add testQUICServerApplicationCloseBeforeHandshakeComplete: drop all client packets after the ClientHello so the server closes at key state .handshake, and check that the client disconnects without having to reject an APPLICATION_CLOSE - Add testQUICClientClosesOnFrameNotAllowedDuringHandshake: the server sends HANDSHAKE_DONE in a Handshake packet and the client must close right away. This covers the receiver side on its own, since fixing the sender keeps the first test from reaching that path - Add waitForServerConnected to QUICTestHarness so a test can continue once only the client is connected Both tests fail until the fix in the next commit.
josephnoir
force-pushed
the
rh/error-during-handshake
branch
from
October 6, 2026 13:15
4ea57e5 to
f6fce3d
Compare
A server that closes with an application error before it has processed the client's Finished still has key state .handshake, so it sent APPLICATION_CLOSE (0x1d) in a Handshake packet. Before apple#157 the client processed that frame anyway. Since then it rejects it as a PROTOCOL_VIOLATION, but the close is deferred during inbound processing and was dropped when handleInboundPacket returned false, so the client never reported the disconnect and waited for the idle timeout. - Send CONNECTION_CLOSE (0x1c) with APPLICATION_ERROR and no reason instead of APPLICATION_CLOSE in Initial and Handshake packets (RFC 9000 §10.2.3) - Run the deferred close when handleInboundPacket stops processing a datagram
josephnoir
force-pushed
the
rh/error-during-handshake
branch
from
October 6, 2026 13:15
f6fce3d to
01a2a5d
Compare
Contributor
Author
|
You are welcome! The additional There is also an earlier close that is redundant since this is the only call site. I removed it in a separate commit, so I can easily revert that part if you would prefer to keep it. |
josephnoir
marked this pull request as ready for review
October 6, 2026 13:20
josephnoir
requested review from
agnosticdev,
ekinnear,
kkuk24,
rpaulo and
tfpauly
as code owners
October 6, 2026 13:20
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The test
testConnectionErrorin the StreamTableTests has started failing frequently with 0.4.0. Bisec points to #157 as the change in flakiness. However, the preexisting issue is that closing during the handshake sends the wrong error code. RFC 9000 10.2.3.:This PR adds regression tests and two fixes:
0x000Cwhen the packet spaceapplicationDatahas not been reached.