Guard the Espressif echoserver forward cleanup - #1255
Open
ejohnstown wants to merge 4 commits into
Open
ejohnstown wants to merge 4 commits into
ejohnstown wants to merge 4 commits into
Conversation
ejohnstown
requested review from
wolfSSL-Fenrir-bot
and
a lite review from Copilot
September 15, 2026 17:47
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1255
No scan targets match the changed files in this PR. Review skipped.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The cleanup path can leave the worker's forwarding descriptor open.
Pull request overview
Guards Espressif echoserver forwarding cleanup against stale channels and invalid descriptors.
Changes:
- Validates the channel ID before cleanup.
- Guards socket closing and resets forwarding state.
File summaries
| File | Review |
|---|---|
ide/Espressif/ESP-IDF/examples/wolfssh_echoserver/main/echoserver.c |
Moderate issue (1 vote): appFd is not synchronized with the worker's fwdFd, so successful direct connections may remain open during cleanup. |
Review details
Suppressed comments (1)
ide/Espressif/ESP-IDF/examples/wolfssh_echoserver/main/echoserver.c:519
appFdis never populated by this Espressif file's direct-connect path (that path stores the successful socket only in the worker-localfwdFd), so this condition is false for every successful direct-tcpip connection. Once this cleanup callback is live,fwdFdis left open; please make the direct path and all close paths keepappFdin sync, or have cleanup close the worker's descriptor as well.
if (ctx->appFd != (WS_SOCKET_T)-1) {
WCLOSESOCKET(ctx->appFd);
ctx->appFd = -1;
}
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 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 #1255
No scan targets match the changed files in this PR. Review skipped.
WOLFSSH_FWD_LOCAL_CLEANUP now runs, so this echoserver's handler for it runs too. It closes only the socket belonging to the channel that ended, and only when one was 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 late would close the next one's socket - guard the close: the open can fail after the setup, with nothing yet connected and appFd still -1
The cleanup handler closes the descriptor the context holds, and only the accept path recorded one, so a direct forward's socket was left open when its channel closed. The direct path now records it, and the worker's recovery branch and the handler no longer race for the teardown. - record the connected socket on the direct path, as the accept path does - resolve the closed channel with wolfSSH_GetLastRxId(). wolfSSH_worker() names the channel only for the data and EOF statuses, so the recovery branch compared against a stale zero - let the recovery branch clear its stale copy when the handler got there first, and still tear down a locally opened forward, which draws no callback
Recording the direct forward's socket in the context left the two sites that close it on the target's own EOF or reset holding a stale copy, so the cleanup handler closed a descriptor number the task had since reissued. Every close now retires both the worker's copy and the context's, as the reference example does. - clear appFd where a zero read ends the forward - clear appFd and the worker's own fwdFd on a reset, which that branch never reset - compare appFd against a cast -1 in both places, since WS_SOCKET_T is unsigned on Windows
A weekly scheduled job builds both ESP-IDF example projects, so a break in the Espressif port surfaces on its own instead of waiting for a user to report it. - compileAllExamples.sh builds every project under examples/, pointing the local components at WOLFSSH_ROOT and WOLFSSL_ROOT - the workflow covers ESP-IDF release-v5.5 and release-v5.1, built against wolfSSL v5.9.1-stable - a scheduled failure opens or comments on an issue labelled espressif-build-failure
ejohnstown
force-pushed
the
espressif-fwd-guard
branch
from
September 17, 2026 18:44
f4a9af8 to
3c2eb6c
Compare
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Reset fwdBufferIdx on teardown and other forward-close paths to prevent stale buffered data reuse.
Review details
Suppressed comments (1)
ide/Espressif/ESP-IDF/examples/wolfssh_echoserver/main/echoserver.c:1169
fwdBufferIdxis not cleared when this cleanup path invalidates the descriptor. If target data was read but still pending infwdBuffer(for example while the channel was not yet confirmed), a later forward reuses those bytes and sends them on the new channel. ResetfwdBufferIdxon this teardown and the other forward-close paths below.
fwdFd = -1;
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
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_CLEANUPnow reaches applications, so the Espressif echoserver's handler for it runs where it used to be dead code. It was the only copy in the tree left unguarded.