examples/echoserver: hold what a channel send did not take by yosuke-wolfssl · Pull Request #1261 · wolfSSL/wolfssh

yosuke-wolfssl · 2026-09-18T00:53:08Z

Problem

ssh_worker() treated every positive write(), send() and wolfSSH_ChannelIdSend()
as a complete transfer. SendChannelData() clamps each send to
min(peerWindowSz, peerMaxPacketSz), so short sends are routine and seven sites
discarded the unsent suffix — the forwarding path reset its buffer index on any
positive return. Not confined to small windows: a 4 MB echo at the default window
loses 98 KiB. Closes f-10544 (High).

Fix (`examples/echoserver/echoserver.c`)

Each WS_AppCtx stages both directions and keeps what its sink would not take:
buffer/bufferIdx/bufferOff toward the channel, fdBuffer/fdIdx/fdOff toward
the descriptor.

app_drain_to_channel() advances bufferOff by what the send took and holds the
rest on WS_WANT_WRITE, WS_WINDOW_FULL, WS_REKEYING, WS_CHANNEL_NOT_CONF and
WS_CHAN_RXD. For the latter four it restores the entry ssh->error: a leftover
WS_WINDOW_FULL fails DoKexInit() on the peer's next KEXINIT.
app_pump_to_fd() moves channel data to the pty or forward socket, both now
nonblocking, and keeps what would block. The descriptor sits in the select() write
set while it owes bytes, so no write can stall the loop.
app_echo_pump() echoes the shell channel, then runs process_bytes(). It
replaces the EOF drain.
app_flush_fd() gives a forward up to 5 s without progress after the session ends
to take what it still owes.
The agent socket stays blocking; app_write_all() retries interrupted sends.

A descriptor leaves the read set while its channel-bound buffer is non-empty, both
pumps run every pass, and a forward clears both buffers as it connects.

Tests (`scripts/fwd-bulk.test`)

Both phases need python3 and ssh, and skip without them. OpenSSH logs in with a
password, so no key algorithm has to be compiled in.

Phase 3 echoes 32 MB through ssh -L while the client keeps sending but stops
reading for 10 s. The first revision of this PR deadlocks there at ~4.2 MB, blocked in
send() to the forward; so does this one with the forward left blocking.
Phase 4 ends a forward while the echoserver still holds bytes for its target, then
checks the next forward's target gets only its own. Without the reset on connect, the
second target receives 69–4113 stale bytes (3/3 runs).

Verification

make check 13/13; GCC -Werror clean on 6 configurations; CI green.
Phases 3–4 plus a local stress matrix (two-way, rekey mid-stream, SSH peer stalling;
blocking and -N sockets): 18/18 on macOS, 18/18 on Linux (ubuntu:24.04).

Not in this PR

Bytes still owed to a forward's target are dropped when the peer closes the
channel, or the target half-closes, before the target has read them. Pre-existing:
a client that sends 600 KB and closes at once loses ~120 KB to a stalled target on
master too, and ~5 KB more here (fdBuffer). OpenSSH and portfwd don't close that
way, and lost nothing in testing. Keeping them needs the library to hold a closed
channel's unread data.
tests/api.c regression tests. Withdrawn: they force a rekey mid-transfer in
small-highwater builds, hitting a separate defect that reproduces on master — the
echo path treats a rekey-blocked send as fatal. They land with that fix.
dump_stats() has the same flaw. A plain retry is worse than the truncation: the
second send finds the window exhausted and the caller ends the session. Staging the
blob across passes is out of proportion for debug output behind a control byte.
The ESP-IDF copy, deferred: its ssh_worker() has diverged by ~580 lines and has
no select() write set, which this fix depends on.

Copilot

🟡 Changes recommended

Critical EOF flushing and forwarding teardown issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Reworks echo-server buffering to preserve unsent data during short channel and descriptor writes, with regression tests for flow control and half-close behavior.

Changes:

Adds persistent staging and reliable drain helpers.
Updates echo, agent, and forwarding paths.
Adds 8,000-byte round-trip regression tests.

File summaries

File	Summary
`tests/api.c`	Adds flow-control, refill, and half-close tests.
`examples/echoserver/echoserver.c`	Preserves partial transfers across worker passes.

Review details

Suppressed comments (1)

examples/echoserver/echoserver.c:1760

When the local forwarding socket reaches EOF, this path changes fwdCtx to APP_STATE_LISTEN and continues without resetting bufferIdx/bufferOff. Any staged tail left by an earlier channel send therefore survives into the next accepted connection and is sent on its channel. Reset the staging fields on this EOF/reset teardown as well (and do the same for the analogous agent-socket close paths).

                    if (cnt_r == 0) {
                        /* Read zero-returned. Socket is closed. Go back
                           to listening. */
                        WCLOSESOCKET(fwdFd);
                        fwdFd = -1;
                        threadCtx->fwdCtx.appFd = -1;
                        if (threadCtx->fwdCbCtx.hostName != NULL) {
                            WFREE(threadCtx->fwdCbCtx.hostName, NULL, 0);
                            threadCtx->fwdCbCtx.hostName = NULL;
                        }
                        threadCtx->fwdCtx.state = APP_STATE_LISTEN;
                        continue;

Files reviewed: 2/2 changed files
Comments generated: 2
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