ssh, internal: flush the worker's queued output on every call by yosuke-wolfssl · Pull Request #1217 · wolfSSL/wolfssh

yosuke-wolfssl · 2026-08-31T05:55:20Z

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, so wolfSSH_SendPacket() is the only thing that can discharge it, and the peer has spent its window and goes silent waiting for it. The application calls wolfSSH_worker(), as wolfssh/ssh.h directs — but the worker gated its flush on DoReceive()'s return, and an idle socket makes DoReceive() return WS_FATAL_ERROR, which the gate did not list. No write is attempted, on that call or any later one.

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->error keeps the receive's code when the receive failed, and the close's when a WS_CHANNEL_CLOSED pass hard-failed its flush. Every other status keeps the send's.
WS_REKEYING is withheld 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 existed to work around the gate.

The gate ignores ret deliberately. An idle receive and a hard one both surface as WS_FATAL_ERROR, so a narrower gate would key on ssh->error == WS_WANT_READ and reintroduce this same class of bug: a status nobody thought to list stops the flush.

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

SendPacketFlush() now records its code on every transport failure path, not only WS_WANT_WRITE — otherwise a hard send failure left the idle receive's WS_WANT_READ in place and callers would select for read on a dead socket. Two rules follow, both stated on wolfSSH_SendPacket(): a later write to ssh->error on the same pass must be conditional on the flush having succeeded, and success is never written into the field. wolfSSH_TriggerKeyExchange() violated the second, and runs from HighwaterCheck() inside wolfSSH_SendPacket(), so a clean flush could zero the field for any caller.

What the wider flush exposed

Flushing on every pass reaches two places the old gate shielded:

BundlePacket() left outputBuffer.length past packetStartIdx when framing failed, so an unpadded, un-MAC'd, unencrypted partial packet stayed queued. The old gate excluded WS_FATAL_ERROR, so nothing went out; the new one would have put it on the wire. Its failure arm now drops that packet alone.
wolfSSH_shutdown() mapped WS_CHAN_RXD and WS_EOF from its close-read to WS_SUCCESS. If the receive highwater fires on that read it queues a KEXINIT, and a blocked socket left the teardown reporting success with bytes owed. It now reports WS_WANT_WRITE, which ssh.h already documents for the call.

Consumers

Three shell loops treated anything but WS_WANT_READ as fatal — examples/echoserver/echoserver.c:1205, apps/wolfsshd/wolfsshd.c:1364, ide/Espressif/.../echoserver.c:1170 — one line each. For these three the teardown is pre-existing: master's worker already returns WS_WANT_WRITE via its second DoReceive() path, and master's SendPacketFlush() already records it, so they already drop a live session when the send buffer fills mid-transfer. This change only makes it easier to reach.

ssh.h states the worker's contract and its two exceptions; WS_WINDOW_FULL comes off the return list, since no path reaches it. No public API change.

Tests

Sixteen 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, send, buffer, callback or framing failure — each against channel data, extended data, a half-close, a rekey, or a channel close. One arm is knowingly uncovered: a DoReceive() returning plain WS_SUCCESS with output queued, which needs a non-channel packet builder nothing else uses.

Verification

unit.test 159 passed / 0 failed; regress.test passed. Clean under gcc-13 -Werror across 6 configurations, plus lint.
Every fix has a negative control: reverting it fails its own test and no other.
Network contention (-DWOLFSSH_TEST_BLOCK, scripts/sftp.test) passes at WOLFSSH_BLOCK_PROB 70/50/30 in 61s/33s/13s. scp.test and get-put.test skip under that macro by their own design.

Known limitations, not addressed here

Five caller loops mishandle an owed flush, and they are deferred together because none has a test that can verify a fix — nothing drives a shell session under -DWOLFSSH_TEST_BLOCK.

File	Fault
`examples/echoserver/echoserver.c:806`	`select()` at `:999` watches read fds, `NULL` timeout
`ide/Espressif/.../echoserver.c:794`	same, `select()` at `:984`
`apps/wolfsshd/wolfsshd.c:1364`	skips its `select()` when `wolfSSH_stream_peek()` has data
`examples/echoserver/echoserver.c:1496`	retries with no wait at all
`apps/wolfsshd/wolfsshd.c:1131`	retries with no wait at all

The last two are a regression this PR creates, not one it inherits. On master an idle receive attempts no flush, so ssh->error is WS_WANT_READ and both loops exit after one pass; here the flush runs every pass and re-sets WS_WANT_WRITE, so they spin until the peer's window opens. One is wolfsshd, which ships.

An earlier revision fixed those two with a tcp_select_write() helper. It was pulled out: fixing two of five left the class half-done and grew the diff without closing the stall. 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.

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: 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 · 2026-09-02T22:55:37Z

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.

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.

- The echoserver and Espressif shell loops and the Windows wolfsshd shell loop treat a WS_WANT_WRITE from wolfSSH_worker() as non-fatal.

- wolfSSH_TriggerKeyExchange() writes ssh->error only when SendKexInit() fails. It runs from HighwaterCheck() inside wolfSSH_SendPacket(), so writing WS_SUCCESS there erased what the pass the mark fired on had already reported. - test_TriggerKeyExchangeKeepsError() seeds ssh->error and checks a rekey that starts cleanly leaves it alone.

- wolfSSH_worker() calls wolfSSH_SendPacket() whenever ssh->outputBuffer holds bytes and the session is not disconnected, in place of doing so only for WS_SUCCESS, WS_WANT_READ, WS_CHAN_RXD or WS_EOF. ssh->error keeps the receive's code when the receive failed, and the close's when a WS_CHANNEL_CLOSED pass hard-failed its flush; a function-scope sendRet also masks the WS_REKEYING report. Drops the second DoReceive(), its WS_WINDOW_FULL case, the WOLFSSH_TEST_BLOCK fork, and the separate WS_CHANNEL_CLOSED flush. - BundlePacket() resets ssh->outputBuffer.length to ssh->packetStartIdx when the framing fails, and wolfSSH_shutdown() reports WS_WANT_WRITE when its read for the peer's close leaves output queued. - SendPacketFlush() records its code in ssh->error on every transport failure path; wolfSSH_SendPacket() says so and what a later write to that field owes it. - wolfssh/ssh.h drops WS_WINDOW_FULL from wolfSSH_worker() and names the two exceptions to a status surviving a failed flush: a hard-failed WS_CHANNEL_CLOSED, and WS_REKEYING. Comments in ssh.c, unit.c, the echoserver, the Espressif copy and portfwd name the channel's own state or the failing call instead of restating a contract. - Sixteen unit tests and the extended TestWorkerReportsDisconnect cover what ret and ssh->error hold after a receive, send, buffer, callback or framing failure, against channel data, extended data, a half-close, a rekey, a close, and a teardown read. The #ifndef WOLFSSH_TEST_BLOCK guards around TestWorkerReadsWhenSendWouldBlock go with the send-first fork.

- tcp_select_write() joins tcp_select(), with WS_SELECT_SEND_READY at the end of the enum. tcp_select() passes NULL for writefds and cannot wait on the write side. - The Windows wolfsshd window-change drain and sftp_worker()'s handshake flush retry wait on it. The drain records a give-up in ret, and sftp_worker() breaks only on WS_SELECT_ERROR_READY. - The echoserver, Espressif and Windows wolfsshd shell loops take the event from wolfSSH_worker()'s return when it carries one, in place of reading ssh->error alone. The two echoservers cover WS_CHAN_RXD, WS_REKEYING, WS_CHANNEL_CLOSED and WS_EOF; wolfsshd covers the three it has arms for.

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #1217

Scan targets checked: wolfssh-src, wolfssh-bugs

Findings: 3
3 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 · 2026-09-10T08:20:29Z

                        continue;
                    }
-                    else if (rc != WS_WANT_READ) {
+                    else if (rc != WS_WANT_READ && rc != WS_WANT_WRITE) {


Nonblocking echoserver stalls on queued output · Logic errors

WS_WANT_WRITE now keeps the loop alive, but select() monitors only readFds. Once a queued flush blocks and the peer is silent, wolfSSH_worker() is never retried and the session stalls.

Related known finding #10544 (similar but distinct): Both are in echoserver ssh_worker and concern nonblocking output progress, but #10544 loses unwritten tails after partial write operations; this candidate fails to wait for socket writability after WS_WANT_WRITE. The root causes and required patches differ.

Suggested fix: When the worker reports WS_WANT_WRITE, monitor sshFd in writefds and invoke the worker when it becomes writable.
Basis: POSIX select(): writefds identifies file descriptors ready for writing.

wolfSSL-Fenrir-bot · 2026-09-10T08:20:29Z

                        continue;
                    }
-                    else if (rc != WS_WANT_READ) {
+                    else if (rc != WS_WANT_READ && rc != WS_WANT_WRITE) {


ESP-IDF echoserver stalls on queued output · Logic errors

WS_WANT_WRITE now keeps the loop alive, but select() monitors only readFds. Once a queued flush blocks and the peer is silent, wolfSSH_worker() is never retried and the session stalls.

Related known finding #10544 (similar but distinct): Both concern nonblocking output handling in an echoserver ssh_worker, but #10544 mishandles partial successful writes in the main example while this candidate omits write readiness monitoring in the ESP-IDF port. They are different operations, files, and fixes.

Suggested fix: When the worker reports WS_WANT_WRITE, monitor sshFd in writefds and invoke the worker when it becomes writable.
Basis: lwIP sockets select(): writefds reports sockets ready for writing.

wolfSSL-Fenrir-bot · 2026-09-10T08:20:29Z

                        continue;
                    }
-                    else if (rc != WS_WANT_READ) {
+                    else if (rc != WS_WANT_READ && rc != WS_WANT_WRITE) {


Windows wolfsshd shell stalls on queued output · Logic errors

WS_WANT_WRITE now keeps the shell loop alive, but its select() supplies only readFds, and timeout iterations do not call wolfSSH_worker(). A silent peer leaves queued protocol output unsent.

Suggested fix: Monitor sshFd for writability after WS_WANT_WRITE and call wolfSSH_worker() when the socket is write-ready.
Basis: Microsoft Winsock select(): writefds identifies sockets on which data can be sent.

yosuke-wolfssl self-assigned this Aug 31, 2026

Copilot AI lite review requested due to automatic review settings August 31, 2026 05:55

Copilot AI reviewed Aug 31, 2026

yosuke-wolfssl requested a review from wolfSSL-Fenrir-bot August 31, 2026 05:55

wolfSSL-Fenrir-bot previously requested changes Aug 31, 2026

View reviewed changes

Comment thread src/ssh.c Outdated

yosuke-wolfssl force-pushed the fix/worker-deadlock branch from b9d4cc3 to a650544 Compare August 31, 2026 07:02

yosuke-wolfssl requested a review from wolfSSL-Fenrir-bot August 31, 2026 07:03

yosuke-wolfssl commented Aug 31, 2026

View reviewed changes

Comment thread src/ssh.c Outdated

wolfSSL-Fenrir-bot previously requested changes Aug 31, 2026

View reviewed changes

Comment thread src/ssh.c

Comment thread apps/wolfsshd/wolfsshd.c

yosuke-wolfssl force-pushed the fix/worker-deadlock branch from a650544 to 7b1ec65 Compare August 31, 2026 23:13

yosuke-wolfssl commented Aug 31, 2026

View reviewed changes

Comment thread src/ssh.c

yosuke-wolfssl commented Aug 31, 2026

View reviewed changes

Comment thread apps/wolfsshd/wolfsshd.c

yosuke-wolfssl force-pushed the fix/worker-deadlock branch from 7b1ec65 to d95204d Compare September 1, 2026 00:42

yosuke-wolfssl requested a review from wolfSSL-Fenrir-bot September 1, 2026 00:47

wolfSSL-Fenrir-bot reviewed Sep 1, 2026

View reviewed changes

yosuke-wolfssl assigned wolfSSL-Bot and unassigned yosuke-wolfssl Sep 1, 2026

yosuke-wolfssl requested a review from ejohnstown September 1, 2026 01:25

yosuke-wolfssl force-pushed the fix/worker-deadlock branch from d95204d to b598ecc Compare September 1, 2026 06:49

ejohnstown requested changes Sep 1, 2026

View reviewed changes

Comment thread src/ssh.c Outdated

yosuke-wolfssl force-pushed the fix/worker-deadlock branch from b598ecc to e2839fa Compare September 2, 2026 03:04

yosuke-wolfssl requested a review from wolfSSL-Fenrir-bot September 2, 2026 03:05

wolfSSL-Fenrir-bot requested changes Sep 2, 2026

View reviewed changes

Comment thread src/ssh.c

yosuke-wolfssl force-pushed the fix/worker-deadlock branch from e2839fa to e5524ca Compare September 2, 2026 04:37

yosuke-wolfssl commented Sep 2, 2026

View reviewed changes

Comment thread src/ssh.c

yosuke-wolfssl requested a review from wolfSSL-Fenrir-bot September 2, 2026 04:38

wolfSSL-Fenrir-bot requested changes Sep 2, 2026

View reviewed changes

Comment thread apps/wolfsshd/wolfsshd.c

Comment thread examples/echoserver/echoserver.c Outdated

yosuke-wolfssl force-pushed the fix/worker-deadlock branch from e5524ca to d15c978 Compare September 2, 2026 07:30

wolfSSL-Fenrir-bot requested changes Sep 2, 2026

View reviewed changes

ejohnstown self-requested a review September 2, 2026 23:00

yosuke-wolfssl force-pushed the fix/worker-deadlock branch 2 times, most recently from fe68694 to 35f752f Compare September 3, 2026 01:57

yosuke-wolfssl requested a review from wolfSSL-Fenrir-bot September 3, 2026 01:58

wolfSSL-Fenrir-bot requested changes Sep 3, 2026

View reviewed changes

Comment thread tests/unit.c Outdated

Comment thread tests/unit.c

yosuke-wolfssl force-pushed the fix/worker-deadlock branch from 35f752f to cd5c5e8 Compare September 3, 2026 02:50

yosuke-wolfssl requested a review from wolfSSL-Fenrir-bot September 3, 2026 02:50

yosuke-wolfssl commented Sep 3, 2026

View reviewed changes

Comment thread tests/unit.c Outdated

yosuke-wolfssl commented Sep 3, 2026

View reviewed changes

Comment thread tests/unit.c

wolfSSL-Fenrir-bot requested changes Sep 3, 2026

View reviewed changes

Comment thread examples/echoserver/echoserver.c

yosuke-wolfssl commented Sep 3, 2026

View reviewed changes

Comment thread examples/echoserver/echoserver.c

ejohnstown assigned yosuke-wolfssl and unassigned wolfSSL-Bot Sep 3, 2026

yosuke-wolfssl force-pushed the fix/worker-deadlock branch from cd5c5e8 to f177bae Compare September 3, 2026 06:18

yosuke-wolfssl requested a review from wolfSSL-Fenrir-bot September 3, 2026 06:18

wolfSSL-Fenrir-bot requested changes Sep 3, 2026

View reviewed changes

Comment thread examples/echoserver/echoserver.c

yosuke-wolfssl commented Sep 3, 2026

View reviewed changes

Comment thread examples/echoserver/echoserver.c

yosuke-wolfssl force-pushed the fix/worker-deadlock branch from f177bae to c9f9857 Compare September 4, 2026 00:30

yosuke-wolfssl requested a review from wolfSSL-Fenrir-bot September 4, 2026 00:31

wolfSSL-Fenrir-bot requested changes Sep 4, 2026

View reviewed changes

Comment thread apps/wolfsshd/wolfsshd.c

yosuke-wolfssl commented Sep 4, 2026

View reviewed changes

Comment thread apps/wolfsshd/wolfsshd.c

yosuke-wolfssl added 4 commits September 10, 2026 09:49

apps, examples: tolerate a worker want-write

e8c1d34

- The echoserver and Espressif shell loops and the Windows wolfsshd shell loop treat a WS_WANT_WRITE from wolfSSH_worker() as non-fatal.

yosuke-wolfssl force-pushed the fix/worker-deadlock branch from c9f9857 to 9b6ccc6 Compare September 10, 2026 08:05

yosuke-wolfssl requested a review from wolfSSL-Fenrir-bot September 10, 2026 08:05

wolfSSL-Fenrir-bot requested changes Sep 10, 2026

View reviewed changes

Uh oh!

Conversation

yosuke-wolfssl commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

The fix (src/ssh.c)

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

What the wider flush exposed

Consumers

Tests

Verification

Known limitations, not addressed here

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #1217

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #1217

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #1217

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #1217

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #1217

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #1217

Required changes (1)

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

Uh oh!

yosuke-wolfssl commented Sep 2, 2026

Note for last Fenrir-bot comments and reviewers:

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #1217

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #1217

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #1217

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #1217

Uh oh!

yosuke-wolfssl commented Aug 31, 2026 •

edited

Loading

The fix (`src/ssh.c`)

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