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
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.
| 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 documents a disconnect guarantee that two public send APIs do not honor · API contract violations
The new comment states "every send call below reports WS_DISCONNECT from then on", but wolfSSH_SendIgnore() (src/ssh.c:1535) and wolfSSH_TriggerKeyExchange() (src/ssh.c:1189) are declared below it and have no SendAfterDisconnect() gate, so they still emit SSH_MSG_IGNORE and SSH_MSG_KEXINIT on a session the library considers over.
Related known finding #7512 (similar but distinct): Both involve wolfSSH_SendIgnore's public API contract, but 7512 concerns discarding its caller payload; this finding concerns missing post-disconnect send gating in SendIgnore and wolfSSH_TriggerKeyExchange. The faulting operations and root causes differ, and separate patches are required.
Fix: Add the SendAfterDisconnect() gate to wolfSSH_SendIgnore() and wolfSSH_TriggerKeyExchange(), or narrow the comment to the six gated calls.
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)