ssh: flush any queued output at teardown - #1219
Conversation
There was a problem hiding this comment.
Pull request overview
Updates wolfSSH_shutdown() teardown behavior so it will flush any already-queued outbound bytes left behind by a short send (not only a queued local disconnect), while still preventing post-disconnect traffic unless the only queued output is the local disconnect.
Changes:
- Generalize the shutdown-time “flush queued disconnect” gate into a “flush queued output” gate, with special-casing to suppress non-disconnect output after the peer has disconnected.
- Extend regression coverage for queued output flush scenarios: rejected auth with no channel, channel-close queued after channel retirement, and a live-channel short-send during shutdown.
- Document the broader shutdown flush behavior in the public header.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/ssh.c |
Generalizes queued-output flushing at teardown and in the post-disconnect send path. |
wolfssh/ssh.h |
Expands wolfSSH_shutdown() API comment to describe the broader flush semantics and retry behavior. |
tests/regress.c |
Adds regression tests covering newly supported queued-output flush scenarios and short-send retry behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1219
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 1
Required changes (1)
Completed flush on a live session leaves a stale WS_WANT_WRITE latched
File: src/ssh.c:1207
Function: wolfSSH_shutdown
Category: Incorrect error handling
The stale-error clear is gated on ssh->disconnected. On the paths FlushQueuedOutput() newly enables — a live session with no channel, e.g. a CHANNEL_CLOSE whose channel DoChannelClose() already retired — the flush empties the output buffer while ssh->error stays WS_WANT_WRITE, so wolfSSH_get_error() reports a write owed on an empty buffer.
Related known finding #8817 (similar but distinct): Both are in wolfSSH_shutdown and affect channel-close/shutdown handling, but #8817 faults in the ChannelFind ID-type lookup while this candidate faults in the post-flush error reset condition. Their root causes and affected operations differ, and correcting either requires a separate patch.
Recommendation: Clear the latched WS_WANT_WRITE whenever the flush completed, not only when ssh->disconnected is set.
Referenced code: src/ssh.c:1207-1208 (2 lines)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
5c6cdbf to
1383054
Compare
|
Fixed: a flush that completes on a live session now clears the The cited path does not reproduce, though. On the CHANNEL_CLOSE whose channel |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1219
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 1
Required changes (1)
Stale flushRet overrides a teardown that completed the queued write
File: src/ssh.c:1262
Function: wolfSSH_shutdown
Category: Incorrect error handling
flushRet was only reachable as non-success when the session was disconnected, which nulled channel and skipped every later send. The widened flush now fires on live sessions, where SendChannelEof/SendChannelExit/SendChannelClose each flush the whole output buffer and can drain the flush's leftovers; the unconditional override then returns WS_WANT_WRITE with an empty buffer, masking the real result (e.g. WS_SOCKET_ERROR_E from the close send).
Related known finding #8817 (similar but distinct): Both affect wolfSSH_shutdown teardown behavior, but #8817 faults in channel lookup using a peer ID as a self ID, causing teardown sends to be skipped. This candidate faults in the later unconditional flushRet override after teardown sends may drain output. The root causes and operations differ, and each needs a separate patch.
Recommendation: Gate the override on the flush still being owed, e.g. if (flushRet != WS_SUCCESS && wolfSSH_OutputPending(ssh)), mirroring the flushed guard used for ssh->error.
Referenced code: src/ssh.c:1262-1263 (2 lines)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
1383054 to
e8b93ca
Compare
|
Fixed: the flush's The gate is narrower than the suggested |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1219
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.
e8b93ca to
0500a6c
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1219
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.
0500a6c to
a3eee22
Compare
84f7b86 to
e92b492
Compare
wolfSSH_shutdown() flushes whatever a short send left in the output buffer, not just a queued disconnect. A rejected auth's USERAUTH_FAILURE has no channel, and DoChannelClose() retires the channel as soon as the close is bundled, so neither had anything left to carry the retry. - The gate still refuses a flush once the peer has disconnected, unless our own disconnect is the thing queued. - A flush that finishes on a live session clears the WS_WANT_WRITE the short send latched, as the disconnected path already did. - ssh.h documents the widened flush and the WS_WANT_WRITE a short one leaves wolfSSH_shutdown() returning. - Tests cover the rejected auth with no channel and the close whose channel DoChannelClose() already retired. - A flush that short-sends on a live channel leaves the teardown queued behind it. The retry adds no second EOF or close, and skips the close wait while its own flush is still short. - A teardown send that drained the leftovers settles the flush; one that failed outranks it, since a reset leaves the buffer intact.
e92b492 to
a16726e
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1219
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.
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
wolfSSH_shutdown()flushes whatever a short send left in the output buffer, not just a queued disconnect. A rejected auth'sUSERAUTH_FAILUREhas no channel, and a channel close is retired the moment it is bundled, so neither had anything left to carry the retry.