portfwd: drain the buffer before leaving on EOF - #1228
Open
ejohnstown wants to merge 2 commits into
Open
Conversation
A zero read on the local socket left the loop at once, and anything read but not yet accepted by wolfSSH_ChannelSend() went with it. With the peer's window full that tail is up to a buffer's worth, so a transfer that ends while the window is being credited comes out short. - stop polling the socket on end-of-input and keep looping until the buffer is empty
scripts/fwd.test sends eight short lines, so nothing in the suite drives a forwarding channel past its first window, and a forward that stalled once the window needed crediting would go unnoticed. - push a payload several windows long through a local direct-tcpip forward and compare the bytes that arrive - give the listening nc its stdin from a fifo a sleep holds open: reading end-of-input makes nc close the connection, which truncates the transfer and looks exactly like a stall - bail out early once the byte count stops moving, so a real stall reports in seconds - read the size the receiver is held to back from the payload file - dump the logs on failure, check the listening nc came up, and take ports clear of the ones fwd.test.expect hardcodes, so a squatted or shared port is not reported as a stall
ejohnstown
requested review from
wolfSSL-Fenrir-bot
and
a lite review from Copilot
September 3, 2026 00:18
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new EOF handling currently treats recv() < 0 as EOF (masking socket errors), and the new shell test uses [ with -a in loop conditions in a way that is less portable/error-prone than the repo’s existing style.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a port forwarding edge case where portfwd_worker() would exit immediately on local socket EOF, potentially dropping buffered data that had not yet been accepted by wolfSSH_ChannelSend() (e.g., while SSH window credits are in flight). It also adds a new bulk forward test to exercise transfers spanning multiple SSH windows.
Changes:
- Update
examples/portfwd/portfwd.cto stop polling the local socket on EOF and keep looping until the buffered outbound data is drained. - Add
scripts/fwd-bulk.testto push a multi-window payload through a local direct-tcpip forward and validate byte-for-byte integrity. - Register the new test script in
scripts/include.amso it runs undermake check.
File summaries
| File | Description |
|---|---|
| scripts/include.am | Adds the new bulk forwarding test script to the check scripts list. |
| scripts/fwd-bulk.test | New bulk data forwarding test that verifies multi-window payload forwarding correctness. |
| examples/portfwd/portfwd.c | Adjusts EOF handling to drain the local-to-SSH buffer before exiting the worker loop. |
Review details
Suppressed comments (2)
scripts/fwd-bulk.test:172
- Using
[with-ainside a single test expression is error-prone and less portable across /bin/sh implementations. Prefer&&with separate test brackets.
while [ "$got" -lt "$payload_size" -a "$counter" -lt "$transfer_limit" ]; do
scripts/fwd-bulk.test:159
- Using
[with-ainside a single test expression is error-prone and less portable due to historical precedence/parse quirks across /bin/sh implementations. Prefer combining two test brackets with&&instead.
while [ ! -s "$fwd_ready_file" -a "$counter" -lt 20 ]; do
- Files reviewed: 3/3 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.
Comment on lines
728
to
+737
| rxd = (int)recv(appFd, | ||
| appBuffer + appBufferUsed, appBufferSz - appBufferUsed, 0); | ||
| if (rxd > 0) | ||
| appBufferUsed += rxd; | ||
| else | ||
| break; | ||
| else { | ||
| /* Local end-of-input. Stop polling the socket and leave once | ||
| * what is buffered has gone out; leaving now would drop it. */ | ||
| appEof = 1; | ||
| FD_CLR(appFd, &templateFds); | ||
| } |
| server_pid=$! | ||
|
|
||
| counter=0 | ||
| while [ ! -s "$ready_file" -a "$counter" -lt 20 ]; do |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A zero read on the local socket left portfwd_worker()'s loop at once and
took with it anything wolfSSH_ChannelSend() had not yet accepted, so a
transfer ending while the window is being credited came out short.
buffer is empty.
forward, since scripts/fwd.test never drives past the first window.