wolfsshd: keep the shell loop past a stderr EOF - #1263
Open
ejohnstown wants to merge 6 commits into
Open
ejohnstown wants to merge 6 commits into
ejohnstown wants to merge 6 commits into
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This pull request prevents shell-output truncation after stderr EOF during stdout back-pressure and adds regression coverage.
Changes:
- Updates shell-loop EOF and polling behavior.
- Adds a 16 MB stalled-reader regression test.
- Registers the test in the SSHD suite.
File summaries
| File | Summary and final review comment |
|---|---|
apps/wolfsshd/wolfsshd.c |
Updates shell pipe handling. Critical (2 votes): retry the outer loop on EINTR so pending stderr bytes are not dropped. |
apps/wolfsshd/test/sshd_stderr_eof_test.sh |
Adds back-pressure regression coverage. Moderate (1 vote): use per-invocation temporary files to support concurrent runs safely. |
apps/wolfsshd/test/run_all_sshd_tests.sh |
Registers the new test. Moderate (3 votes): update the external-host skipped-test tally. |
Review details
Suppressed comments (1)
apps/wolfsshd/test/sshd_stderr_eof_test.sh:41
- The suite runner deliberately supports concurrent runs with per-run ports, but this test writes a fixed config, input file, and result file in shared working directories. Two runs can overwrite or truncate each other's files and report a false short transfer. Use a per-invocation
mktemp -d(with EXIT cleanup) and pass absolute paths into the nested command instead of fixed filenames.
cat <<CONF > sshd_config_test_stderr_eof
- 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.
read() returns 0 at end of file and leaves errno alone, so the stderr arm consults it only for a -1 return. A 0 marks that stream done and drops it from the read set; stdout does the same, so neither spins on a descriptor that stays ready once it has reported EOF. - a stale EINTR from the child's SIGCHLD no longer ends the loop with the peer's output still queued - EINTR and EWOULDBLOCK join EAGAIN as non-fatal on a -1 return - select polls rather than blocks once the child is gone, its output is drained and nothing is held, so the loop still reaches its exit
sshd_stderr_eof_test.sh streams 16 MB through a shell session whose reader stalls, so the send window fills and the child exits while the loop still holds a backlog. The transfer repeats twelve times because whether the stale errno at the stderr EOF is fatal is a race. - a short transfer is never correct, so a failure here is always real - the fixed tree passes 48 consecutive transfers; the truncating one fails on the first - runs in the sudo-only block beside sshd_window_full_test.sh, which reaches the same path about 1 run in 12
A stream leaves the read set once it has reported EOF, so a child whose output is drained while it still runs leaves the SSH socket as the only descriptor watched, and its SIGCHLD is the only wake left. One handled between the ChildRunning test and select() would not interrupt the call, so wait a second at a time there. An exited child still polls.
The stdout and pty childFd reads treat EINTR and EWOULDBLOCK as non-fatal, the set the stderr read already uses, so an interrupted read cannot end the session with the peer's output still queued. The "err != 0" term goes with it; read() sets errno on the -1 return that is the only way into the test.
A byte count cannot tell a short transfer from one that never finished and the runner sets no deadline of its own, so the client runs under "timeout" and a 124 is reported as a hang. An empty PID means the daemon never came up, which the count would otherwise report as the truncation this test exists to catch. - scratch paths held in variables, since bash rewrites PWD on the cd back into the test directory - an EXIT trap removes the 16 MB scratch file and stops the daemon
The runner leases a port block per run so two runs can share a host, and this test's config and 16 MB payload are the other half of that: a second run overwrites them, and whichever finishes first removes them from under the other, which then reports a transfer that never ran short. Both live in a directory of this run's own now. - the external-host tally counts 12 local-only tests, not 10
ejohnstown
force-pushed
the
shell-eof-errno
branch
from
September 18, 2026 23:01
238da54 to
c97a768
Compare
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.
wolfsshd truncates shell output under write back-pressure: the child's stderr reaching end of file ends the loop while its stdout still has bytes queued. Follow-up to the conditions PR 1253 introduced.
read()leaveserrnoalone at EOF, so the stderr arm consults it only on a -1 return, and a stream that has reported EOF leaves the read set instead of being spun on.select()polls rather than blocks once the child is gone with nothing held, since that always-ready pipe was what kept the loop turning.sshd_stderr_eof_test.shcovers it: 16 MB through a stalled reader, twelve passes, failing on the first short transfer. Master truncates 4 of 12 runs under a stalled reader, the fix 0 of 24; the test fails master on pass 1 and passes 48 consecutive transfers here.