Skip to content

Read the trie header at the view's byteOffset - #1

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

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

Conversation

@openwong2kim

Copy link
Copy Markdown

What

patches/unicode-trie-typescript.diff drops upstream unicode-trie's Buffer header path, leaving every caller on the DataView path:

// upstream unicode-trie
const isBuffer = (typeof data.readUInt32BE === 'function') && (typeof data.slice === 'function');
if (isBuffer) {
  this.highStart = data.readUInt32LE(0);   // respects byteOffset
  ...
} else {
  const view = new DataView(data.buffer);  // does not
}

That isn't equivalent for Buffer inputs. new DataView(buffer) starts at byte 0 of the underlying ArrayBuffer and ignores the view's byteOffset, whereas readUInt32LE is relative to the view.

It matters because 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 in the process leaves the decoded trie at a non-zero byteOffset, and highStart / errorValue / uncompressedLength get read from unrelated bytes.

This passes byteOffset and byteLength to the DataView, restoring what the dropped Buffer path did.

Impact

The generated UnicodeProperties.ts in xterm.js's addon-unicode-graphemes decodes to 3023 bytes — under the 4096 byte pooling threshold, so it is always pooled and always exposed. Depending on where in the pool it lands you get silently wrong character widths (no error raised), Error: Data error out of tiny-inflate at import time, or a multi-gigabyte allocation.

One line repro against the published addon, on node v24.15.0:

$ 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');/* ...same as above... */"
3

Browsers are unaffected — they take the atob branch, which allocates at byteOffset 0.

Why here as well as in xterm.js

xterm.js vendors the output of this patch chain. The same fix is proposed there in xtermjs/xterm.js#6080, but without this change the next regeneration would silently revert it.

Verification

I reproduced the chain locally (patch with unicode-trie-index.diff, then unicode-trie-typescript.diff) and confirmed:

  • regenerating the diff from the unmodified output reproduces the committed patch byte for byte, so the regeneration is faithful;
  • the pre-change output is byte for byte identical to what xterm.js currently ships;
  • the post-change output is byte for byte identical to the fixed file in the xterm.js PR.

The patch is regenerated with plain diff, matching the existing format; the change folds into an existing hunk (3 insertions, 1 deletion).

The upstream unicode-trie constructor has two header paths: a Buffer path
using readUInt32LE, and a DataView path for plain Uint8Arrays. This
patch drops the Buffer path, so every caller now goes through
`new DataView(data.buffer)` -- which ignores the view's byteOffset and
reads from byte 0 of the underlying ArrayBuffer.

That is not equivalent for Buffers. 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. The generated
trie in xterm.js's unicode-graphemes addon decodes to 3023 bytes, well
under the 4096 byte pooling threshold, so it is always pooled.

Pass byteOffset and byteLength so the header is read relative to the
view, restoring the behaviour the dropped Buffer path had.
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