ssh.c: don't fail a channel read whose window credit is deferred

[yosuke-wolfssl]

Problem

Two defects in the same three lines.

1. A read that copied bytes reports failure. _ChannelReadExt() already
separates "bytes were delivered" from "the window-adjust packet made it onto the
wire". The other two paths conflate them. _ChannelRead() — behind
wolfSSH_ChannelIdRead() and wolfSSH_ChannelRead() — consumes the bytes, then
returns the send result:

inputBuffer->idx += bufSz;                    /* bytes consumed */
updateResult = _UpdateChannelWindow(channel);
if (updateResult == WS_SUCCESS)
    updateResult = bufSz;
return updateResult;                          /* WS_WANT_WRITE -> "failed" */

_UpdateChannelWindow() returns WS_WANT_WRITE whenever the adjust would block —
routine on a non-blocking socket, not an error. The caller is told the read failed
after the data has already left the input buffer, so those bytes are gone, and a
caller using the usual if (cnt_r <= 0) break; shape tears the session down
(src/wolfscp.c, apps/wolfsshd/wolfsshd.c). SendPacket() sets ssh->error only
for WS_WANT_WRITE, so a hard transport failure during the adjust was invisible on
both paths.

2. A channel that never gets its window back. wolfSSH_stream_read() called
_UpdateChannelWindow() before advancing inputBuffer->idx, so it credited
bytesToAdd = inputBuffer->idx — the previous read's bytes. On the first read of a
session idx is 0, ChannelCreditWindow() returns early on total == 0, and no
SSH_MSG_CHANNEL_WINDOW_ADJUST is sent at all. If that read drained the window,
channel->windowSz stays 0 and neither side can move. Every later read credited one
read late. _ChannelRead() was never affected — it already advanced idx first.

Fix (src/ssh.c)

Both paths now report the bytes copied and keep the send result out of band, matching
_ChannelReadExt():

• _ChannelRead() returns bufSz unconditionally; wolfSSH_stream_read()
  advances inputBuffer->idx before the adjust, then returns n. That reordering is
  also the fix for defect 2.
• Both record a non-success adjust result in ssh->error and WLOG at
  WS_LOG_ERROR for anything other than WS_WANT_WRITE.
• _ChannelRead() also clears a stale WS_WANT_WRITE once the adjust does go out
  with the output buffer drained — its entry points, unlike wolfSSH_stream_read(),
  do not reset ssh->error.

A deferred credit still goes out, flushed by the next send or by wolfSSH_worker(),
with one caveat now stated in the header: wolfSSH_worker() flushes only when its
receive returned WS_SUCCESS, WS_WANT_READ or WS_CHAN_RXD, so a WS_EXTDATA or
WS_REKEYING turn does not.

Also (src/wolfsftp.c): wolfSSH_SFTP_Close()'s STATE_CLOSE_SEND arm was the
only NoticeError() check in the file ungated by ret. A parked WS_WANT_WRITE —
routine under the new contract — could make it return WS_FATAL_ERROR after a
successful send and re-send SSH_FXP_CLOSE on every application retry. Now gated like
its siblings.

API compatibility

wolfSSH_stream_read(), wolfSSH_ChannelRead() and wolfSSH_ChannelIdRead() now
return the byte count where a deferred or failed window adjust previously produced a
negative return; the adjust status moves to wolfSSH_get_error().
wolfSSH_ChannelReadExt() already had this shape (50ee1b61). All three prototypes
in wolfssh/ssh.h and the block comments in src/ssh.c document it. This needs a
ChangeLog.md line at release prep — not added here, since this repo only edits
ChangeLog.md at release.

One thing gets marginally worse: a hard transport failure during the adjust no longer
breaks the caller's read loop immediately. Every such loop calls wolfSSH_worker() on
the next iteration and fails there, so it is a one-iteration delay, not a lost error.

Tests (tests/unit.c)

One harness per path, each putting a full window of channel data and reading it back:

Phase	IO send	Asserts
Deferred credit	WS_CBIO_ERR_WANT_WRITE	byte count, payload, wolfSSH_get_error(), credited window, consumed buffer
Hard failure	WS_CBIO_ERR_GENERAL	byte count still returned, error is WS_SOCKET_ERROR_E, credit left owed
Clean credit	counting send (stream_read) / full send (ChannelIdRead)	one adjust reached the wire and the window is back / error retired to WS_SUCCESS, no credit owed

Verification

• unit.test, api.test, testsuite.test and scripts/{sftp,scp,fwd,get-put}.test
  all pass; unit.test also clean under ASan + UBSan.
• Clean under -Werror with gcc-13 across 6 configurations (enable-all, Zephyr
  defines, sftp-only, scp-only, default, small-stack).
• Negative controls: reverting the ssh->error recording fails both hard-failure
  assertions; reverting the stale-WS_WANT_WRITE clearing fails the retire assertion;
  restoring the old idx/credit ordering sends 0 window adjusts where the fix sends 1.

The echo server's worker loop has its own partial-write problems in this area. Those
are a separate rework PR that builds on this one — deliberately not in scope here.

[Copilot]

Pull request overview

This pull request fixes a correctness issue in wolfSSH’s channel read paths where a deferred CHANNEL_WINDOW_ADJUST send (e.g., WS_WANT_WRITE on non-blocking sockets) could cause the read API to report failure even after bytes were already delivered/consumed, leading callers to prematurely tear down connections.

Changes:

• Update wolfSSH_stream_read() and the internal _ChannelRead() path (used by wolfSSH_ChannelIdRead() / wolfSSH_ChannelRead()) to always return the number of bytes copied/consumed, decoupling that from the window-adjust send result.
• Record non-success window-adjust send results in ssh->error (and log hard failures), matching the “bytes delivered vs. credit flushed” split already used by _ChannelReadExt().
• Add targeted unit tests covering both affected read entry points under a WS_WANT_WRITE send scenario.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/ssh.c	Ensures channel/stdout read APIs report bytes read even when window-adjust send is deferred, while still surfacing the deferred/failed credit via ssh->error.
tests/unit.c	Adds unit tests validating correct byte reporting, payload integrity, local window crediting, and ssh->error behavior under deferred window-adjust sends.

---

💡 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 #1192

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

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 flush advice added to three public prototypes in wolfssh/ssh.h is wrong in the case that matters.

Details

Proved with a harness against libwolfssh_test.a: after a deferred adjust, five wolfSSH_worker() calls on an idle socket produce zero IO sends and leave the 24-byte WINDOW_ADJUST sitting in the output buffer, because GetInputData() returns WS_FATAL_ERROR with WS_WANT_READ and that is outside worker’s flush gate. That is exactly the state stalled channel is in. The alternative remedy is unreachable: wolfSSH_SendPacket and wolfSSH_OutputPending are WOLFSSH_LOCAL, so a read-only application has no public flush at all. Not created by this PR, but this PR promotes the wrong advice into the public API documentation. (This is the same underlying gap as the existing worker-idle-flush note.)

[ejohnstown]

One other thing. Make the PR title match your one commit's title. Thanks!