ssh: flush the worker's queued output and report what it is waiting on

[yosuke-wolfssl]

Problem

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

A channel read credits the window, ChannelCreditWindow() bundles a
CHANNEL_WINDOW_ADJUST into ssh->outputBuffer, and the socket write blocks.
The credit is deliberately not re-parked — the packet is already encrypted and
sequenced, so re-sending it would inflate the peer's window — which leaves
wolfSSH_SendPacket() as the only thing that can discharge it.

The peer has spent its window and goes silent waiting for that adjust. The
application has nothing of its own to send, so it calls wolfSSH_worker(),
which is what wolfssh/ssh.h tells it to do. The worker gated its flush on
DoReceive()'s return value, and on an idle socket DoReceive() returns
WS_FATAL_ERROR — not one of the gated values. No write was attempted, on
that call or any later one.

Fix (src/ssh.c)

• wolfSSH_worker() flushes whenever ssh->outputBuffer holds bytes,
  instead of only for WS_SUCCESS/WS_WANT_READ/WS_CHAN_RXD.
• It reports a specific status. DoReceive()'s WS_FATAL_ERROR is
  replaced with ssh->error when that holds a real code, so a caller can tell
  an idle socket from a decrypt or MAC failure.
• The return describes the receive. An owed flush is state, not an event,
  so it stays in ssh->error; only a send failure other than WS_WANT_WRITE
  reaches the return. No snapshot-and-restore of ssh->error is involved.
• Drops the nested second DoReceive(), its WS_WINDOW_FULL case, and the
  WOLFSSH_TEST_BLOCK fork of the function body.
• GetInputData() returns its condition and DoReceive() records it;
  _ChannelRead(), _ChannelReadExt() and
  SendPendingChannelWindowAdjust() lose their savedError dances.

Callers (apps/, examples/)

• FlushQueuedSend() loops while wolfSSH_get_error() reports an owed flush
  within its deadline, and returns WS_WANT_WRITE if the queue is still out.
• server_worker() and HandleConnection() treat WS_CHANNEL_CLOSED from
  wolfSSH_shutdown() as a clean end alongside WS_SOCKET_ERROR_E; wolfsshd's
  graceful-shutdown drain keeps looping on WS_EXTDATA.
• client, scpclient, sftpclient and the wolfssh app gate on return
  values; 12 now-dead WS_FATAL_ERROR translations are removed.

The contract this establishes

Function shape	How status is reported
Returns a byte count (stream_read, SendChannelData, ...)	the return is data, so status goes in ssh->error
Returns a status (wolfSSH_worker, wolfSSH_shutdown, ...)	the return is the status; no second channel

wolfSSH_worker() and wolfSSH_shutdown() move to the second row.
SendChannelData() and SendChannelExtendedData() are made to satisfy the
first properly, recording from a captured local rather than reading a possibly
stale ssh->error, and marking the owed flush when the flush of
already-queued output blocks — so the next call reports the block instead of
acknowledging a payload it never bundled.

Two consequences to be aware of when integrating:

• wolfSSH_worker() no longer returns WS_WANT_WRITE for an owed flush, and
  wolfSSH_get_error() after such a call reports WS_WANT_WRITE rather than
  the receive's status. Read the return for what happened; read
  wolfSSH_get_error() to ask whether a flush is still owed.
• When the receive and the flush both fail hard on the same call, the flush's
  error is what reaches the caller; the receive error is not preserved.

Intentionally not in this PR

wolfSSH_SendPacket() keeps writing ssh->error. Removing it was tried
and reverted. It has 57 call sites; most hand the status onward without
recording it, and consumers several layers up read the field. Removing the
producer before its consumers deadlocked the SFTP suite under forced
write-blocking. Consumer-first is safe at every step; producer-first is not.

Core public APIs still return generic WS_FATAL_ERROR. wolfSSH_accept(),
wolfSSH_connect() and around ten others put the detail in ssh->error.
Migrating them is caller-visible for every integrator and wants its own audit.

Example application loops are unchanged. An app that selects only for readability
with no timeout still never calls wolfSSH_worker() once the peer goes silent,
so the flush gets no opportunity to run. examples/echoserver and
examples/client are in that shape. Correcting them requires rework on app side,
in different branch.

A follow-up PR covers these: ~198 WS_FATAL_ERROR collapse points in
src/wolfsftp.c, then its readers, then deleting the SendPacket write last.
Also, examples/echoserver and examples/client would be covered by
another PR.

Verification

• tests/unit.test 119/0; regress, api, auth, kex, testsuite pass.
• make check clean apart from sftp.test/scp.test, which flake under
  parallel make check and pass serially — same as on master.
• Contention matrix (-DWOLFSSH_TEST_BLOCK, WOLFSSH_BLOCK_PROB 70/50/30):
  sftp.test passes at all three, as does a three-client sequential transfer
  (1KB/2MB/10MB) against a single non-blocking echoserver.
• gcc-13 -Werror clean across 6 configurations.

Known coverage gap: scp.test skips entirely in a WOLFSSH_TEST_BLOCK
build ("wolfscp client does not support non-blocking mode"), so SCP is affected
by these changes but unverified under contention.

[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 #1213

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 4
4 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 #1213

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

Required changes (1)

scpclient still fails the run on the want it just declared benign

File: examples/scpclient/scpclient.c:350
Function: scp_client
Category: Incorrect error handling

Line 329 was changed to accept WS_WANT_READ/WS_WANT_WRITE from the shutdown drain, but the same ret reaches line 350, which sets return_code = 1 for anything other than WS_SUCCESS/WS_CHANNEL_CLOSED, so a benign want still makes the client exit non-zero. examples/client/client.c got both checks updated; this one only got the first.

Recommendation: Exclude WS_WANT_READ and WS_WANT_WRITE in the final return_code check and in the log guard at line 339.

Referenced code: examples/scpclient/scpclient.c:350-351 (2 lines)

---

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

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1213

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 5
5 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]

Unconditional flush overwrites a fatal receive error in ssh-error with WS_WANT_WRITE · Incorrect error handling

The flush now runs regardless of DoReceive()'s result, so wolfSSH_SendPacket() replaces a fatal receive code (WS_DECRYPT_E, WS_VERIFY_MAC_E, WS_OVERFLOW_E) in ssh->error with WS_WANT_WRITE. Callers dispatching on wolfSSH_get_error() — wolfsshd.c:930 and :1510, echoserver.c:1431 — treat it as transient and keep driving a dead session.

Related known finding #11638 (similar but distinct): Both involve DoReceive status handling in src/ssh.c, but #11638 returns WS_EXTDATA incorrectly from wolfSSH_stream_read, while this overwrites fatal receive state during wolfSSH_worker flushing. The operations, root causes, and required patches differ.

Fix: Restore the hard receive status into ssh->error after the flush when ret is a fatal code, so only the return carries the owed-flush marker.

[wolfSSL-Fenrir-bot]

Shutdown drain is skipped when the owed flush lives only in ssh-error · Logic errors

wolfSSH_worker() no longer returns WS_WANT_WRITE for an owed flush, so wolfSSH_shutdown() returns WS_SUCCESS (via its WS_CHAN_RXD remap at ssh.c:1200) while teardown output is still queued. The ret-only gate then skips the drain and the socket closes with those packets unsent. Same at echoserver.c:1647 and sftpclient.c:1794.

Fix: Also enter the drain when wolfSSH_get_error(ssh) == WS_WANT_WRITE, at all three shutdown sites.

[wolfSSL-Fenrir-bot]

wolfSSH_worker() overwrites the owed-flush marker in ssh-error with WS_REKEYING · Channel handling errors

The isKeying block runs after the flush and unconditionally writes ssh->error = WS_REKEYING, destroying the WS_WANT_WRITE that wolfSSH_SendPacket() just stored for a blocked flush. Since ssh->error is the only owed-flush indicator under the new contract, FlushQueuedSend() (apps/wolfssh/wolfssh.c:337,344) exits its retry loop and returns WS_SUCCESS while ssh->outputBuffer still holds unsent rekey bytes.

Fix: Only set ssh->error = WS_REKEYING when sendRet != WS_WANT_WRITE, leaving the owed-flush marker intact otherwise.

[wolfSSL-Fenrir-bot]

Graceful-shutdown drain loop is skipped when only a flush is owed · Channel handling errors

The drain gate now reads wolfSSH_shutdown()'s return, but the PR's contract puts an owed flush in ssh->error, not the return. When the peer's CHANNEL_CLOSE arrives during shutdown (return WS_CHANNEL_CLOSED, mapped to 0) with a blocked write, the loop is skipped and wolfSSH_free() discards the queued EOF/CLOSE/window-adjust bytes; the previous wolfSSH_get_error() gate drained them.

Fix: Also enter the drain loop when wolfSSH_get_error(ssh) == WS_WANT_WRITE, and apply the same at echoserver.c:1647 and sftpclient.c:1794.

[wolfSSL-Fenrir-bot]

sftp_worker pre-select flush loop becomes an unbounded busy-wait · Channel handling errors

wolfSSH_worker() now leaves ssh->error == WS_WANT_WRITE for as long as the flush is owed, so this loop — which has no select(), sleep, or iteration bound — spins at 100% CPU until the peer's socket drains, and ignores any channel data received meanwhile. Previously the worker left ssh->error at WS_WANT_READ, so the loop exited after one pass.

Related known finding #10544 (similar but distinct): Both findings affect echoserver worker-side SSH I/O handling, but #10544 is ssh_worker’s loss of unsent tails after partial writes, whereas this is sftp_worker’s unbounded retry loop on an owed flush; the faulting operations, root causes, and required patches differ.

Fix: Bound the loop with a tcp_select() on writability (or an attempt cap) before each wolfSSH_worker() call.