wolfsshd: wait on the write side in the shell loop

[yosuke-wolfssl]

Problem

The WIN32 SHELL_Subsystem() loop in apps/wolfsshd/wolfsshd.c waited on the wrong
side of the socket: select() was passed NULL for writefds; the wait was skipped
entirely on passes holding child output; and rc was loop-carried, so a timed-out pass
could stop calling wolfSSH_worker() altogether, latching the backlog and hanging the
session.

Underneath both copies sat the same defect. windowFull recorded how many bytes a
send left behind but erased why, and the causes need different waits:

Cause	Unblocked by
WS_WANT_WRITE — queued in wolfSSH, socket refused	the socket becoming writable
WS_WINDOW_FULL / WS_REKEYING — peer credit spent	a window adjust arriving (a read)
short count — clamped by peerMaxPacketSz	nothing — send again now

SendChannelData() clamps to min(peerWindowSz, peerMaxPacketSz, maxPacketSz) and
returns only the count, so the last two are indistinguishable to the caller. Every
consumer of windowFull had to guess, and each guess was right for some causes and
wrong for others — a spin in one, a stall in another.

Closes 8cc2fa42e040bd60.

Fix (apps/wolfsshd/wolfsshd.c)

Record the state instead of inferring it:

typedef enum {
    SHELL_SEND_READY,    /* a packet-size clamp only; send again now        */
    SHELL_SEND_BLOCKED,  /* credit, rekey or the socket; wait on the socket */
    SHELL_SEND_NEVER     /* the channel can never take them; end the session */
} SHELL_SEND_STATE;

SHELL_BACKLOG binds that state to the byte count and the stream, replacing
windowFull and windowFullExt in both copies; SHELL_BacklogState() derives it from
the send result, wolfSSH_OutputPending(), and the channel's window and packet size,
so the loop waits, retries immediately, or gives up on evidence rather than a guess.

SHELL_SEND_NEVER matters because WS_WINDOW_FULL does not only mean "credit spent":
SendChannelData() also returns it when peerMaxPacketSz is zero, which is taken
verbatim from CHANNEL_OPEN and which no window adjust can ever clear. The packet size
is therefore tested before the window, which would otherwise mask it whenever both are
zero. A backlog on a retired channel is NEVER for the same reason — no adjust can
reach a channel that is gone.

Two invariants hold while a backlog is held: the loop never reads more child output
(both copies reuse shellBuffer, so reaching a read would overwrite unsent bytes), and
the child's descriptors stay out of the read set (an exited child's pipe is always
ready, so watching it would spin).

WIN32 additionally: select() runs every pass so no continue skips the wait, fd sets
are rebuilt per pass, pending no longer latches, the write set is passed as NULL
while empty since Winsock rejects an empty non-NULL set with WSAEINVAL, and inbound
data is read into its own channelBuffer rather than sharing shellBuffer with the
backlog, as the POSIX copy has always done. WS_REKEYING is handled in the
WS_CHAN_RXD arm rather than breaking the loop — the worker returns it in place of
WS_CHAN_RXD, so a separate arm would strand the keystroke it masked — and a
zero-length channel read no longer ends the session. processState is initialised, so a
break before GetExitCodeProcess() no longer reads an indeterminate value.

Verification

Preflight clean (lint plus six gcc-13 -Werror configs), each commit standalone so the
series bisects. MSVC Debug x64 wolfsshd.exe: 0 warnings, 0 errors, with the WIN32 path
confirmed present in the binary. unit, regress, testsuite and the four script
tests green; merges cleanly with #1251. skoll multi review,bugs,audit: 0 critical,
0 high. The POSIX sshd suite cannot run on macOS — setgroups() fails against
NGROUPS_MAX — so sshd-test.yml gates that half.

Not in this PR

• A shell back-pressure test. Nothing in the tree can hold a session in write
  back-pressure, so the new branches have no functional coverage.
• The exit-status policy — sending no status unless a terminated one was obtained
  (ee2918352885c95f). Pre-existing on the other break paths and wire-visible.
• The library-side fix. SHELL_BacklogState() reads peerWindowSz, which is
  internal. An accessor for a channel's send room would let a consumer read only what
  it can send, so no backlog would exist to classify — and the same bookkeeping could
  go from five other consumers. wolfSSH_ChannelGetSessionGranted() on master is the
  same pattern.

[Copilot]

🟡 Changes recommended

Two moderate issues remain unresolved regarding initial pending-output tracking and blocked-send retries.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This pull request updates SHELL_Subsystem() to improve queued-output and back-pressure handling on Windows and POSIX.

Changes:

• Tracks pending output after worker, read, and send operations.
• Rebuilds Windows descriptor sets with state-dependent waits.
• Separates channel-window handling from POSIX write readiness.

Review findings:

• Moderate (2 votes): seed wantWrite from wolfSSH_OutputPending(ssh) after the preflight worker call.
• Moderate (1 vote): retry queued output only when the socket is reported writable.

File summaries

File	Summary
apps/wolfsshd/wolfsshd.c	Updates Windows and POSIX shell-loop polling and output handling.

Review details

Suppressed comments (1)

apps/wolfsshd/wolfsshd.c:2300

• wantWrite means output is still owed, not that select() reported the socket writable. When the socket remains blocked and there is no inbound data, the 800-us timeout expires, but this condition still calls wolfSSH_worker() solely because wantWrite is carried from the previous pass; the worker retries the same blocked send about 1,250 times per second instead of waiting for writability. Gate this term on FD_ISSET(sshFd, &writeFds) while retaining windowFull/read handling for the other wake conditions.

            if (wantWrite || windowFull || pending
                    || FD_ISSET(sshFd, &readFds)) {

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1253

Scan targets checked: wolfssh-src, wolfssh-bugs

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 #1253

Scan targets checked: wolfssh-src, wolfssh-bugs

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

💬 1 finding(s) from an earlier review are still open and were not re-posted:

• POSIX window-full path still busy-spins on child readiness — apps/wolfsshd/wolfsshd.c:2951

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

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1253

Scan targets checked: wolfssh-src, wolfssh-bugs

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 #1253

Scan targets checked: wolfssh-src, wolfssh-bugs

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 #1253

Scan targets checked: wolfssh-src, wolfssh-bugs

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 #1253

Scan targets checked: wolfssh-src, wolfssh-bugs

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.

Review addressed.