Skip to content

util: strip whole CSI sequences per ECMA-48 - #65379

Open
luantaraschi wants to merge 1 commit into
nodejs:mainfrom
luantaraschi:util-csi-ecma48
Open

util: strip whole CSI sequences per ECMA-48#65379
luantaraschi wants to merge 1 commit into
nodejs:mainfrom
luantaraschi:util-csi-ecma48

Conversation

@luantaraschi

Copy link
Copy Markdown

stripVTControlCharacters() recognises a control sequence by a hand written list of final bytes, inherited from the bundled copy of ansi-regex:

'|(?:(?:\\d{1,4}(?:;\\d{0,4})*)?' +
'[\\dA-PR-TZcf-nq-uy=><~]))', 'g',

The list is incomplete in two directions. It misses parameter bytes such as <, and it misses final bytes such as @, X, d, a and b. When a sequence uses one of them the match ends in the middle of it and the rest is left in the string. The function does not fail to strip, it strips the wrong bytes.

Measured on main at e2b33e2:

input today expected
a ESC [ < 35 ; 10 ; 20 M b a35;10;20Mb ab
a ESC [ > 1 ; 2 c b a1;2cb ab
a ESC [ 3 @ b a@b ab
a ESC [ 3 X b aXb ab
a ESC [ 5 d b adb ab
a ESC [ 2 SP q b a qb ab
a ESC [ ! p b unchanged ab
a ESC [ 38 : 2 : 255 : 0 : 0 m b a:2:255:0:0mb ab
a ESC [ 4 : 3 m b a:3mb ab

The first row is an SGR mouse report, which is what a terminal sends back on every click once a program has enabled mouse reporting. Rows six and seven carry intermediate bytes: DECSCUSR sets the cursor style and DECSTR is a soft reset, both emitted by common shells and editors. The last two use : to separate sub parameters, which is how truecolour and curly underlines are written by editors that follow the newer form.

This is not limited to the public API. getStringWidth() strips before measuring and lib/internal/readline/interface.js places the cursor from that width, so the leftovers are counted as printable columns:

getStringWidth(ESC [ < 35 ; 10 ; 20 M)  ->  9   (expected 0)
getStringWidth(ESC [ > 1 ; 2 c)         ->  4   (expected 0)
getStringWidth(ESC [ 2 SP q)            ->  2   (expected 0)

The change

ECMA-48 5.4 defines a control sequence as CSI, then any number of parameter bytes (0x30-0x3F), then any number of intermediate bytes (0x20-0x2F), then a single final byte (0x40-0x7E). This adds that as an alternative and leaves both existing alternatives in place:

  • the string terminator alternative still runs first, so OSC 8 hyperlinks and anything else ending in BEL, ESC \ or 0x9C match exactly as before;
  • the hand written list still runs last, because it also matches sequences whose last byte is a digit, such as ESC 7 and ESC 8, which the ECMA-48 alternative does not.

The new alternative is only reached where the string terminator alternative does not match, so the change can extend a match but never shorten one. I checked that rather than assuming it. Over 400000 pseudo random strings built from introducers, parameter bytes, intermediate bytes, final bytes, string terminators and ordinary text, 10640 come out different from main, and in zero of them does the new pattern leave more behind than the current one.

Two shapes I tried first, and why they are not here

Both were discarded on measurements, and both are easy to arrive at, so they seem worth recording.

Sharing the existing introducer prefix makes the match quadratic. The prefix [[\]()#;?]* also consumes ;, so it and the parameter byte class compete for the same run and every split is retried. On ESC [ followed by n semicolons and no final byte that shape took 411 ms at n = 20000 and 2.6 s at n = 50000. Giving the new alternative its own introducer, with a prefix that excludes ; and ?, keeps it linear: 200000 semicolons take 0.9 ms on main and 1.2 ms here.

Putting the new alternative first is faster and wrong. With the control sequence alternative ahead of the string terminator one, the common ESC [ ... m is matched immediately and the whole thing gets faster than main, by about 70% on the corpora below. It also changes the result for 226 of those 400000 strings, because ESC [ followed by text and a string terminator is matched today by the string terminator alternative and would then be cut short. I would rather be slower than change what those return, but if you disagree the reordering is a two line difference and I am happy to switch.

Cost

Measured with the built binary inside a quiet Linux container, minimum of 41 rounds of 50 iterations, three independent runs, reported as the range across them:

corpus size main this branch
no escape sequences 58 KB 0.013 ms 0.013 ms
SGR colour only 42 KB 0.128 to 0.131 ms 0.163 to 0.166 ms (+26% to +28%)
terminal output with cursor moves 63 KB 0.321 to 0.324 ms 0.372 to 0.380 ms (+16% to +18%)

Text with no escape sequences is unaffected, which is the short circuit at the top of stripVTControlCharacters() doing its job. Text that does contain them costs one extra alternative per match. In absolute terms it is tens of microseconds per 40 to 60 KB, but it is a real regression and I would rather put the number here than have it found in review.

Testing

test/parallel/test-util-stripvtcontrolcharacters.js gains thirteen cases, one per sequence family above. I verified them in both directions on a real binary rather than by reasoning about the pattern: reverting only lib/internal/util/inspect.js, rebuilding and running the file gives

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected
+ 'a35;10;20Mb'
- 'ab'

and it passes with the change restored. None of the thirteen passed before. The eleven cases already in the file are untouched.

Built with GCC 14 on Debian trixie, v27.0.0-pre. python3 tools/test.py -J parallel gives 4719 passing and 3 failing: test-permission-drop-ffi, and test-cluster-primary-error and test-cluster-primary-kill on timeout. None of the three mentions stripVTControlCharacters or getStringWidth, and all three fail the same way on a binary built from the reverted lib/internal/util/inspect.js, so they are my container and not this change.

The twelve files that do reach this regex through getStringWidth() all pass: test-icu-stringwidth, test-util-inspect, test-util, the four readline files, test-repl-multiline, test-repl-highlight, test-util-inspect-regexp, test-runner-run and the stripping test itself.

Note on #64319

#64319 is open and approved against the same regex, rewriting the OSC alternative so hyperlinks with ( in the URI strip correctly. They are mostly disjoint, that one being about the string terminator alternative and this one about control sequences, but they do meet in one place and it seems better to say so than to let it be found: #64319 also fixes the : sub parameter case, by adding [;:] to the hand written list. This change fixes it too, as a consequence of : being a parameter byte in the grammar rather than by extending the list. I checked on the built binary that the two cases #64319 adds for the OSC alternative, and the reproduction in #64313, return exactly the same string with this change applied. If #64319 lands first I will rebase on it.

Disclosure: I used an AI coding assistant while working on this, as a research and drafting aid. The analysis, the choice of the ECMA-48 grammar, the two discarded shapes and every measurement above are mine and I ran them myself. I can explain any line of this change in review.

stripVTControlCharacters() recognises a control sequence by a hand
written list of final bytes, inherited from the bundled copy of
ansi-regex. The list covers neither parameter bytes such as `<` and
`:`, used by mouse reports and by sub parameters, nor final bytes such
as `@`, `X`, `d`, `a` and `b`. A sequence that uses one of them
matches only in part, so the rest of it is left in the string rather
than removed.

getStringWidth() strips before measuring and readline places the cursor
from that width, so the leftovers are counted as printable columns.

Add an alternative built from the control sequence structure in
ECMA-48 5.4: any number of parameter bytes (0x30-0x3F), any number of
intermediate bytes (0x20-0x2F), then a single final byte (0x40-0x7E).
Both existing alternatives are kept and the new one is only reached
where they do not match, so a match can be extended but never shortened.

The new alternative carries its own introducer instead of sharing the
existing prefix, because the shared prefix also consumes `;` and the
two competing for the same run made the match quadratic.

Signed-off-by: Luan Taraschi <130802253+luantaraschi@users.noreply.github.com>
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants