wolfSSH_stream_read consumes before crediting and always reports the byte count. - #1188
wolfSSH_stream_read consumes before crediting and always reports the byte count.#1188yosuke-wolfssl wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes data-loss and deadlock scenarios caused by partial writes in the echo server example and incorrect window-credit sequencing in wolfSSH_stream_read(). It also adds targeted regression tests to cover both the stream/window-credit behavior and the short-send retry logic in an integration-style scenario against the echo server.
Changes:
- Update
wolfSSH_stream_read()to consume bytes before window crediting and always return the bytes copied, recording window-adjust send failures inssh->error. - Rework
examples/echoserver/echoserver.cI/O loops to correctly handle partial sends/writes by buffering unsent data and retrying when the channel becomes writable/unblocked. - Add new unit/API tests to validate deferred window adjust behavior and window credit round-tripping with a small receive window.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/ssh.c |
Fixes stream read window-credit sequencing and return/error reporting behavior. |
examples/echoserver/echoserver.c |
Adds robust partial-write handling for shell/agent/forwarding paths and select() wakeups on WANT_WRITE. |
tests/unit.c |
Adds a unit regression test ensuring deferred window credit doesn’t “lose” consumed bytes. |
tests/api.c |
Adds an echoserver-gated API test that validates window credit recovery across a payload larger than the receive window. |
Suppressed comments (1)
examples/echoserver/echoserver.c:1125
- In the agent relay path, agentChannelId is initialized to (word32)-1 and (per a file-wide search) is never updated, but it is passed to wolfSSH_ChannelIdRead()/wolfSSH_ChannelIdSend(). Since those APIs look up channels by self ID, this will consistently fail with WS_INVALID_CHANID and prevent agent forwarding from working. The code likely needs to plumb the correct self channel ID for the auth-agent channel into agentChannelId (or another tracked field) before attempting reads/sends.
if (lastChannel == agentChannelId) {
cnt_r = wolfSSH_ChannelIdRead(ssh, agentChannelId,
threadCtx->channelBuffer,
sizeof threadCtx->channelBuffer);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
a620821 to
b826f2f
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1188
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.
- app_write_all() hands a whole buffer to a local descriptor across partial transfers and returns WS_FATAL_ERROR otherwise; it waits on writability with a bounded try count and checks the select() result. The pty, agent and forwarding writes in ssh_worker() call it, and SOCKET_EAGAIN and SOCKET_EINTR join the socket error macros. - ssh_worker() carries shellBufferIdx and agentBufferIdx beside the existing fwdBufferIdx. A descriptor is read only while its staging buffer is empty and stays out of the read set otherwise; the forwarding recv() fills the buffer from its base. - Each buffer has a flush block, outside the descriptor-state guards, that subtracts what wolfSSH_ChannelIdSend() took and moves the remainder down. WS_CHANNEL_NOT_CONF, WS_CHAN_RXD, WS_WINDOW_FULL and WS_REKEYING hold the data for a later pass. - WS_WANT_WRITE also holds it and sets wantWrite, which adds sshFd to a write set passed to select(). - Echo mode reads the channel into shellCtx.buffer and shares the shell flush block; process_bytes() runs on that buffer. Issue: F-10544
- wolfSSH_stream_read() advances inputBuffer->idx before _UpdateChannelWindow(), records a non-success result in ssh->error and reports the byte count. - tests/api.c adds test_wolfSSH_stream_read_WindowCredit(), which round-trips 2000 bytes through a 1024-byte client receive window, with its own user-auth and host-key callbacks. - tests/unit.c adds test_stream_read_deferredWindowAdjust(), which puts a full window of channel data and reads it back with an IO send that reports WS_CBIO_ERR_WANT_WRITE, checking the byte count, the payload, ssh->error, the credited window and the consumed buffer.
- _ChannelRead() records a non-WS_SUCCESS _UpdateChannelWindow() result in channel->ssh->error and returns the bytes copied, logging anything other than WS_WANT_WRITE. - tests/unit.c gains test_ChannelIdRead_deferredWindowAdjust(), which seeds ssh->error, then reads a full window through wolfSSH_ChannelIdRead() with an IO send that reports WS_CBIO_ERR_WANT_WRITE, and checks the byte count, the payload, the recorded error, the local window credit and the drained input buffer.
b826f2f to
45b5130
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1188
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.
| #endif /* WOLFSSH_SHELL */ | ||
| /* Drain what a full staging buffer made us defer. A zero read | ||
| * means the channel is empty, not an error. */ | ||
| if (shellRxPending |
There was a problem hiding this comment.
Deferred channel-read drain runs before the flush, so a full staging buffer strands data until unrelated inbound traffic · Logic errors
The new drain block is skipped whenever shellBufferIdx == sizeof shellCtx.buffer, and it executes before the flush at line 1248 that empties the buffer. When a channel read fills the staging buffer exactly and the flush then drains it, select() at line 1065 blocks with shellRxPending still set and channel data still unread, stalling the echo.
Related known finding #10544 (similar but distinct): Both are ssh_worker buffering/flush progress failures, but this is a deferred-read ordering condition after an exactly full staging buffer; #10544 is positive partial-write handling that loses unsent data. The faulting operations, root causes, and required patches differ.
Fix: Move the deferred-read drain after the flush block, or re-run it in the same pass once the flush has freed staging space.
| } | ||
| } while (cnt_w > 0 && shellBufferIdx > 0); | ||
|
|
||
| if (cnt_w > 0) { |
There was a problem hiding this comment.
A positive wolfSSH_ChannelIdSend() return is treated as flushed, but SendChannelData() also returns dataSz on… · Incorrect error handling
SendChannelData() maps both WS_SUCCESS and WS_WANT_WRITE to dataSz (src/internal.c:20365), so a positive return can leave the packet parked in ssh->outputBuffer. The three flush blocks (lines 1265, 1331, 1426) then label it /* drained */ and leave wantWrite clear, so sshFd is not added to the write set and the parked tail is never retried until unrelated inbound traffic wakes the loop.
Related known finding #10544 (similar but distinct): Both misinterpret a positive channel-send result in ssh_worker, but this candidate concerns WS_WANT_WRITE being reported as dataSz while data remains in the library output buffer and write readiness is not armed; #10544 concerns partial writes whose local unsent suffix is discarded. Retrying output differs from preserving and resending partial source data.
Fix: After a positive return, set wantWrite when wolfSSH_get_error(ssh) == WS_WANT_WRITE (or wolfSSH_OutputPending(ssh) is true) in all three flush blocks.
Problem
ssh_worker()in the echo server treats every positivewrite(),send(), andwolfSSH_ChannelIdSend()return as a complete transfer.SendChannelData()clampseach send to
min(peerWindowSz, peerMaxPacketSz, maxPacketSz)and returns thatclamped count, so a send larger than the peer's window returns short. Every call
site then overwrites or discards its source buffer — the forwarding path resets
fwdBufferIdxto0on any positive result — and the unsent suffix is lostsilently. Reachable through the shell, agent, and forwarding paths.
Separately,
wolfSSH_stream_read()creditsinputBuffer->idxbytes but advancedidxonly afterwards, so a read never credits its own bytes. A single read thatdrains the entire receive window leaves the window at zero with nothing left to
trigger a credit, wedging the stream permanently. Latent at the default 128 KB
window; immediate with a smaller one.
Fix (
examples/echoserver/echoserver.c)Two mechanisms, split by which side owns the backpressure:
drains, so
app_write_all()advances past each partial transfer and returnsWS_FATAL_ERRORif it cannot complete.process inbound packets. Each direction stages into its
WS_AppCtxbuffer; theflush subtracts what was sent and
WMEMMOVEs the remainder for the next pass.> 0WS_CHANNEL_NOT_CONF,WS_CHAN_RXD,WS_WINDOW_FULL,WS_REKEYINGWS_WANT_WRITEsshFdto a write set soselect()wakes on writabilityA descriptor stays out of the read set while its buffer is pending. Closes f-10544.
Fix (
src/ssh.c)wolfSSH_stream_read()consumes before crediting and always reports the byte count,recording a failed adjust in
ssh->error— the contract_ChannelReadExt()alreadyfollows, so returning an error after consuming cannot drop the caller's bytes.
Tests
test_wolfSSH_stream_read_WindowCredit()(tests/api.c) round-trips 2000 bytesthrough a 1024-byte client receive window against the echo server.
test_stream_read_deferredWindowAdjust()(tests/unit.c) reads a full window withan IO send that reports
WS_CBIO_ERR_WANT_WRITE.Verification
unit,api,testsuite,regress,kex,auth, and thescripts/tests includingfwd.test.-Werroracross six GCC configurations; ASan + UBSan clean.intermediate variant that consumed the bytes but returned the error.