wolfsshd: fix data loss in shell relay loop on EINTR, stderr windowFull, and WS_WANT_WRITE#996
wolfsshd: fix data loss in shell relay loop on EINTR, stderr windowFull, and WS_WANT_WRITE#996ejohnstown wants to merge 4 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.
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.
| * Re-evaluate the loop condition so any pending windowFull | ||
| * data and remaining pipe contents still get drained. */ | ||
| if (errno == EINTR) | ||
| continue; |
There was a problem hiding this comment.
🟠 [Medium] PTY sessions can hang after SIGCHLD · Logic errors
The EINTR retry re-enters the loop after SIGCHLD, but PTY mode never sets stdoutEmpty, so the loop shutdown condition stays true after the shell exits.
Fix: Set stdoutEmpty on PTY EOF or make the shutdown condition not require stdoutEmpty for PTY sessions.
ZD #21760