internal: send LOCAL_CLEANUP on forward delete - #1229
Conversation
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.
2a720f9 to
7621086
Compare
|
The force push carries one change beyond the review comments above, in The double-close fix covered a single ordering: In reverse mode the worker adopts the callback's socket, so Teardown now clears the shared copy when it is the one doing the closing. The comparison keeps that to the socket the callback owns; a descriptor from Like the first ordering, this is reachable only because the library now emits the action, so it is not a regression against master. It is not exercised end to end here: |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1229
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.
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 - gate the handler on the channel id the library passes in the port parameter. A channel can outlive its turn in the single forwarding slot, and a cleanup arriving after the next forward has moved in would close that one's live socket - 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 - resolve the closed channel with wolfSSH_GetLastRxId(). wolfSSH_worker() names the channel only for the data and EOF statuses, so the recovery branch was comparing against a stale zero and ran only for a forward that happened to be channel 0 - 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 - clear the shared descriptor when the exit path is the one that closes it: the loop can leave with the channel still open, and freeing the session then runs the handler on a descriptor already closed - 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
7621086 to
486d9eb
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1229
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_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.