Repository navigation
Re-issue a WINDOW_ADJUST refused during key exchange - #257
Open
cristicalin wants to merge 1 commit into
Open
cristicalin wants to merge 1 commit into
cristicalin wants to merge 1 commit into
Conversation
Motivation: When a child channel delivers a read, ChildChannelWindowManager.unbufferBytes credits any window increment to its own view of the peer's window before the CHANNEL_WINDOW_ADJUST is written, and the adjust is written with a nil promise. While a key exchange is in progress the connection state machine refuses channel messages, so the adjust is dropped and nothing reports it, but the local window still believes the peer was given the increment. From then on we believe the peer has more window than it does. Once the increment we lost is more than half the target window (the usual case when the peer's messages do not divide the window evenly), or a second adjust is lost the same way, the peer runs out of window while we believe it still has most of it, so we never emit another adjust and the channel stalls forever. This was found while building an SFTP client against a server that rekeys often (OpenSSH with a small RekeyLimit): long downloads stalled permanently. Modifications: - Write the window adjust with a real promise and, if the write fails, take the increment back (new ChildChannelWindowManager.incrementNotSent). The next unbufferBytes then sees the true window and emits the adjust again. The subtraction saturates at zero rather than trapping. - Add EndToEndTests.testWindowAdjustRefusedDuringKeyExchangeIsReissuedAfterwards: the server fills most of the client's window, the client starts a rekey and then reads, so its adjust is refused; after the rekey completes, the server sends a further full window, which only arrives if the adjust is re-issued. Without the fix the client receives exactly one initial window (8388608 of 14388608 bytes). Result: A window adjust refused during a key exchange is re-issued with the next read delivered after the exchange. The re-issue still needs such a read: if the refused adjust came from the last buffered read while the peer's window was already 0, the channel can still stall. A complete fix would re-check child windows when a key exchange completes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
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.
Re-issue a
CHANNEL_WINDOW_ADJUSTthat the connection refuses during a key exchange, instead of keeping its credit.Fixes #256.
Motivation
ChildChannelWindowManager.unbufferBytescredits a window increment to the child's view of the peer's window beforeSSHChildChannel.deliverSingleReadwrites theCHANNEL_WINDOW_ADJUST, and the write uses a nil promise. While a key exchange is in progress the connection state machine refuses channel messages, so the adjust is dropped silently while the local window still counts it. When the lost increment is more than half the target window, or a second adjust is lost the same way, the peer runs out of window while we believe it still has most of it: no further adjust is sent and the channel stalls permanently. Found while building an SFTP client against OpenSSH with a smallRekeyLimit, where long downloads stalled.Modifications
SSHChildChannel.deliverSingleReadwrites the window adjust with a promise; if the write fails, the increment is taken back through a newChildChannelWindowManager.incrementNotSent(_:). The refusal is synchronous on the event loop, so the window is corrected before any further inbound processing, and the nextunbufferBytesemits the adjust again. The subtraction saturates at zero rather than trapping.EndToEndTests.testWindowAdjustRefusedDuringKeyExchangeIsReissuedAfterwards: withautoReadoff, the server fills most of the client's window; the client starts a rekey and then reads, so its adjust is refused; after the exchange completes the server sends another full window, which only arrives if the adjust is re-issued. The message size is chosen so it does not divide the half-window, which is the case where one lost adjust is fatal.Result
XCTAssertEqual failed: ("8388608") is not equal to ("14388608")— the client receives exactly one initial window and stalls.Executed 347 tests, with 0 failures (0 unexpected).swift format lint --strictis clean on the changed files.RekeyLimit 1M, 128 KiB reads × 8 in flight), 3 × 64 MiB completed across 1,528 key exchanges with 16 adjusts refused and re-issued.Not covered
The re-issue happens on the next read delivered after the key exchange. If the refused adjust came from the last buffered read while the peer's window was already 0, nothing triggers it; a complete fix would re-check child windows when a key exchange completes. The issue also lists two smaller related leaks (the outbound-window debit for writes refused during a key exchange, and a
CHANNEL_CLOSEdropped during one), which this PR does not touch.