Read the unicode trie header at the view's byteOffset - #6080
Open
openwong2kim wants to merge 1 commit into
Open
Conversation
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.
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.
Fixes #6079.
The defect
UnicodeTrie's constructor reads its 12 byte header withnew DataView(buffer)starts at byte 0 of the underlyingArrayBufferand ignores the view'sbyteOffset. AUint8Arrayneed 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 ofBuffer.poolSize.UnicodeProperties.tsdecodes the trie exactly that way (_dec), so any earlier small allocation in the process leaves the trie at a non-zerobyteOffset, andhighStart/errorValue/uncompressedLengthare then read from unrelated bytes.Reproduction
The only difference is one small allocation before the addon is loaded. Nothing is thrown. Verified on node v24.15.0 with
@xterm/headless6.0.0 and both@xterm/addon-unicode-graphemes0.4.0 (latest) and 0.5.0-beta.292 (beta).Symptoms, worst first
Error: Data errorthrown out of tiny-inflate while the module is evaluating, so the import fails.uncompressedLengthhappens 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,byteOffsetbecomes 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
atobbranch of_dec, which allocates a freshUint8ArrayatbyteOffset0. Browsers are unaffected, and the Playwright tier structurally cannot observe this. The regression test has to run in node.The test
unicode-trie.test.tsbuilds a trie image in the serialized format (12 byte header + doubly deflated body), places it at a range ofbyteOffsets, and asserts the header still reads correctly. It uses DEFLATE "stored" blocks, so it needs no compressor and no node builtins — which also meanssrc/tsconfig.jsondoesn't need@types/nodeadded.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 errorat 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 underaddons/*/src/(addon-image, addon-ligatures, addon-search, addon-serialize, addon-webgl), but none inside athird-party/directory — you may prefer to keep vendored directories test-free. Happy to move it, just say where.Scope
this.data = new Uint32Array(data.buffer), looks like the same defect but isn't reachable: tiny-inflate returns either its freshly allocateddestordest.slice(0, destLen), and both havebyteOffset0 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.Verification
npm run test-unit— 2413 passingnpm run lint— cleannpm run build— cleanBy 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.