Skip to content

fix(state machine): PENDING is queued, not refused — don't tear down the transport - #269

Open
wdrs-dev wants to merge 1 commit into
connectbot:mainfrom
wdrs-dev:fix/pending-is-not-refused
Open

fix(state machine): PENDING is queued, not refused — don't tear down the transport#269
wdrs-dev wants to merge 1 commit into
connectbot:mainfrom
wdrs-dev:fix/pending-is-not-refused

Conversation

@wdrs-dev

Copy link
Copy Markdown

The bug

SshClientStateMachine.process() compares against a single outcome:

private suspend fun process(event: SshEvent): Boolean =
    stateMachine.processEvent(event) == ProcessingResult.PROCESSED

processEvent has three. IGNORED means no transition matched — that is a refusal. PENDING means
another event was being processed and this one was queued, which is what KStateMachine's default
queuePendingEventHandler does (confirmed in the 0.38.1 bytecode: StateMachineImpl initialises
pendingEventHandler with queuePendingEventHandler).

So "queued" is read as "refused". For the authorization guards that false reaches
requireAccepted, which raises ProtocolViolationException — 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.

How it shows up

An Android SSH client on 0.4.1, ordinary typing on a LAN, sshd at LogLevel DEBUG3 — four
disconnects in six minutes, all client-initiated:

Received disconnect from <ip> port 40252:2: Unexpected SSH packet SSH_MSG_CHANNEL_DATA in the current protocol state

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 PENDING is a fine answer for them: the event will be processed. Only IGNORED
is 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):

sshlib tests failures
0.4.1 8 2
0.4.1 + this patch 8 0
0.4.1 + this patch (again) 8 0

Both baseline failures raise exactly the exception above.

Not addressed here

The same == PROCESSED comparison exists in SshChannelStateMachine.kt:503 and
SftpStateMachine.kt:471. I have not looked at whether they are reachable in the same way.

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

1 participant