fix(state machine): PENDING is queued, not refused — don't tear down the transport - #269
Open
wdrs-dev wants to merge 1 commit into
Open
fix(state machine): PENDING is queued, not refused — don't tear down the transport#269wdrs-dev wants to merge 1 commit into
wdrs-dev wants to merge 1 commit into
Conversation
…ransport `processEvent` has three outcomes, not two. `IGNORED` means no transition matched — that is the refusal. `PENDING` means another event is being processed and this one was QUEUED, which is what the default `queuePendingEventHandler` does. Comparing against `PROCESSED` alone turns "queued" into "refused". The authorization guards have no effect of their own — they only ask whether a packet fits the current state — so for them `PENDING` is a fine answer: the event will be processed. But `requireAccepted` treats their `false` as a protocol violation, and a protocol violation tears down the whole transport, not just a channel. The window opens whenever the packet loop reads a packet while a locally-initiated event has suspended to write to the socket: `OpenChannel`, `SendChannelRequest`, a window-change. In an interactive session that is constantly. Observed in the wild as the client sending `SSH_MSG_DISCONNECT` reason 2 with `Unexpected SSH packet SSH_MSG_CHANNEL_DATA in the current protocol state` — four times in six minutes of ordinary typing, with sshd logging the disconnect it RECEIVED from us. `process()` is left alone for events that have side effects: for those, "queued" is genuinely not "done". Measured against a real sshd, driving the library directly: a bench of 8 cases went from 2 failures to 0, twice in a row. The same `== PROCESSED` comparison exists in `SshChannelStateMachine.kt:503` and `SftpStateMachine.kt:471` and is not addressed here.
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.
The bug
SshClientStateMachine.process()compares against a single outcome:processEventhas three.IGNOREDmeans no transition matched — that is a refusal.PENDINGmeansanother event was being processed and this one was queued, which is what KStateMachine's default
queuePendingEventHandlerdoes (confirmed in the 0.38.1 bytecode:StateMachineImplinitialisespendingEventHandlerwithqueuePendingEventHandler).So "queued" is read as "refused". For the authorization guards that
falsereachesrequireAccepted, which raisesProtocolViolationException— and a protocol violation tears downthe whole transport, not just a channel.
The window opens whenever the packet loop reads a packet while a locally-initiated event has
suspended to write to the socket:
OpenChannel,SendChannelRequest, a window-change. In aninteractive session that is constantly.
How it shows up
An Android SSH client on 0.4.1, ordinary typing on a LAN, sshd at
LogLevel DEBUG3— fourdisconnects in six minutes, all client-initiated:
Terminal output is what the connection exists to carry, so there is nothing to throttle. It also
takes SFTP, port forwards and everything else multiplexed on that transport down with it.
The fix
The authorization guards have no effect of their own — they only ask whether a packet fits the
current state — so
PENDINGis a fine answer for them: the event will be processed. OnlyIGNOREDis a refusal.
process()is deliberately left alone for events that do have side effects (OpenChannel,SendChannelRequest, …): for those, "queued" genuinely is not "done".Measured
Against a real sshd, driving the library directly (8 cases: large writes, channel churn, rekey at
2s, output floods, and combinations):
Both baseline failures raise exactly the exception above.
Not addressed here
The same
== PROCESSEDcomparison exists inSshChannelStateMachine.kt:503andSftpStateMachine.kt:471. I have not looked at whether they are reachable in the same way.