Skip to content

Commit c54051e

Browse files
authored
fix(native): reject typed arrays other than Uint8Array in feed (#49)
## Why The goal is to handle the items still open after the #12 follow-ups and land each on main as its own PR. This one fixes the raw addon's `feed`. `native/README.md` says the addon's `feed` throws `TypeError("feed typed array must be Uint8Array")` for a typed array that is not a `Uint8Array`. It throws `TypeError("feed data must be a string, Buffer, or Uint8Array")` for any other value. The typed-array check in `TerminalWrap::Feed` never ran. The `IsBuffer()` branch before it is true for every ArrayBufferView in Node, because `napi_is_buffer` accepts any view. So a `Uint16Array` or `Float32Array` was written as raw bytes, and a `DataView` failed with `Error: Invalid argument`. ## What changed - `native/terminal.cc`: I deleted the `IsBuffer()` branch in `TerminalWrap::Feed`. A Buffer is a `Uint8Array`, so the typed-array branch now handles it. - `test/terminal.test.ts`: new raw-addon tests. `Uint16Array`, `Float32Array` and `Uint8ClampedArray` throw the typed-array `TypeError`. A `DataView` throws the generic `TypeError`. `Buffer`, `Uint8Array` and a Buffer slice with a byte offset still write "AB". - `native/README.md`: the Verification section no longer lists the typed-array errors as untested. ## Scope Only the raw addon's `feed` argument check changes. `src/index.ts` already rejects these values before native code runs, so the public API keeps its behavior. ## Blast Radius Code that calls the raw addon directly with a non-`Uint8Array` typed array or a `DataView` now gets a `TypeError`. Before, it got raw bytes written or `Error: Invalid argument`. Package users who go through `createTerminal` from the package see no change. ## Verification The test commit (1727d01) fails on the unfixed code: ``` × rejects a Uint16Array in the addon feed with a TypeError 5ms × rejects a Float32Array in the addon feed with a TypeError 1ms × rejects a Uint8ClampedArray in the addon feed with a TypeError 0ms × rejects a DataView in the addon feed with a TypeError 1ms ✓ writes a Buffer in the addon feed 0ms ✓ writes a Uint8Array in the addon feed 0ms ✓ writes a Buffer slice with a byte offset in the addon feed 0ms ⎯⎯⎯⎯⎯⎯⎯ Failed Tests 4 ⎯⎯⎯⎯⎯⎯⎯ FAIL test/terminal.test.ts > GhosttyVtTerminal > rejects a Uint16Array in the addon feed with a TypeError FAIL test/terminal.test.ts > GhosttyVtTerminal > rejects a Float32Array in the addon feed with a TypeError FAIL test/terminal.test.ts > GhosttyVtTerminal > rejects a Uint8ClampedArray in the addon feed with a TypeError AssertionError: expected undefined to be an instance of TypeError FAIL test/terminal.test.ts > GhosttyVtTerminal > rejects a DataView in the addon feed with a TypeError AssertionError: expected Error: Invalid argument to be an instance of TypeError Tests 4 failed | 3 passed | 33 skipped (40) ``` With the fix (fabeeaf): ``` ✓ test/terminal.test.ts (40 tests | 33 skipped) 5ms Tests 7 passed | 33 skipped (40) ``` Repro script (`node repro-feed.mjs` after `npm run build`), after the fix. On main the raw `Uint16Array`, `Float32Array` and `Uint8ClampedArray` cases returned "ok", and `DataView` threw `Error: Invalid argument`. ``` raw feed(Buffer 'AB'): ok, text "AB" raw feed(Uint8Array [65,66]): ok, text "AB" raw feed(Uint16Array [0x4241]): TypeError: feed typed array must be Uint8Array raw feed(Float32Array [1]): TypeError: feed typed array must be Uint8Array raw feed(Uint8ClampedArray [65,66]): TypeError: feed typed array must be Uint8Array raw feed(DataView(2 bytes 'AB')): TypeError: feed data must be a string, Buffer, or Uint8Array public feed(Buffer 'AB'): ok, text "AB" public feed(Uint8Array [65,66]): ok, text "AB" public feed(Uint16Array [0x4241]): TypeError: feed data must be a string, Buffer, or Uint8Array public feed(Float32Array [1]): TypeError: feed data must be a string, Buffer, or Uint8Array public feed(Uint8ClampedArray [65,66]): TypeError: feed data must be a string, Buffer, or Uint8Array public feed(DataView(2 bytes 'AB')): TypeError: feed data must be a string, Buffer, or Uint8Array ``` Gates, each exit 0: 1. `npm ci && LIBGHOSTTY_VT_PREFIX=<ReleaseFast> npm run build:native && npm run build && npm run verify`: 84 tests passed. 2. The same with the ReleaseSafe prefix: 84 tests passed. 3. UBSan: `rm -rf dist && LIBGHOSTTY_VT_PREFIX=<ReleaseFast> CXXFLAGS="-fsanitize=undefined,float-cast-overflow -fno-sanitize-recover=all" LDFLAGS="-fsanitize=undefined,float-cast-overflow" npm run build:native && npm test && npm run smoke`: 84 tests passed, smoke passed. 4. `.agents/skills/verify-native-lifecycle/run.sh`: `OK: every result matches EXPECT.` --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `medium`_
1 parent 9d5a707 commit c54051e

3 files changed

Lines changed: 34 additions & 5 deletions

File tree

‎native/README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,5 +76,5 @@ C++ declarations:
7676
- Run (after `npm ci`): `npm run build:libghostty && npm run build:native && npm test`
7777
- Every native test skips when the addon does not load (`test/native-support.ts:nativeSupport`), so check the test output for skips.
7878
- `mise run ci-ubsan` builds the addon with UBSan (Linux, GCC), then runs the tests and smoke. Undefined behavior aborts the run. The task also fails when the addon's object files contain no UBSan checks, which means the flags did not reach g++ (`mise.toml:tasks.ci-ubsan`).
79-
- `test/workers.test.ts` checks the per-environment state in child processes with `worker_threads`. `test/terminal.test.ts` covers `feed`, `snapshot`, `resize`, `dispose`, the `RangeError` cases of the `createTerminal` checks in the addon, the `encodeMouse` bytes of each mouse format, `cursorVisible`, the cell style fields, DECSTR, and whether `scrollbackLimit` keeps history. `test/snapshot-safety.test.ts` checks in child processes that `Object.prototype` accessors cannot dispose the terminal mid-`snapshot`. `test/mouse-safety.test.ts` runs the `encodeMouse` limits and the getter re-entry cases against both the package API and the raw addon object. It skips when the addon is missing, like the other native suites. Its child-process cases load `src/` through `tsx`, so they need no `dist/` build. No vitest test covers `formatPlain`, `formatHtml`, the content of `scrollbackLines`, or the typed-array errors.
79+
- `test/workers.test.ts` checks the per-environment state in child processes with `worker_threads`. `test/terminal.test.ts` covers `feed`, `snapshot`, `resize`, `dispose`, the `RangeError` cases of the `createTerminal` checks in the addon, the `encodeMouse` bytes of each mouse format, `cursorVisible`, the cell style fields, DECSTR, and whether `scrollbackLimit` keeps history. `test/snapshot-safety.test.ts` checks in child processes that `Object.prototype` accessors cannot dispose the terminal mid-`snapshot`. `test/mouse-safety.test.ts` runs the `encodeMouse` limits and the getter re-entry cases against both the package API and the raw addon object. It skips when the addon is missing, like the other native suites. Its child-process cases load `src/` through `tsx`, so they need no `dist/` build. No vitest test covers `formatPlain`, `formatHtml`, or the content of `scrollbackLines`.
8080
- For changes to `TerminalWrap` or `InitAll`, also run the TLA+ lifecycle model: [`.agents/skills/verify-native-lifecycle/SKILL.md`](https://github.com/coder/libghostty-vt-node/blob/main/.agents/skills/verify-native-lifecycle/SKILL.md).

‎native/terminal.cc‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -307,10 +307,6 @@ Napi::Value TerminalWrap::Feed(const Napi::CallbackInfo& info) {
307307
string_data = info[0].As<Napi::String>().Utf8Value();
308308
data = reinterpret_cast<const uint8_t*>(string_data.data());
309309
len = string_data.size();
310-
} else if (info[0].IsBuffer()) {
311-
const auto buffer = info[0].As<Napi::Buffer<uint8_t>>();
312-
data = buffer.Data();
313-
len = buffer.Length();
314310
} else if (info[0].IsTypedArray()) {
315311
const auto typed_array = info[0].As<Napi::TypedArray>();
316312
if (typed_array.TypedArrayType() != napi_uint8_array) {

‎test/terminal.test.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -562,6 +562,39 @@ describeIfNative("GhosttyVtTerminal", () => {
562562
expect((thrown as Error).message).toMatch(message);
563563
});
564564

565+
it.each<[string, () => unknown, RegExp]>([
566+
["Uint16Array", () => new Uint16Array([0x4241]), /^feed typed array must be Uint8Array$/],
567+
["Float32Array", () => new Float32Array([1]), /^feed typed array must be Uint8Array$/],
568+
["Uint8ClampedArray", () => new Uint8ClampedArray([65, 66]), /^feed typed array must be Uint8Array$/],
569+
["DataView", () => new DataView(new Uint8Array([65, 66]).buffer), /^feed data must be a string, Buffer, or Uint8Array$/],
570+
])("rejects a %s in the addon feed with a TypeError", (_name, makeData, message) => {
571+
const term = loadNative().createTerminal({ cols: 10, rows: 1 });
572+
let thrown: unknown;
573+
try {
574+
term.feed(makeData() as Uint8Array);
575+
} catch (error) {
576+
thrown = error;
577+
} finally {
578+
term.dispose();
579+
}
580+
expect(thrown).toBeInstanceOf(TypeError);
581+
expect((thrown as Error).message).toMatch(message);
582+
});
583+
584+
it.each<[string, () => Uint8Array]>([
585+
["Buffer", () => Buffer.from("AB")],
586+
["Uint8Array", () => new Uint8Array([65, 66])],
587+
["Buffer slice with a byte offset", () => Buffer.from("xxABxx").subarray(2, 4)],
588+
])("writes a %s in the addon feed", (_name, makeData) => {
589+
const term = loadNative().createTerminal({ cols: 10, rows: 1 });
590+
try {
591+
term.feed(makeData());
592+
expect(term.getVisibleText()).toBe("AB");
593+
} finally {
594+
term.dispose();
595+
}
596+
});
597+
565598
it("makes dispose idempotent and rejects use after dispose", () => {
566599
const term = createTerminal({ cols: 80, rows: 24 });
567600
term.dispose();

0 commit comments

Comments
 (0)