wolfsshd: fix data loss in shell relay loop on EINTR, stderr windowFull, and WS_WANT_WRITE#996
Open
ejohnstown wants to merge 6 commits into
Open
wolfsshd: fix data loss in shell relay loop on EINTR, stderr windowFull, and WS_WANT_WRITE#996ejohnstown wants to merge 6 commits into
ejohnstown wants to merge 6 commits into
Conversation
SIGCHLD from the exec child interrupts select(), returning -1 and breaking the loop. Any windowFull data awaiting a peer window adjust was then abandoned, causing intermittent 32 KB truncations on large exec transfers. Continue on EINTR.
Stderr and stdout shared shellBuffer/windowFull, so a stderr hold retried on stdout, and a stderr partial send let the stdout read clobber the remainder. Tag the held bytes with windowFullExt; retry via extended_data_send and skip the stdout read when set.
ChannelIdSend / extended_data_send returning WS_WANT_WRITE set wantWrite but didn't save cnt_r, so the next read overwrote the held bytes. Store cnt_r in windowFull at all three read sites (stderr, stdout, pty childFd); flag windowFullExt for stderr so the retry routes through extended_data_send.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens wolfsshd’s shell relay loop to avoid losing output when I/O is interrupted or temporarily blocked, and to correctly preserve the stream (stdout vs stderr) associated with buffered “window full” remainders.
Changes:
- Retry
select()when interrupted byEINTRso the loop can continue draining pending SSH/window-full and pipe data. - Track whether buffered
windowFullbytes belong to stderr (windowFullExt) and resend them viawolfSSH_extended_data_send()when appropriate. - Preserve the current read buffer on
WS_WANT_WRITEso data can be retried instead of dropped.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Guard the retry against a negative cnt_w so a non-sentinel send error cannot become a negative memmove offset. - Clear windowFullExt at the stdout and childFd save sites so stream ownership is set unconditionally.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #996
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
Lets the relay loop reach its shutdown condition after SIGCHLD instead of relying on a -1/EIO break.
- buffer unsent bytes on backpressure/partial send and resend - hold loop open until buffered data flushes after child exit
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.
ZD #21760