Skip to content

Read the unicode trie header at the view's byteOffset - #6080

Open
openwong2kim wants to merge 1 commit into
xtermjs:masterfrom
openwong2kim:fix/unicode-trie-byteoffset
Open

Read the unicode trie header at the view's byteOffset#6080
openwong2kim wants to merge 1 commit into
xtermjs:masterfrom
openwong2kim:fix/unicode-trie-byteoffset

Conversation

@openwong2kim

@openwong2kim openwong2kim commented Jul 28, 2026

Copy link
Copy Markdown

Fixes #6079.

The defect

UnicodeTrie's constructor reads its 12 byte header with

const view = new DataView(data.buffer);

new DataView(buffer) starts at byte 0 of the underlying ArrayBuffer and ignores the view's byteOffset. A Uint8Array need not own its whole buffer.

In node, Buffer.from(str, 'base64') returns a view into a shared 8 KiB pool whenever the result is smaller than half of Buffer.poolSize. UnicodeProperties.ts decodes the trie exactly that way (_dec), so any earlier small allocation in the process leaves the trie at a non-zero byteOffset, and highStart / errorValue / uncompressedLength are then read from unrelated bytes.

Reproduction

$ node -e "const{Terminal}=require('@xterm/headless');const{UnicodeGraphemesAddon}=require('@xterm/addon-unicode-graphemes');const t=new Terminal({allowProposedApi:true});t.loadAddon(new UnicodeGraphemesAddon());t.unicode.activeVersion='15-graphemes';console.log(t._core.unicodeService.getStringCellWidth('\u{1F468}‍\u{1F469}‍\u{1F467}'))"
2

$ node -e "Buffer.from('x');const{Terminal}=require('@xterm/headless');const{UnicodeGraphemesAddon}=require('@xterm/addon-unicode-graphemes');const t=new Terminal({allowProposedApi:true});t.loadAddon(new UnicodeGraphemesAddon());t.unicode.activeVersion='15-graphemes';console.log(t._core.unicodeService.getStringCellWidth('\u{1F468}‍\u{1F469}‍\u{1F467}'))"
3

The only difference is one small allocation before the addon is loaded. Nothing is thrown. Verified on node v24.15.0 with @xterm/headless 6.0.0 and both @xterm/addon-unicode-graphemes 0.4.0 (latest) and 0.5.0-beta.292 (beta).

Symptoms, worst first

  1. Silently wrong widths, no error at all — the case above. Running this addon's own width vectors in node with a single preceding allocation fails 3 of 13.
  2. Error: Data error thrown out of tiny-inflate while the module is evaluating, so the import fails.
  3. A multi-gigabyte allocation and an apparent hang, when the garbage uncompressedLength happens to be large.

Which one you get depends on where in the pool the trie lands.

Why a regression test, not just the one line

The decoded trie is currently 3023 bytes, against node's 4096 byte pooling threshold (Buffer.poolSize >>> 1) — only 1073 bytes of headroom. If a future Unicode regeneration pushes the trie past 4096 bytes, node stops pooling it, byteOffset becomes 0, and the bug silently disappears, then reappears if the trie later shrinks. The test pins the behaviour independently of the trie's size, so this can't quietly come back.

Why the current tests can't catch it

This addon's tests are Playwright integration tests, and browsers take the atob branch of _dec, which allocates a fresh Uint8Array at byteOffset 0. Browsers are unaffected, and the Playwright tier structurally cannot observe this. The regression test has to run in node.

The test

unicode-trie.test.ts builds a trie image in the serialized format (12 byte header + doubly deflated body), places it at a range of byteOffsets, and asserts the header still reads correctly. It uses DEFLATE "stored" blocks, so it needs no compressor and no node builtins — which also means src/tsconfig.json doesn't need @types/node added.

Without the fix 9 of its 10 cases fail; with it all 10 pass. Both failure classes above are covered — wrong values at small offsets, Data error at larger ones.

A question on placement: I put it beside the file it tests, in src/third-party/. There's plenty of precedent for node unit tests under addons/*/src/ (addon-image, addon-ligatures, addon-search, addon-serialize, addon-webgl), but none inside a third-party/ directory — you may prefer to keep vendored directories test-free. Happy to move it, just say where.

Scope

  • Line 95, this.data = new Uint32Array(data.buffer), looks like the same defect but isn't reachable: tiny-inflate returns either its freshly allocated dest or dest.slice(0, destLen), and both have byteOffset 0 and own their whole buffer. I instrumented the real addon to confirm (byteOffset=0 byteLength=51056 buffer.byteLength=51056). Changing it would be a no-op today, so I left it out to keep this to a single fix. Glad to harden it separately if you'd like it defensive.
  • The same defect is in the patch that generates this vendored file, so the next regeneration would revert this fix. Companion PR: PerBothner/unicode-properties#1. I verified that patch chain reproduces this file byte for byte, before and after.

Verification

  • npm run test-unit — 2413 passing
  • npm run lint — clean
  • npm run build — clean

By contributing this code I agree to license it under xterm.js' MIT license, and I confirm that I have the right to contribute and license it.

The UnicodeTrie constructor read its 12 byte header through
`new DataView(data.buffer)`, which ignores the view's byteOffset and so
starts at byte 0 of the underlying ArrayBuffer.

A Uint8Array need not own its whole buffer. In node,
`Buffer.from(str, 'base64')` returns a view into a shared 8 KiB pool
whenever the result is smaller than half of Buffer.poolSize, so any
earlier small allocation leaves the decoded trie at a non-zero
byteOffset and the header is read from unrelated bytes.

Pass byteOffset and byteLength so the header is read relative to the
view. Adds a unit test that builds a trie image at a range of
byteOffsets.
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.

addon-unicode-graphemes: trie header is read via DataView(data.buffer) ignoring byteOffset, so a pooled base64 decode silently corrupts widths

1 participant