ssh, internal: flush the worker's queued output on every call

[yosuke-wolfssl]

Problem

A read-only application on a non-blocking socket stalls permanently.

A channel read credits the window, ChannelCreditWindow() bundles a CHANNEL_WINDOW_ADJUST into ssh->outputBuffer, and the socket write blocks. The credit is not re-parked — the packet is already encrypted and sequenced — so wolfSSH_SendPacket() is the only thing that can discharge it. The peer has spent its window and goes silent waiting for that adjust.

The application calls wolfSSH_worker(), as wolfssh/ssh.h directs. The worker gated its flush on DoReceive()'s return, and an idle socket makes DoReceive() return WS_FATAL_ERROR — not one of the gated values. No write is attempted, on that call or any later one. The gate also listed WS_WANT_READ, which DoReceive() never returns; that dead arm is the bug.

The fix (src/ssh.c)

wolfSSH_worker() flushes whenever output is queued and the session is live:

if (ssh != NULL && !ssh->disconnected && ssh->outputBuffer.length != 0) {

• !ssh->disconnected keeps bytes from going out after a DISCONNECT (RFC 4253 §11.1), which the old gate excluded only by accident.
• ssh->error keeps the receive's code when the receive itself failed, and the close's code when a WS_CHANNEL_CLOSED pass hard-failed its flush. Every other status keeps the code the send set.
• The WS_REKEYING report is skipped when the flush failed, so a dead transport is not reported as a rekey to drive.
• Removes the second DoReceive(), its WS_WINDOW_FULL arm, the WOLFSSH_TEST_BLOCK ordering fork, and the separate WS_CHANNEL_CLOSED flush — all four only existed to work around the gate.

Why the gate ignores ret entirely. An idle receive and a hard one both surface as WS_FATAL_ERROR, so a narrower gate would have to key on ssh->error == WS_WANT_READ. Flushing after a MAC or decrypt failure sends only our own already-framed bytes on a session being torn down, and gating on the idle case would reintroduce the class of bug this fixes: a status nobody thought to list stops the flush. That is how the original WS_WANT_READ arm went dead.

ssh->error discipline (src/internal.c, src/ssh.c)

SendPacketFlush() records its code in ssh->error on every transport failure path, not only WS_WANT_WRITE. Otherwise a hard send failure during the flush left the idle receive's WS_WANT_READ in place, and callers routing on wolfSSH_get_error() would select for read on a dead socket.

That cuts both ways, and both are now stated on wolfSSH_SendPacket():

• A later write to ssh->error on the same pass must be conditional on the flush having succeeded. Four writers satisfy it — wolfSSH_shutdown()'s flushRet == WS_SUCCESS, the rekey mask's sendRet == WS_SUCCESS, the worker's rxErr precedence, and SendChannelData()'s writes nested under ret == WS_SUCCESS.
• Success is never written into ssh->error. wolfSSH_TriggerKeyExchange() did, and it runs from HighwaterCheck() inside wolfSSH_SendPacket(), so a successful flush could zero the field for any of its callers. Fixed at the source rather than in the one caller that noticed.

_ChannelRead() and _ChannelReadExt() still record the adjust's code themselves: ChannelCreditWindow() can fail with WS_BAD_ARGUMENT or WS_OVERFLOW_E before reaching the transport, where SendPacketFlush() never runs.

wolfSSH_worker(), wolfSSH_ChannelSendEof() and wolfSSH_stream_read() doc blocks in ssh.h state the rule; WS_WINDOW_FULL comes off the worker's return list, since no path reaches it. No public API change.

Consumers

Three shell loops treat anything but WS_WANT_READ as fatal, one line each:

File	Line
examples/echoserver/echoserver.c	1205
apps/wolfsshd/wolfsshd.c (WIN32)	1364
ide/Espressif/.../echoserver.c	1170

This is a pre-existing teardown, not a regression introduced here. Master's wolfSSH_worker() already returns WS_WANT_WRITE — the second DoReceive() path ends in ret = sendRet — and master's SendPacketFlush() already records it in ssh->error. So on master these loops already drop a live session whenever the send buffer fills while the peer is still sending, which is an ordinary bulk upload. This change makes that easier to reach, by flushing on passes the old gate skipped, so tolerating the status belongs with it. Everything beyond that is caller-loop work and is deferred; see below.

Tests

Fourteen new unit tests cover the flush on an idle receive, the owed flush across calls, and what ret and ssh->error hold after a receive failure, a hard send failure, a discarded buffer, an out-of-bounds send, a missing send callback and a bad buffer state — each alongside channel data, extended data, a half-close, a rekey, or a channel close. test_TriggerKeyExchangeKeepsError() covers the rekey trigger, and TestWorkerReportsDisconnect covers queued output staying unsent on the disconnect pass.

The #ifndef WOLFSSH_TEST_BLOCK guards around TestWorkerReadsWhenSendWouldBlock are gone — the send-first fork they worked around no longer exists, so it runs in every configuration.

One arm is knowingly uncovered: a DoReceive() that returns a plain WS_SUCCESS with output queued and a failing flush, where ret = sendRet. Reaching it needs a builder for a non-channel packet that nothing else in the suite uses.

Verification

• unit.test 157 passed / 0 failed; regress.test passed.
• Clean under gcc-13 -Werror across 6 configurations, plus lint.
• Every ssh->error write is backed by a negative control: reverting it makes a named test fail, and no other.
• Network contention (-DWOLFSSH_TEST_BLOCK, scripts/sftp.test), the harness whose ordering fork this removes:

WOLFSSH_BLOCK_PROB	Result	Time
70	pass	61s
50	pass	33s
30	pass	13s

scp.test and get-put.test exit 77 (skip) under WOLFSSH_TEST_BLOCK by their own design, so sftp.test is the only script test that reaches this path.

Known limitations, not addressed here

Five caller loops mishandle an owed flush. None is made worse by this PR; all predate it. They are deferred together because none has a test that can verify a fix — scripts/sftp.test drives sftp_worker(), and nothing drives a shell session under -DWOLFSSH_TEST_BLOCK.

File	Loop	Fault
examples/echoserver/echoserver.c:806	ssh_worker()	select() at :999 watches read fds with a NULL timeout
ide/Espressif/.../echoserver.c:794	fork of the above	select() at :984, identical
apps/wolfsshd/wolfsshd.c:1364	WIN32 shell loop	skips its select() at :1300 when wolfSSH_stream_peek() has data
examples/echoserver/echoserver.c:1496	sftp_worker() handshake retry	retries with no wait at all
apps/wolfsshd/wolfsshd.c:1131	WIN32 window-change drain	retries with no wait at all

apps/wolfsshd/wolfsshd.c:1942 already has the shape wanted, in the POSIX loop: one select per iteration watching read and write together, driven by a wantWrite flag.

An earlier revision fixed the last two with a tcp_select_write() helper. That was pulled out: fixing two of five left the class half-done and grew the diff without closing the stall, and the helper belongs with the loops it was built for. The follow-up writes the shell-session harness first, then fixes all five.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1217

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1217

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1217

Scan targets checked: wolfssh-bugs, wolfssh-src

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

Fenrir's latest completed scan found no issues; clearing the prior automated change request.

[ejohnstown]

The deleted WS_CHANNEL_CLOSED block also protected the close status in ssh->error, and that became load-bearing once the other half of this PR made SendPacketFlush() write ssh->error on hard failures too (internal.c:5008).

DoChannelClose() returns WS_CHANNEL_CLOSED even on a short send (internal.c:12119-12129), leaving bytes queued. If the flush then hard-fails, ret is WS_CHANNEL_CLOSED — neither WS_SUCCESS nor WS_FATAL_ERROR — so neither arm fires and WS_SOCKET_ERROR_E stands. Master left the latched close alone here. Since every drive loop dispatches on wolfSSH_get_error(), wolfsshd.c:2032's orderly arm is skipped and :2058 SIGKILLs the child instead.

Minimal fix, keeping the deliberate supersede behavior for WS_CHAN_RXD / WS_EOF / WS_EXTDATA:

  else if (ret == WS_CHANNEL_CLOSED
          || (ret == WS_FATAL_ERROR && rxErr != WS_WANT_READ)) {

DoChannelCloseFlushesReply doesn't catch this — with s_sendRefusals = 2 its flush succeeds, so the assertion passes because nothing wrote ssh->error, not because anything preserved it. A variant that refuses both DoChannelClose() sends and then hard-fails the flush with ConnResetIoSend would be the control.

[yosuke-wolfssl]

Thanks — It was real and I had the same wrong instinct you did, that the
deleted block was equivalent to master. It isn't, and the SIGKILL
fall-through is the part that makes it matter.

One correction: the suggested condition breaks DoChannelCloseWantWrite.
Applied as written it restores the close over any flush failure including
back-pressure, so ssh->error reads WS_CHANNEL_CLOSED where that test
asserts WS_WANT_WRITE — 148 pass / 1 fail. fe936c8b's own "a short flush
leaves WS_WANT_WRITE latched" is what it collides with. One extra term keeps
both:

else if ((ret == WS_CHANNEL_CLOSED && sendRet != WS_WANT_WRITE)
        || (ret == WS_FATAL_ERROR && rxErr != WS_WANT_READ)) {

test_DoChannelCloseHardFlushKeepsClose is the control you asked for, with a
RefuseThenResetIoSend mock: both DoChannelClose() sends refused, then the
flush hard-fails. It fails without the arm.

I also took some issues addressed by the report.
F5 (ssh.h precondition), F6 (Espressif copy), F8's blank line, and
the comment now states the rule structurally rather than listing three of five
statuses — which is how F1 got past me. Subject is down to 47 characters.

F2 is paced rather than capped. A cap changes termination: the loop exits with
the flush still owed, ret is WS_FATAL_ERROR not WS_WANT_WRITE, and
wolfSSH_SFTP_PendingSend() reports recvState->toSend, not
ssh->outputBuffer — so the loop below doesn't pick it up and a silent peer
stalls. Pacing fixes the CPU without touching termination. Note tcp_select()
passes NULL for writefds, so it can only pace, not wait for writability; the
comment says so.

F3 I've left out. It's Windows-only, windows-sftp.yml drives the SFTP
subsystem rather than the shell path, and a busy-wait that terminates fails no
assertion — so neither CI nor a local MSVC build would catch a mistake there.
Following up with a test that reaches the loop first.

F7: reasoning recorded in the PR description. F4 and the F8 boilerplate
refactor deferred.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1217

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1217

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1217

Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 2
1 finding(s) posted as inline comments (see file-level comments below)

Required changes (1)

wolfsshd WIN32 forced-command drain loop becomes an unbounded CPU spin

File: apps/wolfsshd/wolfsshd.c:1131
Function: SHELL_Subsystem
Category: Resource leaks

wolfSSH_worker() now leaves ssh->error == WS_WANT_WRITE on an idle receive with queued output (src/ssh.c:3731-3746), so this unpaced loop never terminates while the peer stops reading the non-blocking socket (set at wolfsshd.c:3412) — a remote 100% CPU spin per session.

Recommendation: Wait for socket writability with a bounded select/timeout between passes and give up after a cap instead of looping on WS_WANT_WRITE.

Referenced code: apps/wolfsshd/wolfsshd.c:1131-1135 (5 lines)

---

This review was generated automatically by Fenrir. Reported findings require changes before merge.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1217

Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 1

Required changes (1)

wolfsshd Windows drain loop becomes a delay-free busy-wait on the worker's new WS_WANT_WRITE

File: apps/wolfsshd/wolfsshd.c:1131
Function: SHELL_Subsystem
Category: Incorrect error handling

wolfSSH_worker() now leaves ssh->error == WS_WANT_WRITE when an idle receive is followed by a blocked flush (previously the removed second DoReceive() left WS_WANT_READ). This drain loop has no select(), sleep, or iteration cap, so a peer that stops reading pins a CPU core on the non-blocking connection socket for as long as it keeps the socket unwritable.

Recommendation: Wait for write readiness with select() on the write set, or bound the loop with a deadline and a pause between retries.

Referenced code: apps/wolfsshd/wolfsshd.c:1131-1136 (6 lines)

---

This review was generated automatically by Fenrir. Reported findings require changes before merge.

[yosuke-wolfssl]

Note for last Fenrir-bot comments and reviewers:

Confirmed, and deferred deliberately.

The mechanism is as described: that drain has no select(), sleep or cap, and
wolfSSH_worker() now leaves WS_WANT_WRITE where the removed second
DoReceive() left WS_WANT_READ. The shell loop at :1364 has the same
problem for a different reason — it skips its select() whenever
wolfSSH_stream_peek() has buffered data.

Both are folded into one follow-up with examples/echoserver's ssh_worker()
and the ESP-IDF copy, which need the same write-set rework.
apps/wolfsshd/wolfsshd.c:1942-1945 is the model: one select per iteration
watching read and write together, driven by the wantWrite flag the worker's
status sets.

Not fixing it in this PR because nothing can verify it. windows-sftp.yml
drives the SFTP subsystem, never SHELL_Subsystem()'s shell path, and a
busy-wait that terminates fails no assertion — so neither CI nor a local MSVC
build would catch a mistake, only a compile break. An earlier blind fix to this
same function introduced a spin that review caught. The follow-up writes the
shell-session harness first, then the fix.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1217

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1217

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1217

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.