wolfsshd: drain the shell channel before closing the child's stdin - #1212
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Improves wolfsshd’s shell subsystem robustness by ensuring buffered channel input is drained to the child before stdin is closed, and tightens “disconnect is terminal” behavior across drivers and inbound packet dispatch.
Changes:
- Gate
wolfSSH_accept()/wolfSSH_connect()after disconnect to prevent further handshake progress or flushing queued disconnects as “next handshake message”. - Skip inbound packet dispatch after disconnect (except
DISCONNECT) so late traffic is dropped and no replies/callbacks fire. - Update wolfsshd’s shell loop to (a) derive the shell channel id from the actual channel list and (b) drain the channel’s buffer to the child before closing stdin; add regression tests for disconnect gating/dispatch behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
wolfssh/ssh.h |
Updates public API docs describing disconnect semantics and what still drains after disconnect. |
wolfssh/internal.h |
Clarifies internal disconnected flag behavior and how inbound dispatch changes post-disconnect. |
src/ssh.c |
Adds disconnect gating in accept/connect and adjusts worker behavior when rekeying vs disconnected. |
src/internal.c |
Skips inbound message dispatch after disconnect (except DISCONNECT) to silence replies/callbacks. |
apps/wolfsshd/wolfsshd.c |
Fixes shell channel selection and drains buffered input before closing child stdin; avoids DEFAULT_NEXT_CHANNEL assumptions. |
tests/regress.c |
Adds regression tests and packet builders to validate disconnect gating and post-disconnect dispatch behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
9b2d3ea to
cb434e8
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1212
Scan targets checked: wolfssh-bugs, wolfssh-src
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
SHELL_Subsystem() hands the child whatever the peer sent, whichever pass it arrived on, and closes the write end of its stdin only once that buffer is dry. It works off the shell channel's own inputBuffer, so data held back while the window was full is still handed over; the old read ran only on the worker's WS_CHAN_RXD and was skipped outright while windowFull. - The channel id comes from the head of the channel list at entry, the only channel open there, rather than from DEFAULT_NEXT_CHANNEL, which a build can override. - A lookup that finds nothing is not an EOF: only a channel that is present and drained closes the pipe. - Data arriving behind the peer's EOF, which RFC 4254 section 5.3 forbids, is dropped rather than written to a stdin that is already closed. The write would fail with EBADF and end the session mid-stream. - The short-write retry tests for a -1 return before reading errno, which nothing else sets.
cb434e8 to
924e1c7
Compare
SHELL_Subsystem()drops what the peer sent whenever the send window is full. The read that hands channel data to the child runs only on the worker'sWS_CHAN_RXDand is skipped outright whilewindowFullis set, and the pipe is then closed on a read of zero without asking whether anything is still buffered on the channel. A peer that fills the window and then stops sending loses whatever arrived in that window.The channel it works on is named by
DEFAULT_NEXT_CHANNELas well, so a build that overrides the macro closes the stdin of a channel it never read.inputBuffer, so data held back while the window was full is still handed over, whichever pass it arrived onwaitpid()below waiting on a child that may never exiterrno, which nothing else sets, and finishes a partial write rather than dropping the remainder: the read took the bytes off the channel, so that is the only copywindowFull. Not because the buffers overlap -- they are disjoint -- but because writing to the child's stdin while the peer will not take its output deadlocks it: it blocks on a full stdout pipe, stops reading stdin, and the write never returnsBoth bugs are live on master today and are independent of the EOF work; this is lifted out of #1195 so it can land ahead of #900, which rewrites the same file.
This branch contains the two commits of #1211, which must merge first -- review only the last commit here (GitHub shows three commits and six files for that reason). Merge order: #1211, #1212, #1195, then #1148. The end-to-end coverage for the drain travels with #1195: on master
DoChannelEof()answers a half-close with an EOF of its own, which latcheseofTxd, so wolfSSHd cannot send the command's output back and no half-close test can pass until that lands.