Shutdown channel lookup and disconnect state - #1190
Open
ejohnstown wants to merge 7 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses two connection-teardown correctness issues in wolfSSH: (1) wolfSSH_shutdown() could skip channel teardown messages due to looking up the channel using the wrong ID field, and (2) SSH disconnect handling now records a terminal session state so later stream operations reliably fail with WS_DISCONNECT.
Changes:
- Fix
wolfSSH_shutdown()channel lookup to match by peer channel ID (WS_CHANNEL_ID_PEER) so EOF/exit-status/close are sent even when local/peer IDs differ. - Introduce a persistent
ssh->disconnectedsession flag, set on send/receive of SSH_MSG_DISCONNECT, and enforce it inwolfSSH_stream_read()/wolfSSH_stream_send(). - Add/extend unit and regression tests to cover both behaviors.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| wolfssh/internal.h | Adds disconnected session flag to WOLFSSH state. |
| src/ssh.c | Fixes shutdown channel lookup and makes stream read/send fail terminally after disconnect. |
| src/internal.c | Sets ssh->disconnected when DISCONNECT is received/sent. |
| tests/unit.c | Adds unit test covering shutdown behavior when peer/local channel IDs differ. |
| tests/regress.c | Extends regression coverage to ensure disconnect is terminal across subsequent stream calls, including after sending DISCONNECT. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wolfSSL-Fenrir-bot
requested changes
Aug 21, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1190
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. Reported findings require changes before merge.
ejohnstown
commented
Aug 24, 2026
wolfSSL-Fenrir-bot
requested changes
Aug 24, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1190
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
ejohnstown
commented
Aug 25, 2026
ejohnstown
commented
Aug 25, 2026
ejohnstown
commented
Aug 25, 2026
wolfSSL-Fenrir-bot
requested changes
Aug 25, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1190
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
ejohnstown
commented
Aug 25, 2026
ejohnstown
commented
Aug 25, 2026
wolfSSL-Fenrir-bot
requested changes
Aug 25, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1190
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
ejohnstown
commented
Aug 26, 2026
ejohnstown
commented
Aug 26, 2026
wolfSSH_shutdown() searched for the session channel by the peer's channel ID while telling ChannelFind() to match the local ID field. Each side numbers its channels independently, so the search usually found nothing. - The session channel is the head of the list; take it directly instead of searching for what is already in hand. - Restores the EOF, exit-status and close sends, and the drain that waits on the peer's close, all skipped on the NULL result. - Only bit when the two IDs differ, so the single-channel tests, where both sides pick 0, never saw it. - unit.c: shut down a channel whose peer ID is not its local ID, then check that EOF and close went out. Issue: F-8817
SSH_MSG_DISCONNECT left nothing behind but ssh->error, which wolfSSH_stream_read() clears on entry. An application looping on the stream calls lost the code and went back to a connection already over. - Add WOLFSSH.disconnected, set by DoDisconnect() and SendDisconnect(). - DoDisconnect() sets it before decoding the payload, so a malformed message still ends the session. RFC 4253 section 11.1. - wolfSSH_stream_read() and wolfSSH_stream_send() report WS_DISCONNECT from the flag instead of reaching for the transport again. - Both guards run ahead of the channelList NULL test, so a torn-down session reports the disconnect rather than WS_BAD_ARGUMENT. - ssh.h states that undrained channel data goes with the session; internal.h states which calls the flag gates and which it does not. - regress.c: the receive side, the send side, and both of those again on a session with an open channel. Issue: F-8837 The test channel credits the peer's window too. Left at 0, SendChannelData() bails with WS_WINDOW_FULL before the wire, and the "nothing went out" checks would hold with the gate removed.
The disconnect flag gated wolfSSH_stream_read() and wolfSSH_stream_send(), which is the client-side API. wolfsshd and echoserver drive their channels through the channel-id calls, so the daemon was never gated at all. - New SendAfterDisconnect() helper, used by the six send entry points: stream_send, stream_exit, ChannelIdSend, ChannelIdSendExt, extended_data_send and global_request. - Reads stay open, since data that arrived before the disconnect is still the caller's. wolfSSH_stream_read() drains its buffer and reports WS_DISCONNECT only once it runs dry. - wolfSSH_worker() stays ungated; the shutdown paths still pump it. - ssh.h and internal.h describe the split. - regress.c: buffered data survives the disconnect, and every send call refuses without a byte leaving the session. Issue: F-8837 Every public send call means every one: the channel-pointer sends (wolfSSH_ChannelSend, wolfSSH_ChannelSendExt, wolfSSH_ChannelExit), the forwarding requests and both wolfSSH_ChannelFwdNew* opens carry the gate too, and none of them had a message-filter backstop. ChannelCreditWindow() parks its credit rather than sending. The reads that drain what arrived before the disconnect credit the window for the bytes taken, and that credit went straight to the transport: each drain put a CHANNEL_WINDOW_ADJUST on the wire after the session was over, and a failing send replaced the byte count already copied for the caller.
wolfSSH_stream_peek() is how the shell loops decide whether a channel is drained. It had no disconnect check, so a dead session looked exactly like a drained one: zero bytes available, nothing to tell them apart. - Report WS_DISCONNECT once the buffered data runs dry, the same shape wolfSSH_stream_read() uses. What is still buffered comes back first. - ssh.h and internal.h name peek alongside the read call, and no longer claim the read side is ungated outright. - regress.c: peek sees the buffered byte, then sees the disconnect. Raised from the channel-eof branch, where peek becomes the drain gate for the wolfsshd and echoserver shell loops. Issue: F-8837
The ssh.h comment promised that every send call below it reports WS_DISCONNECT, but three did not: wolfSSH_TriggerKeyExchange(), wolfSSH_SendIgnore() and wolfSSH_SendDisconnect(). - All three now take the SendAfterDisconnect() gate, so the sentence in ssh.h describes the code rather than the intent. - TriggerKeyExchange() is the highwater callback's rekey trigger, so this also stops a rekey starting on a session the peer has ended. - SendIgnore() and SendDisconnect() gained the NULL check the gate needs; both already reported WS_BAD_ARGUMENT for that from the callee. - A second disconnect is refused: one ends the session. - regress.c: the three calls join the send sweep. Issue: F-8837
wolfSSL-Fenrir-bot
requested changes
Aug 26, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1190
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. Reported findings require changes before merge.
philljj
reviewed
Aug 26, 2026
The disconnect gate left three ways for traffic to reach a peer that had already ended the session, and it made the default highwater callback report a failure for a packet that had gone out fine. - wolfSSH_shutdown() drops the channel when ssh->disconnected is set, so the EOF, exit status and close are skipped along with the wait for a close the peer will never send - wsHighwater() skips the rekey request on a disconnected session, so a firing high water mark no longer turns SendDisconnect() and SendChannelEof() into failures - wolfSSH_ChangeTerminalSize() gained the SendAfterDisconnect() gate, making the ssh.h contract true for every send declared below it - regress covers all three, including that shutdown leaves eofTxd and closeTxd clear and puts nothing on the wire A disconnect of our own left queued by a short send still reaches the peer. SendDisconnect() records disconnectTxd once the packet is bundled, and one FlushQueuedDisconnect() helper gates the retry in wolfSSH_SendDisconnect() and wolfSSH_shutdown() on that. Keying it on disconnected alone would push whatever was queued, since the peer's disconnect sets that flag too and leaves only unrelated traffic behind. wolfSSH_shutdown() flushes ahead of the channel-list test, so the peer's close retiring the last channel does not strand the disconnect, and an unfinished flush outranks WS_CHANNEL_CLOSED. Its WS_WANT_WRITE stays in ssh->error as well, since callers gate their shutdown retry on that. The highwater guard sits in HighwaterCheck(), not in the default callback: the return that fails the send comes from whatever callback the application installed, and it propagates out through wolfSSH_SendPacket(). Issue: F-8837
The two disconnect tests ran on a session that had never finished user auth, so IsMessageAllowed() blocked the sends on its own and the "nothing on the wire" assertions held even with the gates removed. Both now sit past user auth. With only the shutdown gate reverted the test measures 72 bytes out and both teardown flags set, where before it measured nothing. - wolfSSH_stream_peek() reports WS_DISCONNECT when the channel is gone, the way wolfSSH_stream_read() already did; a missing channel used to read as a bad argument on a session that had simply ended - the drain test covers the no-channel case for both calls Issue: F-8837
wolfSSL-Fenrir-bot
requested changes
Aug 26, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1190
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. Reported findings require changes before merge.
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.
Two independent connection-teardown fixes. A channel lookup that matched the wrong ID field, and a disconnect that left no lasting mark on the session. Both covered by unit.c / regress.c tests that fail without the change.
wolfSSH_shutdown() channel lookup (F-8817)
Terminal disconnect state (F-8837)
SSH_MSG_DISCONNECT set only ssh->error, which wolfSSH_stream_read() clears on entry. New WOLFSSH.disconnected, set by DoDisconnect() before the payload decode and by SendDisconnect() before the send, so a partial or failed send still ends the session. RFC 4253 section 11.1.
The invariant is: after a disconnect nothing more goes out, but what already arrived can still be read. It is documented in wolfssh/ssh.h and on the field in wolfssh/internal.h.
Nothing more goes out. One SendAfterDisconnect() helper gates every send entry point declared below that comment in ssh.h:
wolfSSH_shutdown() is a send path too, and is declared above that comment. It drops the channel when the flag is set, skipping the EOF, exit-status and close sends along with the wait for a close the peer will never send.
wsHighwater(), the default highwater callback, skips the rekey on a disconnected session. Without that, gating wolfSSH_TriggerKeyExchange() would have turned a firing high water mark into a failure for a packet that had gone out fine, since wolfSSH_SendPacket() returns HighwaterCheck()'s result.
wolfSSH_worker() stays ungated on purpose: wolfSSH_shutdown() and several examples pump it after an error, and it is the retry path for a short disconnect send.
What already arrived can still be read. The read calls are deliberately not gated: channel data that arrived before the disconnect was legitimately sent and is still the caller's. Gating wolfSSH_ChannelIdRead() and wolfSSH_extended_data_read() would discard it on exactly the paths wolfsshd uses to drain shell output. wolfSSH_stream_read() and wolfSSH_stream_peek() follow the invariant too: they drain their buffer first and report WS_DISCONNECT only once it runs dry, or straight away when the channel is gone and there is no buffer left.
Tests
Every gate has a regression test in tests/regress.c, checked against the unfixed code. The send sweep asserts each entry point reports WS_DISCONNECT and that io.outSz never moves afterwards; separate tests cover the buffered-data drain, the shutdown teardown, the highwater callback and the terminal resize. tests/unit.c covers the shutdown channel lookup with a peer ID that differs from the local one.
The disconnect tests run on a session past user auth. Without that, IsMessageAllowed() blocks the sends on its own and the "nothing on the wire" assertions hold even with the gates removed.