internal: send LOCAL_CLEANUP on forward delete - #1229
Open
ejohnstown wants to merge 4 commits into
Open
Conversation
WOLFSSH_FWD_LOCAL_CLEANUP is part of the public WS_FwdCbAction contract and the library never sent it, so an application's handler never ran and every peer-opened forward leaked what its setup callback allocated. ChannelDelete() now sends it, so a peer close, an open refused after the setup ran, wolfSSH_ChannelFree(), and freeing the session all report it. - record the setup in a new fwdSetupTxd bit on WOLFSSH_CHANNEL: a locally opened forward gets no LOCAL_SETUP, so the channel type alone cannot say whether the application holds anything - clear the bit when the cleanup goes out, so a channel reports it once - pass the channel's id in the port parameter, the way WOLFSSH_FWD_CHANNEL_ID does, so an application with two forwards can tell which one ended - TestDirectTcpipFwdCbRejectsChannelId now counts three callback calls
WOLFSSH_FWD_LOCAL_CLEANUP now runs, and it runs from DoChannelClose() ahead of the WS_CHANNEL_CLOSED the worker sees. The handler has already closed the socket and moved the state on by then, so the recovery branch no longer matched and left ssh_worker() holding a closed descriptor. - guard the handler's close: the open can fail after the setup, with nothing yet connected - have the recovery branch clear its stale copy of the descriptor when the handler got there first, and still do the whole teardown for a locally opened forward, which draws no callback - clear the pending direct connect as well: it is only cleared on success, so a refused target left it set and the worker connected again with the host name the handler had just freed
The LOCAL_CLEANUP handler closes the target socket, and portfwd_worker() closed its own copy of the same descriptor again at teardown. That was unreachable while the library never emitted the action, which is why the report against it was set aside; it is reachable now. - record the cleanup in the forwarding state and skip the second close - reset the record when a fresh forward's socket is adopted, or one left by an earlier refused open skips closing a live socket at exit - drop the handler comment saying the action is never emitted
WOLFSSH_FWD_LOCAL_CLEANUP now fires, and what must not fire matters as much as what must: the gate is the channel's own setup record, not its type. - a peer-opened direct-tcpip channel reports SETUP, CHANNEL_ID, then CLEANUP when it closes - a forwarding channel this side opened reports no cleanup, and a session channel no forwarding action at all - an open that fails after a successful setup still reports the cleanup, and a setup that reported failure draws none - freeing the session, or the channel with wolfSSH_ChannelFree(), reports it, and the session free after a channel free does not report it again
ejohnstown
requested review from
wolfSSL-Fenrir-bot
and
a lite review from Copilot
September 3, 2026 00:18
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
NotifyFwdLocalCleanup() dereferences ssh->ctx without a NULL check, which is an avoidable potential crash and inconsistent with existing guards in this file.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes missing emission of WOLFSSH_FWD_LOCAL_CLEANUP for peer-opened forwarding channels so applications can reliably tear down resources allocated during WOLFSSH_FWD_LOCAL_SETUP, and updates example handlers/tests to reflect the corrected callback sequence.
Changes:
- Track successful forwarding setup per-channel (
fwdSetupTxd) and emitWOLFSSH_FWD_LOCAL_CLEANUPfromChannelDelete()exactly once when appropriate. - Update example forwarding handlers (echoserver/portfwd) to handle cleanup running and avoid double-closing sockets.
- Add regression coverage asserting when cleanup must fire (and must not) across close/free and open-failure paths.
File summaries
| File | Description |
|---|---|
| wolfssh/internal.h | Adds per-channel fwdSetupTxd bit to gate cleanup emission. |
| src/internal.c | Emits WOLFSSH_FWD_LOCAL_CLEANUP from ChannelDelete() when setup succeeded. |
| tests/regress.c | Adds regression tests validating forwarding callback action sequences. |
| examples/portfwd/portfwd.c | Avoids double-close by latching when cleanup already closed the forwarded socket. |
| examples/echoserver/echoserver.c | Makes cleanup handler resilient to already-closed sockets and clears stale forward state. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+3853
to
+3855
| ssh = channel->ssh; | ||
| if (ssh == NULL || ssh->ctx->fwdCb == NULL) | ||
| return; |
Comment on lines
+3838
to
+3841
| /* Counterpart of the WOLFSSH_FWD_LOCAL_SETUP sent when a forwarding channel | ||
| * was opened, so the application can release what it set up there. Gated on | ||
| * that setup having gone out: a locally opened forward never receives one, | ||
| * and cleaning up after it would free what it does not own. Runs from |
| byte eofTxd : 1; | ||
| byte openConfirmed : 1; | ||
| byte ptyReq : 1; /* flag for if interactive pty request was received */ | ||
| byte fwdSetupTxd : 1; /* a WOLFSSH_FWD_LOCAL_SETUP went out for this one */ |
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.
WOLFSSH_FWD_LOCAL_CLEANUP is part of the public WS_FwdCbAction contract
and the library never sent it, so an application's handler never ran and
every peer-opened forward leaked what its setup callback allocated.
the channel type, and passes the channel id in the port parameter.
stale descriptor and the pending direct connect.
unreachable while the action was never emitted.
session channel draw no cleanup.