Skip to content

Re-issue a WINDOW_ADJUST refused during key exchange - #257

Open
cristicalin wants to merge 1 commit into
apple:mainfrom
simplekube-ro:simplekube/0.15.0-window-adjust-rollback
Open

cristicalin wants to merge 1 commit into
apple:mainfrom
simplekube-ro:simplekube/0.15.0-window-adjust-rollback

Conversation

@cristicalin

Copy link
Copy Markdown

Re-issue a CHANNEL_WINDOW_ADJUST that the connection refuses during a key exchange, instead of keeping its credit.

Fixes #256.

Motivation

ChildChannelWindowManager.unbufferBytes credits a window increment to the child's view of the peer's window before SSHChildChannel.deliverSingleRead writes the CHANNEL_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 small RekeyLimit, where long downloads stalled.

Modifications

  • SSHChildChannel.deliverSingleRead writes the window adjust with a promise; if the write fails, the increment is taken back through a new ChildChannelWindowManager.incrementNotSent(_:). The refusal is synchronous on the event loop, so the window is corrected before any further inbound processing, and the next unbufferBytes emits the adjust again. The subtraction saturates at zero rather than trapping.
  • New test EndToEndTests.testWindowAdjustRefusedDuringKeyExchangeIsReissuedAfterwards: with autoRead off, 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

  • Without the change the new test fails: XCTAssertEqual failed: ("8388608") is not equal to ("14388608") — the client receives exactly one initial window and stalls.
  • With it, the test passes and the full suite is green: Executed 347 tests, with 0 failures (0 unexpected). swift format lint --strict is clean on the changed files.
  • In the SFTP scenario from the issue (OpenSSH 9.6p1, 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_CLOSE dropped during one), which this PR does not touch.

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>
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.

Child channel stalls permanently when a WINDOW_ADJUST is refused during key exchange

1 participant