Skip to content

echoserver: close the agent socket on reset and handle a rekey - #1209

Merged
philljj merged 2 commits into
wolfSSL:masterfrom
ejohnstown:ccb-phase1-5
Aug 28, 2026
Merged

echoserver: close the agent socket on reset and handle a rekey#1209
philljj merged 2 commits into
wolfSSL:masterfrom
ejohnstown:ccb-phase1-5

Conversation

@ejohnstown

@ejohnstown ejohnstown commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

The worker drops back to APP_STATE_LISTEN when an agent connection ends, but never closes the socket. The next accept() overwrites agentFd, so every agent connection after the first leaks the previous descriptor. The forward path has the same gap on its connection-reset arm, where the socket is closed but fwdFd keeps the closed number.

A rekey was treated as a read failure and ended the session. It cannot just be skipped either: wolfSSH_worker() reports WS_REKEYING in place of WS_CHAN_RXD while keying, and nothing raises the data report again, so ignoring it strands whatever arrived in that call and the peer waits on an answer that never comes. This is the hazard the library already calls out for WS_EXTDATA, which is exempted from the same override.

  • close agentFd and clear it on both the read-zero and the ECONNRESET/ECONNABORTED arms
  • clear fwdFd on the forward reset arm, matching the read-zero arm
  • clear agentCtx.appFd and fwdCtx.appFd wherever the worker closes the socket, so the stored copy cannot outlive the descriptor
  • drain the channel on WS_REKEYING as well as WS_CHAN_RXD, and take an empty read as "nothing buffered" rather than a failure on that path. wolfSSH_ChannelIdRead() has no isKeying gate and the window credit it owes is parked until the rekey completes

Example program only. The echoserver's send paths still treat WS_REKEYING as fatal; that needs a held-output buffer and is left for separate work.

The worker drops back to APP_STATE_LISTEN when an agent connection ends,
but never closes the socket. The next accept() overwrites agentFd, so
every agent connection after the first leaks the previous descriptor. The
forward path has the same gap on its connection-reset arm, where the
socket is closed but fwdFd keeps the closed number.

A rekey was treated as a read failure and ended the session. It cannot
just be skipped either: wolfSSH_worker() reports WS_REKEYING in place of
WS_CHAN_RXD while keying, and nothing raises the data report again, so
ignoring it strands whatever arrived in that call and the peer waits on
an answer that never comes. This is the hazard the library already calls
out for WS_EXTDATA, which is exempted from the same override.

- close agentFd and clear it on both the read-zero and the
  ECONNRESET/ECONNABORTED arms
- clear fwdFd on the forward reset arm, matching the read-zero arm
- clear agentCtx.appFd and fwdCtx.appFd wherever the worker closes the
  socket, so the stored copy cannot outlive the descriptor
- drain the channel on WS_REKEYING as well as WS_CHAN_RXD, and take an
  empty read as "nothing buffered" rather than a failure on that path.
  wolfSSH_ChannelIdRead() has no isKeying gate and the window credit it
  owes is parked until the rekey completes
Copilot AI lite review requested due to automatic review settings August 27, 2026 23:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes correctness issues in the echoserver example’s multiplexing loop by preventing socket descriptor leaks when agent/forward connections reset/close, and by correctly draining buffered channel data during SSH rekeying so sessions don’t stall.

Changes:

  • Close and invalidate agentFd on read-0 and reset/abort paths; also clear threadCtx->agentCtx.appFd.
  • Clear fwdFd on the forward reset/abort path (matching the read-0 path) and clear threadCtx->fwdCtx.appFd wherever the socket is closed.
  • Treat WS_REKEYING like WS_CHAN_RXD for purposes of draining per-channel buffered data, and treat an empty drain during rekey as non-fatal.
Suppressed comments (2)

examples/echoserver/echoserver.c:1062

  • Same issue as the shell-path: cnt_r <= 0 treats negative wolfSSH_ChannelIdRead() results as “nothing buffered” during WS_REKEYING, which can hide real errors.
                            if (cnt_r <= 0) {
                                /* Nothing was buffered. Only an actual data
                                 * report makes that a failure. */
                                if (rc == WS_REKEYING)
                                    continue;

examples/echoserver/echoserver.c:1086

  • Same issue as the other WS_REKEYING drain paths: cnt_r <= 0 will ignore negative wolfSSH_ChannelIdRead() return values during rekey, which can mask real failures.
                            if (cnt_r <= 0) {
                                /* Nothing was buffered. Only an actual data
                                 * report makes that a failure. */
                                if (rc == WS_REKEYING)
                                    continue;

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread examples/echoserver/echoserver.c
wolfSSH_ChannelIdRead() returns a negative value for a real error, and
the rekey arm treated that the same as a zero read. Restrict the
continue to cnt_r == 0 so an error still ends the loop.

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #1209

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.

@ejohnstown
ejohnstown requested a review from philljj August 28, 2026 22:31
@philljj
philljj merged commit ad059d7 into wolfSSL:master Aug 28, 2026
166 checks passed
@ejohnstown
ejohnstown deleted the ccb-phase1-5 branch August 28, 2026 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants