Skip to content

fix: clamp saturated CSI cursor-move counts before the add (int overflow) - #27

Merged
pocketprobe merged 3 commits into
DivByDiamond:workfrom
pocketprobe:fix/work/cursor-arg-overflow
Aug 26, 2026
Merged

fix: clamp saturated CSI cursor-move counts before the add (int overflow)#27
pocketprobe merged 3 commits into
DivByDiamond:workfrom
pocketprobe:fix/work/cursor-arg-overflow

Conversation

@pocketprobe

@pocketprobe pocketprobe commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Fixes an int-overflow class in the relative CSI cursor-move handlers: a saturated CSI count (parseArgument caps at Integer.MAX_VALUE) was added to terminal.x/terminal.y before setClampedCursorPos clamped the result, so the int addition wrapped negative and the clamp landed the cursor on 0 (the near edge) instead of the far edge. Same root cause as the SU/SD freeze (§36 Б2, fixed by clamping in CH8/CH9), but a correctness misposition rather than a DoS.

What this does

  • New Terminal.moveCursorBy(dx, dy) bounds the delta to ±screen extent before the add; setClampedCursorPos still applies the screen + scroll-region clamp to the result, so scroll-region behavior is unchanged (traced scrollFirst=5/scrollLast=10: from y=8 a saturated down-move now hits scrollLast=10, where it hit scrollFirst=5 before). setClampedCursorPos can't be the clamp site — it takes an absolute target, not a delta, so the overflow has already happened before it runs.
  • CUD (B), CUF (C), VPR (e), HPR (a) — the additive movers — now go through moveCursorBy. VPR/HPR are new in PR fix: missing CSI cursor escapes + unify cursor save/restore (SavedCursor) #24 and had inherited the bug.
  • CUU (A), CUB (D) — routed through moveCursorBy for uniformity. They subtract, so they never overflowed; verified bit-identical for in-range args and equivalent for huge args (both clamp to the near edge). This is the only non-buggy code touched, kept so all four cardinal directions share the one primitive.
  • CNL (E) — stays per-handler: it resets the column to 0, which a delta-preserving move can't express; inline Math.clamp on the row delta. CPL (F) is untouched (subtraction + column reset; safe).
  • setRelativeCursorPos under DECOM — a separate overflow the hunt surfaced in the same pass: it did scrollFirst + y with the raw arg, and CUP/HVP/VPA feed the row here, so a saturated row overflowed scrollFirst + MAX_VALUE negative and clamped to scrollFirst (top) instead of scrollLast (bottom). Now bounds y to [0, scrollLast - scrollFirst] before the add, preserving origin-relative semantics (row 1 = scrollFirst). Distinct from §36 m1 / §37 Б6 — the saved-cursor-after-width-change AIOOBE that PR fix: missing CSI cursor escapes + unify cursor save/restore (SavedCursor) #24's SavedCursor refactor already resolved.

Verification

  • 9 new tests in TerminalBufferTest, 100/0. Six revert-and-fail (CUD/CUF/VPR/HPR/CNL/DECOM) feed CSI 2147483647 from a non-zero start and assert the far edge, not 0; CUU/CUB are behavior-preservation for the routing; moveCursorBySaturatedDownInScrollRegionClampsToScrollLast covers the scroll-region case (saturated down-move from inside a region lands on scrollLast, exercising the two-stage delta-then-region clamp). Each pairs a normal small move with the saturated one so the pre-fix path is exercised before the overflow path.
  • Full test suite green (./gradlew test).
  • Verified against upstream/work (760bc44): the three + args[0] sites PR fix: missing CSI cursor escapes + unify cursor save/restore (SavedCursor) #24 added (VPR/HPR/CNL) were the inherited regressions; the two pre-existing ones (CUD/CUF) predate fix: missing CSI cursor escapes + unify cursor save/restore (SavedCursor) #24.

QA gate

  • Checkstyle 0, PMD 0 in touched files. The 5 checkstyle-main errors and 41 pmd-main warnings are pre-existing in unrelated files (inet session imports, TunnelManager).
  • SpotBugs delta 0 — no BugInstance at moveCursorBy/setRelativeCursorPos; the primitive takes primitives and calls existing methods, adding no EI_EXPOSE_REP2/MS_*/PA_* findings.

Notes for review

  • The judgment call is routing CUU/CUB through moveCursorBy when they don't strictly need the fix — it's a uniformity win (all four cardinal directions share one bounded primitive, so a future handler can't forget the clamp) at the cost of touching two files that weren't broken. Happy to pull them back if you'd rather keep this to the five broken handlers only.

Next

  • A dead-code pass is due soon: §36 m11 (Utf8Decoder.hasActiveSequence 0 refs, TerminalIO.putOutput(byte), Terminal.getTerminalWidth() tests-only, the never-called incrementLastLineToDisplay(true) branch, TerminalRenderer.findLineIndex/isPrintableCharacter → private, ImplementedPrivateModes.modeStatus public-mutable → private) plus §37's ColorUtils, RunnableUtils.doNothing, SessionOperator. Separate PR.

Assisted by: GLM (syn:large:text on synthetic.new) — code generation and review.

pocketprobe added a commit to pocketprobe/OC2R that referenced this pull request Aug 26, 2026
)

Loki's review: the y=8 -> scrollLast case traced in the PR body wasn't
covered by the 8 overflow tests. Add moveCursorBySaturatedDownInScroll-
RegionClampsToScrollLast: from inside a region [5..10] (0-indexed 4..9) at
y=6, a saturated CUD (down) must land on scrollLast (9) — exercising the
two-stage clamp (delta bound in moveCursorBy, region clamp in
setClampedCursorPos). Revert-and-fail spot-checked: against the pre-fix
raw-add CUD the test fails with "expected: 9 but was: 4" (the int sum
overflows negative and the region clamp lands on scrollFirst). 100/0.

Assisted by: GLM (syn:large:text on synthetic.new) — code generation and review.
…low)

EscapeUtilities.parseArgument saturates at Integer.MAX_VALUE, and the
relative cursor-move handlers added the saturated arg to terminal.x/y
BEFORE setClampedCursorPos clamped the result. The int addition overflowed
to a negative value, which setClampedCursorPos then faithfully clamped to
0 (the near edge) instead of the far edge — so a guest sending
CSI 2147483647C from column 5 moved the cursor to column 0, not the right
margin. Same bug class as the SU/SD freeze (§36 B2, fixed by clamping in
CH8/CH9), but a correctness misposition rather than a DoS.

setClampedCursorPos can't be the clamp site: it takes an absolute target,
not a delta, so the direction information (and the overflow) has already
happened before it runs. Add Terminal.moveCursorBy(dx, dy), which bounds
the delta to +/- screen extent before the add; setClampedCursorPos still
applies the screen and scroll-region clamp to the result, so scroll-region
behavior is unchanged (traced scrollFirst=5/scrollLast=10: from y=8 a
saturated down-move now hits scrollLast=10, where it hit scrollFirst=5
before).

Affected handlers:
- CUD (B), CUF (C), VPR (e), HPR (a): the additive movers, now via
  moveCursorBy. VPR/HPR are new in PR DivByDiamond#24 and had inherited the bug.
- CUU (A), CUB (D): routed through moveCursorBy for uniformity. They
  subtract, so they never overflowed — verified bit-identical for in-range
  args and equivalent for huge args (both clamp to the near edge).
- CNL (E): stays per-handler — it resets the column to 0, which
  moveCursorBy (delta-preserving) can't express; inline Math.clamp on the
  row delta. CPL (F) is untouched (subtraction + column reset; safe).

Separate overflow found in the same hunt: setRelativeCursorPos under
DECOM did scrollFirst + y with the raw arg. CUP/HVP/VPA pass the row here,
so a saturated row overflowed scrollFirst + MAX_VALUE to a negative int
and clamped to scrollFirst (top) instead of scrollLast (bottom). Bounds y
to [0, scrollLast - scrollFirst] before the add, preserving origin-relative
semantics (row 1 = scrollFirst). Distinct from §36 m1 / §37 B6 (the
saved-cursor-after-width-change AIOOBE PR DivByDiamond#24 fixed via SavedCursor).

Tests: 8 in TerminalBufferTest. Six revert-and-fail (CUD/CUF/VPR/HPR/CNL/
DECOM) assert a saturated count from a non-zero start lands on the far
edge, not 0; CUU/CUB are behavior-preservation for the routing. 99/0.

QA: compileJava + full test suite green; Checkstyle 0, PMD 0 in touched
files; SpotBugs delta 0 (no BugInstance at moveCursorBy/setRelativeCursorPos;
no new EI_EXPOSE_REP2/MS_* — the primitive takes primitives and calls
existing methods). The 5 checkstyle-main / 41 pmd-main warnings are
pre-existing in unrelated files (inet session imports, TunnelManager).

Assisted by: GLM (syn:large:text on synthetic.new) — code generation and review.
These four pre-existing handlers predate PR DivByDiamond#24 and were missing the
final newline that VPR/HPR/CNL (and the rest of the csi/ package) have.
Pure EOF hygiene; no content change.

Assisted by: GLM (syn:large:text on synthetic.new) — code generation and review.
)

Loki's review: the y=8 -> scrollLast case traced in the PR body wasn't
covered by the 8 overflow tests. Add moveCursorBySaturatedDownInScroll-
RegionClampsToScrollLast: from inside a region [5..10] (0-indexed 4..9) at
y=6, a saturated CUD (down) must land on scrollLast (9) — exercising the
two-stage clamp (delta bound in moveCursorBy, region clamp in
setClampedCursorPos). Revert-and-fail spot-checked: against the pre-fix
raw-add CUD the test fails with "expected: 9 but was: 4" (the int sum
overflows negative and the region clamp lands on scrollFirst). 100/0.

Assisted by: GLM (syn:large:text on synthetic.new) — code generation and review.
@pocketprobe
pocketprobe force-pushed the fix/work/cursor-arg-overflow branch from 7c75838 to fba18be Compare August 26, 2026 22:03
@pocketprobe
pocketprobe merged commit 78c733a into DivByDiamond:work Aug 26, 2026
1 check passed
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.

1 participant