Shutdown channel lookup and disconnect state - #1190
Conversation
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.
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 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
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
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.
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
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.
| if (ssh == NULL) | ||
| ret = WS_BAD_ARGUMENT; | ||
|
|
||
| if (ret == WS_SUCCESS && SendAfterDisconnect(ssh)) |
There was a problem hiding this comment.
Gating wolfSSH_TriggerKeyExchange() makes the default highwater callback fail every send after a disconnect · Incorrect error handling
wsHighwater() (src/internal.c:557) is the default highwaterCb, and wolfSSH_SendPacket() returns HighwaterCheck()'s result (src/internal.c:4318). Once disconnected is set, a firing highwater turns every successful packet send into WS_FATAL_ERROR: SendDisconnect() reports failure after writing the message, and SendChannelEof() leaves eofTxd clear so wolfSSH_shutdown() skips the exit-status and close sends.
Related known finding #8817 (similar but distinct): Both can cause wolfSSH_shutdown() to skip EOF/exit/close traffic, but this finding faults wolfSSH_TriggerKeyExchange() returning WS_FATAL_ERROR after disconnect through the highwater callback; #8817 faults wolfSSH_shutdown() using a peer ID with SELF lookup. Their root causes and required patches are separate.
Fix: Return WS_SUCCESS (skip the rekey) instead of WS_FATAL_ERROR when ssh->disconnected is set, or gate inside wsHighwater() rather than in wolfSSH_TriggerKeyExchange().
| WOLFSSH_API int wolfSSH_connect(WOLFSSH* ssh); | ||
| WOLFSSH_API int wolfSSH_shutdown(WOLFSSH* ssh); | ||
| /* A disconnect, sent or received, ends the session. Nothing more goes out: | ||
| * every send call below reports WS_DISCONNECT from then on. Reads are not |
There was a problem hiding this comment.
ssh.h contract claims all send calls below it are gated, but wolfSSH_ChangeTerminalSize() is not · API contract violations
The new comment states "every send call below reports WS_DISCONNECT from then on", but wolfSSH_ChangeTerminalSize() is declared at ssh.h:633 and its implementation (src/ssh.c:1675) calls SendChannelTerminalResize() with no SendAfterDisconnect() gate, so a window-change request still goes out on an ended session.
Fix: Add the SendAfterDisconnect() gate to wolfSSH_ChangeTerminalSize(), or narrow the ssh.h sentence to name the calls actually gated.
| /* The session channel is the head of the list. */ | ||
| if (ret == WS_SUCCESS) { | ||
| channel = ChannelFind(ssh, ssh->channelList->peerChannel, WS_CHANNEL_ID_SELF); | ||
| channel = ssh->channelList; |
There was a problem hiding this comment.
wolfSSH_shutdown() transmits and drains after a disconnect · SSH protocol violations
Taking the head channel directly makes the send block at src/ssh.c:1137 always execute, but wolfSSH_shutdown() is the one send path this PR leaves ungated by ssh->disconnected. After DoDisconnect() it emits CHANNEL_EOF, exit-status and CHANNEL_CLOSE (RFC 4253 §11.1 forbids sending after SSH_MSG_DISCONNECT), then waits in wolfSSH_worker() at src/ssh.c:1163 for a close the peer will not send. Reached in-tree from examples/echoserver/echoserver.c:1640, where error == WS_DISCONNECT passes the guard at line 1639.
Related known finding #8817 (similar but distinct): Both affect wolfSSH_shutdown() channel teardown sends and drain logic, but #8817 faults in ChannelFind using peerChannel with WS_CHANNEL_ID_SELF, while this candidate faults by allowing sends/draining after ssh->disconnected. Their root causes and required patches differ.
Fix: Apply the SendAfterDisconnect() gate in wolfSSH_shutdown() so the channel teardown sends and the drain are skipped once ssh->disconnected is set.
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 Issue: F-8837
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)