Fix client hang when server closes during handshake - #182
Draft
josephnoir wants to merge 2 commits into
Draft
josephnoir wants to merge 2 commits into
josephnoir wants to merge 2 commits into
Conversation
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.
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
Collaborator
|
Thanks for finding this and fixing @josephnoir! |
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.