wolfterm: bound escBuf saves and clear esc state on OSC resume#1076
Merged
Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1076
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
aidangarske
requested changes
Jul 9, 2026
aidangarske
left a comment
aidangarske
left a comment
Member
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 1 total — 1 posted, 0 skipped
Posted findings
- [High] CSI capacity errors are swallowed on resume —
src/wolfterm.c:684-691
Review generated by Skoll.
aidangarske
self-requested a review
July 9, 2026 07:09
Harden the partial-sequence reassembly in wolfSSH_DoControlSeq and fix an escape-state leak in the wolfSSH_ConvertConsole OSC resume path (USE_WINDOWS_API console handling). - Tighten both escBuf partial-save branches to reject bufSz - *idx >= WOLFSSL_MAX_ESCBUF before the WMEMCPY and log via WLOG. escBuf is WOLFSSL_MAX_ESCBUF bytes, so a save of exactly that length fills it with no room for later state; rejecting the boundary keeps the two branches consistent and matches the resume check. - Add the capacity guard to the initial-parse CSI save branch, which previously copied into escBuf with no bound. In the default build getArgs caps the advance below WOLFSSL_MAX_ESCBUF so the copy was already safe; the guard prevents a custom WOLFSSH_MAX_CONSOLE_ARGS > WOLFSSL_MAX_ESCBUF build from overflowing escBuf. - Clear escState to WC_ESC_NONE when a resumed OSC sequence completes, not just escBufSz. Without this escState stayed WS_ESC_OSC and the next call re-entered OSC parsing on plain bytes instead of printing them. Extend test_wolfSSH_ConvertConsole with a CSI sequence split across three calls (ESC[ | args | command char), a trailing plain byte that must be printed once state is cleared, and a single buffer whose CSI args run to the end and complete on the next byte.
yosuke-wolfssl
changed the title
yosuke-wolfssl
force-pushed
the
fix/f_5903
branch
from
July 21, 2026 01:21
b00bbc8 to
3b5345a
Compare
ejohnstown
approved these changes
Jul 21, 2026
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.
Overview
Hardens the
escBufpartial-sequence reassembly inwolfSSH_DoControlSeqandfixes an escape-state leak in the
wolfSSH_ConvertConsoleOSC resume path(
src/wolfterm.c). Windows-only console handling (USE_WINDOWS_API).Changes
Tighten both
escBufsave branches. When a CSI sequence's arguments runto the end of a buffer with no command char, the partial bytes are stashed in
ssh->escBuffor the next call.escBufisWOLFSSL_MAX_ESCBUFbytes, so asave of exactly that length fills it with no room for continued state. Both
save branches now reject
bufSz - *idx >= WOLFSSL_MAX_ESCBUFbefore theWMEMCPYand log viaWLOG(WS_LOG_ERROR, ...), matching the boundarysemantics of the resume check and keeping the two branches consistent.
Guard the initial-parse CSI save branch. The initial-parse path
(
escStatewasWC_ESC_NONE, buffer opens withESC [) copied intoescBufwith no capacity bound. In the default build
getArgscaps the advance atWOLFSSH_MAX_CONSOLE_ARGS(16) <WOLFSSL_MAX_ESCBUF(19), so the copy wasalready safe; the guard is defense-in-depth for a custom
WOLFSSH_MAX_CONSOLE_ARGS > WOLFSSL_MAX_ESCBUFbuild.Clear escape state on OSC resume completion. When a split OSC sequence
completes via the resume handler in
wolfSSH_ConvertConsole, resetescState = WC_ESC_NONE(not justescBufSz). Without this,escStatestayed
WS_ESC_OSCand the next call re-entered OSC parsing on plain bytesinstead of printing them.
Testing
Extended
test_wolfSSH_ConvertConsoleintests/api.c:ESC [|12|m) — the continuation carriesonly argument bytes; verifies reassembly waits (
WS_WANT_READ) and thencompletes (
WS_SUCCESS) without reading past the buffer.sequence finishes must be printed (
WS_SUCCESS), not swallowed back into anew parse.
ESC [ 1 2) — args run to the end of onebuffer; verifies the partial save waits and completes on the next byte.
Verified on native Windows (Visual Studio Build Tools 2022, static Debug x64):
Local GCC preflight sweep (
--enable-all, Zephyr defines, strncpy lint) clean.