fix(vt): bound margins and repeat counts to the screen - #956
Open
Rohilalala wants to merge 1 commit into
Open
Conversation
Several sequences took a number straight from the byte stream and used it to
index the buffer or to size a loop. The bytes are whatever the program in the
terminal emits, so a few of them were enough to take the host process down or
hold it still:
\x1b[?69h\x1b[2;4s\x1b[S on 2 columns -> index out of range
\x1b[1;9r\x1b[S on 2 rows -> index out of range
\x1b[888888889X -> 1.5s
\x1b[888888889I and ...Z -> 2.6s each
a\x1b[888888889b -> minutes
DECSTBM and DECSLRM are now clamped where the region is stored rather than
where it is parsed, which keeps every sequence accepted or rejected exactly as
before while the region itself stays inside the buffer. It is also kept at
least one row and column wide: an empty region parked past the last line is in
bounds but nothing can scroll within it, and every line printed afterwards
comes out wrong.
ECH stops at the right edge, since it cannot erase a column the line does not
have. The tab loops stop as soon as the cursor does not move: past the last tab
stop the position stops changing, and they were counting down the parameter
regardless. REP is capped at a screenful plus the remainder of the count over
the width — past a full screen the result repeats every Width characters, so
that remainder leaves the screen and the cursor exactly where writing every
repeat would have, which the tests check against actually writing them.
Found while fixing charmbracelet#935; the panic was reproducible on main and reachable from
any program's output, including a file someone else wrote.
Rohilalala
force-pushed
the
fix/vt-margin-bounds
branch
from
August 24, 2026 16:52
d4953d0 to
a92d5f1
Compare
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.
Several sequences take a number straight from the byte stream and use it to index the buffer or to size a loop. The bytes are whatever the program in the terminal emits, so a few of them are enough to take the host process down or hold it still. On
main:A program embedding
vtto render another program's output — which is the point of it — can be crashed or wedged by a handful of bytes, including from a file someone else wrote.I found these while working on #935; the panic was surfaced by a reviewer on that branch.
The margins
DECSTBMandDECSLRMare clamped where the region is stored rather than where it is parsed. That keeps every sequence accepted or rejected exactly as before, while the region itself stays inside the buffer. Clamping at parse time instead breaksTestTerminal/CUP_Relative_to_Origin, which relies on\x1b[2;3rbeing accepted on a two-row screen.The region is also kept at least one row and one column wide. An empty region parked past the last line is technically in bounds but nothing can scroll within it, and every line printed afterwards comes out wrong —
\x1b[9;10ron a 6x3 screen produced exactly that.The counted sequences
ECHstops at the right edge; it cannot erase a column the line does not have.CHT,CBT) stop as soon as the cursor does not move. Past the last tab stop the position stops changing and they were counting down the parameter regardless.REPis capped at a screenful plus the remainder of the count over the width. A plain screenful cap is observable — the cursor ends in the wrong column — because past a full screen the result repeats everyWidthcharacters.TestRepeatCountMatchesTheWholeCountchecks the capped result against actually writing the characters.Not addressed
CSI 0 brepeats zero times where ECMA-48 makes an omitted or zero parameter mean one. Pre-existing, and a separate question from bounding a large one.Tests
TestMarginsOutsideThePagedrives each panic sequence and then asserts the scroll region is a region inside the buffer — the weaker "not out of bounds" check passes on the empty-region case, so it checks both.TestCountedSequencesAreBoundedgives each counted sequence a budget several orders of magnitude above what it needs once the work is bounded by the screen rather than the count.gofmt,go vet,golangci-lint --config ../.golangci.yml(matchesmain's two pre-existing findings),go test ./... -race -count=3 -shuffle=on, and 90s of fuzzing for further panics with no new findings.