wolfsshd: drain the shell channel before closing the child's stdin - #1212
Open
ejohnstown wants to merge 3 commits into
Open
wolfsshd: drain the shell channel before closing the child's stdin#1212ejohnstown wants to merge 3 commits into
ejohnstown wants to merge 3 commits into
Conversation
wolfSSH_accept() and wolfSSH_connect() drive the handshake only while the session is live. Both gate on SendAfterDisconnect() ahead of the pending-send block, which would otherwise flush a queued disconnect and count it as the next handshake message; the shutdown paths own that flush. - The prototype sits ahead of both drivers, since either can be the only one built. - TestDisconnectGatesAccept() and TestDisconnectGatesConnect() cover a local disconnect, one from the peer, and a queued short send. One test per endpoint, so a single-sided build keeps the coverage that applies to it. - A received disconnect used to reach the error-state test in wolfSSH_accept() and report WS_INVALID_STATE_E; the gate answers WS_FATAL_ERROR first, and both tests pin that. - The contract comments in ssh.h and internal.h drop the ungated note.
DoPacket() skips the whole message dispatch once ssh->disconnected is set, for every message but a DISCONNECT. The handlers that answer must not -- a close draws an EOF and a close of ours, a request a success or failure, an open a confirmation, an unknown message an UNIMPLEMENTED -- and what the rest would record is of no use to a caller that can no longer send. RFC 4253 section 11.1. - Inbound data from here on is dropped rather than buffered, so the read path hands back only what arrived before the disconnect. ssh.h and internal.h say so, beside the calls and beside the flag. - A DISCONNECT still reaches DoDisconnect(), which sends nothing and is what latches WS_DISCONNECT; SendDisconnect() sets the flag too, so ours can be the one that raised it. - The frame advance steps over the whole packet, so the stream stays in step with no payload bookkeeping of its own. - wolfSSH_worker() stops reporting WS_REKEYING once the session is over. The NEWKEYS that clears isKeying is skipped from here on, so the flag would latch for the rest of the session and the SFTP and SCP drive loops would keep pumping a dead one. - tests/regress.c pins all three: no reply goes out, the stream stays in step with a disconnect queued behind a skipped close, and late channel data is dropped rather than queued.
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.
Contributor
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.
Comment on lines
+1900
to
+1904
| current = wolfSSH_ChannelFind(ssh, shellChannelId, WS_CHANNEL_ID_SELF); | ||
| if (current != NULL | ||
| && current->inputBuffer.length > current->inputBuffer.idx) { | ||
| pending = 1; | ||
| } |
Comment on lines
+1978
to
+1983
| /* The shell channel's own buffer, looked up again because the | ||
| * worker above can retire it. */ | ||
| current = wolfSSH_ChannelFind(ssh, shellChannelId, | ||
| WS_CHANNEL_ID_SELF); | ||
| avail = (current != NULL) ? | ||
| current->inputBuffer.length - current->inputBuffer.idx : 0; |
| word32 quietSz; | ||
| word32 channelId; | ||
|
|
||
| ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); |
| channelId = channel->channel; | ||
| /* Past userauth, or the message filter turns the inbound messages away | ||
| * on its own and the wire check below proves nothing. */ | ||
| ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; |
| word32 quietSz; | ||
| word32 channelId; | ||
|
|
||
| ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); |
| AddSessionChannel(ssh); | ||
| channel = ssh->channelList; | ||
| channelId = channel->channel; | ||
| ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; |
| word32 inSz; | ||
| word32 channelId; | ||
|
|
||
| ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); |
| AssertNotNull(ssh); | ||
| AddSessionChannel(ssh); | ||
| channelId = ssh->channelList->channel; | ||
| ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; |
Comment on lines
+280
to
+290
| static word32 BuildChannelDataPacket(word32 peerChannelId, const char* data, | ||
| byte* out, word32 outSz) | ||
| { | ||
| byte payload[64]; | ||
| word32 idx = 0; | ||
|
|
||
| idx = AppendUint32(payload, sizeof(payload), idx, peerChannelId); | ||
| idx = AppendString(payload, sizeof(payload), idx, data); | ||
|
|
||
| return WrapPacket(MSGID_CHANNEL_DATA, payload, idx, out, outSz); | ||
| } |
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.
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.