From f9326c43b9f59f41332bcb4703e0a02ad543326d Mon Sep 17 00:00:00 2001 From: Lann Martin Date: Sun, 6 Sep 2026 22:03:26 -0400 Subject: [PATCH] built-ins: normalize guest indices/pointers/lengths to u32; Table.get traps on a negative index Guest-supplied i32 arguments arrive signed. The async and stream built-ins (waitable-set.wait/poll/drop, waitable.join, subtask.cancel/drop, stream/future read/write/cancel-*/drop-*, error-context.*, the FACT stream/future transfer trampolines) used them raw, unlike the resource intrinsics in intrinsics/mod.ts. Two observable effects: - `Table.remove(i)` mutated (`array[i] = null; free.push(i)`) BEFORE the caller's class check trapped: a guest passing 0xFFFFFFFF (JS -1) pushed -1 onto the free list, so the next `handles.add` could hand out index -1 behind a guest-catchable trap. `Table.get` also returned `undefined` for a negative index instead of trapping (JS `array[-1]` is `undefined`, not the `null` sentinel), so the range trap never fired. - `stream.write` with n = 0xFFFFFFFF saw a negative length: the `Buffer.MAX_LENGTH` trap (definitions.py 911-920) never tripped and the write parked or completed with 0. With `>>> 0` at every built-in entry, -1 is 0xFFFFFFFF, which is out of range and traps in `Table.get` before any mutation, exactly as the reference's u32-indexed `Table.get/remove` (682-703). `Table.get` additionally guards `i < 0` as defense in depth. Regression: builtin_index_normalization_test.ts. Conformance 0 failed / 0 stale; sched-seeds green. --- runtime/src/cabi/handles.ts | 4 +- runtime/src/intrinsics/async_builtins.ts | 34 ++- runtime/src/intrinsics/stream_builtins.ts | 77 +++--- .../tests/builtin_index_normalization_test.ts | 257 ++++++++++++++++++ 4 files changed, 328 insertions(+), 44 deletions(-) create mode 100644 runtime/tests/builtin_index_normalization_test.ts diff --git a/runtime/src/cabi/handles.ts b/runtime/src/cabi/handles.ts index 1051ea3..8e909d7 100644 --- a/runtime/src/cabi/handles.ts +++ b/runtime/src/cabi/handles.ts @@ -35,7 +35,9 @@ export class Table { free: number[] = []; get(i: number): T { - trapIf(i >= this.array.length, "table index out of range"); + // Indices are u32; a negative i is out of range, and JS `array[-1]` is + // `undefined`, not the `null` sentinel. + trapIf(i < 0 || i >= this.array.length, "table index out of range"); trapIf(this.array[i] === null, "table entry empty"); return this.array[i]!; } diff --git a/runtime/src/intrinsics/async_builtins.ts b/runtime/src/intrinsics/async_builtins.ts index 2ae86f7..3aea88e 100644 --- a/runtime/src/intrinsics/async_builtins.ts +++ b/runtime/src/intrinsics/async_builtins.ts @@ -283,8 +283,12 @@ export function createWaitableSetWait( // (line 2414). const cancellable = opts.cancellable; return (si?: number, ptr?: number) => { + // Guest-supplied index/pointer are u32; core wasm delivers i32 args + // signed (F3, R2). Normalize at the entry boundary. + si = (si ?? 0) >>> 0; + ptr = (ptr ?? 0) >>> 0; trapIf(!inst.mayLeave, "waitable-set.wait: cannot leave component instance"); - const wset = requireWaitableSet(inst, si ?? 0, "waitable-set.wait"); + const wset = requireWaitableSet(inst, si, "waitable-set.wait"); const task = currentTask() as Task; let event: EventTuple; if (task.deliverPendingCancel(cancellable)) { @@ -330,7 +334,7 @@ export function createWaitableSetWait( const ev: EventTuple = cancelled ? [EventCode.TASK_CANCELLED, 0, 0] : wset.getPendingEvent(); - return unpackEvent(opts, inst, ptr ?? 0, ev); + return unpackEvent(opts, inst, ptr, ev); }, onSettled: () => { wset.numWaiting -= 1; @@ -343,7 +347,7 @@ export function createWaitableSetWait( "instead)", ); } - return unpackEvent(opts, inst, ptr ?? 0, event); + return unpackEvent(opts, inst, ptr, event); }; } @@ -357,18 +361,22 @@ export function createWaitableSetPoll( /** See `createWaitableSetWait`: `cancellable` is an option, not a decl field. */ const cancellable = opts.cancellable; return (si?: number, ptr?: number) => { + si = (si ?? 0) >>> 0; + ptr = (ptr ?? 0) >>> 0; trapIf(!inst.mayLeave, "waitable-set.poll: cannot leave component instance"); - const wset = requireWaitableSet(inst, si ?? 0, "waitable-set.poll"); + const wset = requireWaitableSet(inst, si, "waitable-set.poll"); const event = wset.poll(currentTask(), cancellable); - return unpackEvent(opts, inst, ptr ?? 0, event); + return unpackEvent(opts, inst, ptr, event); }; } /** definitions.py `canon_waitable_set_drop` (line 2441). */ export function createWaitableSetDrop(inst: ComponentInstanceState): CoreFn { return (i?: number) => { + // Guest-supplied index is u32; core wasm delivers i32 args signed (F3, R2). + i = (i ?? 0) >>> 0; trapIf(!inst.mayLeave, "waitable-set.drop: cannot leave component instance"); - const wset = inst.handles.remove(i ?? 0); + const wset = inst.handles.remove(i); trapIf( !(wset instanceof WaitableSet), "waitable-set.drop: handle is not a waitable set", @@ -380,8 +388,10 @@ export function createWaitableSetDrop(inst: ComponentInstanceState): CoreFn { /** definitions.py `canon_waitable_join` (line 2451). */ export function createWaitableJoin(inst: ComponentInstanceState): CoreFn { return (wi?: number, si?: number) => { + wi = (wi ?? 0) >>> 0; + si = (si ?? 0) >>> 0; trapIf(!inst.mayLeave, "waitable.join: cannot leave component instance"); - const w = inst.handles.get(wi ?? 0); + const w = inst.handles.get(wi); trapIf(!(w instanceof Waitable), "waitable.join: handle is not a waitable"); trapIf( (w as Waitable).hasSyncWaiter, @@ -392,11 +402,11 @@ export function createWaitableJoin(inst: ComponentInstanceState): CoreFn { "waitable cannot be used synchronously while added to a waitable set " + "(waitable.join)", ); - if ((si ?? 0) === 0) { + if (si === 0) { (w as Waitable).join(null); return; } - const wset = requireWaitableSet(inst, si!, "waitable.join"); + const wset = requireWaitableSet(inst, si, "waitable.join"); (w as Waitable).join(wset); }; } @@ -408,8 +418,9 @@ export function createWaitableJoin(inst: ComponentInstanceState): CoreFn { /** definitions.py `canon_subtask_drop` (line 2494). */ export function createSubtaskDrop(inst: ComponentInstanceState): CoreFn { return (i?: number) => { + i = (i ?? 0) >>> 0; trapIf(!inst.mayLeave, "subtask.drop: cannot leave component instance"); - const s = inst.handles.remove(i ?? 0); + const s = inst.handles.remove(i); trapIf(!(s instanceof Subtask), "subtask.drop: handle is not a subtask"); (s as Subtask).drop(); }; @@ -454,6 +465,7 @@ export function createSubtaskCancel( ): CoreFn { const async_ = decl.async === true; return (i?: number) => { + i = (i ?? 0) >>> 0; // The handle table is the **declared** instance's, not // `current_thread().task.inst`. definitions.py `canon_subtask_cancel` // (line 2469) uses the latter because the reference has no fused @@ -468,7 +480,7 @@ export function createSubtaskCancel( // correction already applied to every other instance-scoped built-in — // see this module's header. trapIf(!inst.mayLeave, "subtask.cancel: cannot leave component instance"); - const subtask = inst.handles.get(i ?? 0); + const subtask = inst.handles.get(i); trapIf( !(subtask instanceof Subtask), "subtask.cancel: handle is not a subtask", diff --git a/runtime/src/intrinsics/stream_builtins.ts b/runtime/src/intrinsics/stream_builtins.ts index 367a453..d3a22e9 100644 --- a/runtime/src/intrinsics/stream_builtins.ts +++ b/runtime/src/intrinsics/stream_builtins.ts @@ -495,6 +495,8 @@ function dropEnd( hi: number, what: string, ): void { + // Guest-supplied index is u32; core wasm delivers i32 args signed (F3, R2). + hi = hi >>> 0; trapIf(!inst.mayLeave, `${what}: cannot leave component instance`); const e = inst.handles.remove(hi); trapIf(!(e instanceof EndT), `${what}: wrong end type for this handle`); @@ -523,9 +525,11 @@ export function createErrorContextNew( ): CoreFn { const opts = ctx.options(decl.options); return (ptr?: number, taggedCodeUnits?: number) => { + ptr = (ptr ?? 0) >>> 0; + taggedCodeUnits = (taggedCodeUnits ?? 0) >>> 0; trapIf(!inst.mayLeave, "error-context.new: cannot leave component instance"); const cx = new LiftLowerContext(cabiOptions(opts), inst, null); - const s = loadStringFromRange(cx, ptr ?? 0, taggedCodeUnits ?? 0); + const s = loadStringFromRange(cx, ptr, taggedCodeUnits); return inst.handles.add(new ErrorContext(s)); }; } @@ -538,17 +542,19 @@ export function createErrorContextDebugMessage( ): CoreFn { const opts = ctx.options(decl.options); return (i?: number, ptr?: number) => { + i = (i ?? 0) >>> 0; + ptr = (ptr ?? 0) >>> 0; trapIf( !inst.mayLeave, "error-context.debug-message: cannot leave component instance", ); - const e = inst.handles.get(i ?? 0); + const e = inst.handles.get(i); trapIf( !(e instanceof ErrorContext), errorContextTrapMessage("error-context.debug-message", e), ); const cx = new LiftLowerContext(cabiOptions(opts), inst, null); - storeString(cx, (e as ErrorContext).debugMessage, ptr ?? 0); + storeString(cx, (e as ErrorContext).debugMessage, ptr); }; } @@ -557,11 +563,12 @@ export function createErrorContextDrop( inst: ComponentInstanceState, ): CoreFn { return (i?: number) => { + i = (i ?? 0) >>> 0; trapIf( !inst.mayLeave, "error-context.drop: cannot leave component instance", ); - const e = inst.handles.remove(i ?? 0); + const e = inst.handles.remove(i); trapIf( !(e instanceof ErrorContext), errorContextTrapMessage("error-context.drop", e), @@ -589,9 +596,9 @@ export function createStreamRead( elem, opts, inst, - i: i ?? 0, - ptr: ptr ?? 0, - n: n ?? 0, + i: (i ?? 0) >>> 0, + ptr: (ptr ?? 0) >>> 0, + n: (n ?? 0) >>> 0, }); } @@ -611,9 +618,9 @@ export function createStreamWrite( elem, opts, inst, - i: i ?? 0, - ptr: ptr ?? 0, - n: n ?? 0, + i: (i ?? 0) >>> 0, + ptr: (ptr ?? 0) >>> 0, + n: (n ?? 0) >>> 0, }); } @@ -633,8 +640,8 @@ export function createFutureRead( elem, opts, inst, - i: i ?? 0, - ptr: ptr ?? 0, + i: (i ?? 0) >>> 0, + ptr: (ptr ?? 0) >>> 0, }); } @@ -654,8 +661,8 @@ export function createFutureWrite( elem, opts, inst, - i: i ?? 0, - ptr: ptr ?? 0, + i: (i ?? 0) >>> 0, + ptr: (ptr ?? 0) >>> 0, }); } @@ -673,7 +680,7 @@ export function createStreamCancelRead( elem, inst, async_: d.async === true, - i: i ?? 0, + i: (i ?? 0) >>> 0, what: "stream.cancel-read", }); } @@ -692,7 +699,7 @@ export function createStreamCancelWrite( elem, inst, async_: d.async === true, - i: i ?? 0, + i: (i ?? 0) >>> 0, what: "stream.cancel-write", }); } @@ -711,7 +718,7 @@ export function createFutureCancelRead( elem, inst, async_: d.async === true, - i: i ?? 0, + i: (i ?? 0) >>> 0, what: "future.cancel-read", }); } @@ -730,7 +737,7 @@ export function createFutureCancelWrite( elem, inst, async_: d.async === true, - i: i ?? 0, + i: (i ?? 0) >>> 0, what: "future.cancel-write", }); } @@ -861,29 +868,35 @@ function transferAsyncEnd(input: { } export function createStreamTransfer(ctx: AsyncTransferContext): CoreFn { - return (srcIdx?: number, srcTable?: number, dstTable?: number) => - transferAsyncEnd({ + return (srcIdx?: number, srcTable?: number, dstTable?: number) => { + srcTable = (srcTable ?? 0) >>> 0; + dstTable = (dstTable ?? 0) >>> 0; + return transferAsyncEnd({ EndT: ReadableStreamEnd as unknown as EndCtor, - srcInst: ctx.streamTableInstance(srcTable ?? 0), - dstInst: ctx.streamTableInstance(dstTable ?? 0), - srcElem: ctx.streamElem(srcTable ?? 0), - dstElem: ctx.streamElem(dstTable ?? 0), - srcIdx: srcIdx ?? 0, + srcInst: ctx.streamTableInstance(srcTable), + dstInst: ctx.streamTableInstance(dstTable), + srcElem: ctx.streamElem(srcTable), + dstElem: ctx.streamElem(dstTable), + srcIdx: (srcIdx ?? 0) >>> 0, what: "stream", }); + }; } export function createFutureTransfer(ctx: AsyncTransferContext): CoreFn { - return (srcIdx?: number, srcTable?: number, dstTable?: number) => - transferAsyncEnd({ + return (srcIdx?: number, srcTable?: number, dstTable?: number) => { + srcTable = (srcTable ?? 0) >>> 0; + dstTable = (dstTable ?? 0) >>> 0; + return transferAsyncEnd({ EndT: ReadableFutureEnd as unknown as EndCtor, - srcInst: ctx.futureTableInstance(srcTable ?? 0), - dstInst: ctx.futureTableInstance(dstTable ?? 0), - srcElem: ctx.futureElem(srcTable ?? 0), - dstElem: ctx.futureElem(dstTable ?? 0), - srcIdx: srcIdx ?? 0, + srcInst: ctx.futureTableInstance(srcTable), + dstInst: ctx.futureTableInstance(dstTable), + srcElem: ctx.futureElem(srcTable), + dstElem: ctx.futureElem(dstTable), + srcIdx: (srcIdx ?? 0) >>> 0, what: "future", }); + }; } /** diff --git a/runtime/tests/builtin_index_normalization_test.ts b/runtime/tests/builtin_index_normalization_test.ts new file mode 100644 index 0000000..15a8abe --- /dev/null +++ b/runtime/tests/builtin_index_normalization_test.ts @@ -0,0 +1,257 @@ +// The u32-normalization cluster: guest-supplied indices/lengths are u32, +// but core wasm delivers i32 args as signed JS numbers, and several +// built-ins use them without `>>> 0` (contrast intrinsics/mod.ts:538-551 +// which does normalize resource handles). Two distinct failure shapes: +// +// (a)/(b) `Table.remove` (async_builtins.ts subtask.drop/waitable-set.drop, +// stream_builtins.ts dropEnd for stream.drop-{readable,writable}) calls +// `get()` (which does not trap on i<0 — see (d)) then unconditionally +// mutates `array[i] = null; free.push(i)` BEFORE the instanceof check +// traps on the bogus (undefined) entry. A guest passing 0xFFFFFFFF +// corrupts the free list before the trap fires. +// (c) `stream.write` with n = 0xFFFFFFFF (definitions.py +// `BufferGuestImpl.__init__` 911-920 traps on length > MAX_LENGTH, which +// is a u32 comparison); GuestBuffer's `length > BUFFER_MAX_LENGTH` check +// sees a negative JS number and never trips. +// (d) `Table.get(i)` with i<0: `i >= this.array.length` is false for +// negative i, and `this.array[i] === null` is false too (index access on +// a negative key reads `undefined`, not the sentinel `null`) — so `get` +// returns `undefined` without trapping at all, rather than treating the +// out-of-range index as a trap. +// +// Authority: definitions.py `Table.get/remove` (682-703, indices are +// non-negative ints; get traps on `i >= len` -- negative i is never a valid +// index either), `BufferGuestImpl.__init__` 911-920 (`Buffer.MAX_LENGTH`). + +import { assertEq } from "./support/asserts.ts"; +import { Trap } from "../src/cabi/mod.ts"; +import { Table } from "../src/cabi/handles.ts"; +import { + createSubtaskDrop, + createWaitableSetDrop, +} from "../src/intrinsics/async_builtins.ts"; +import { + createStreamDropReadable, + createStreamDropWritable, + createStreamNew, + createStreamWrite, +} from "../src/intrinsics/stream_builtins.ts"; +import { + ComponentInstanceState, + popCurrentThread, + pushCurrentThread, + Store, + Task, + Thread, + WaitableSet, +} from "../src/task/mod.ts"; +import type { ResolvedOptions } from "../src/exec/boundary.ts"; + +function assert(cond: boolean, msg: string): asserts cond { + if (!cond) throw new Error(`assertion failed: ${msg}`); +} + +function assertTraps(fn: () => unknown, includes?: string): void { + try { + fn(); + } catch (e) { + assert(e instanceof Trap, `expected a Trap, got ${e}`); + if (includes !== undefined) { + assert( + String(e).includes(includes), + `expected trap containing ${JSON.stringify(includes)}, got: ${e}`, + ); + } + return; + } + throw new Error("expected a trap"); +} + +// The i32 core wasm delivers for guest literal 0xFFFFFFFF. +const NEG_ONE = 0xffff_ffff | 0; +assertEq(NEG_ONE, -1, "sanity"); + +/** A live `MemInst` view over a real WebAssembly.Memory (async_builtins_test.ts style). */ +function mkMemory() { + const memory = new WebAssembly.Memory({ initial: 1 }); + return { + memory, + view: { + addrType: "i32" as const, + get bytes() { + return new Uint8Array(memory.buffer); + }, + get view() { + return new DataView(memory.buffer); + }, + get length() { + return memory.buffer.byteLength; + }, + ptrType: () => "i32" as const, + ptrSize: () => 4 as const, + }, + }; +} + +// --------------------------------------------------------------------------- +// (d) Table.get(-1) should trap, not return undefined +// --------------------------------------------------------------------------- + +Deno.test("Table.get(-1) traps instead of silently returning undefined", () => { + const t = new Table(); + t.add("a"); + // Reference: Table.get traps on any out-of-range index; a negative index + // is out of range (indices are non-negative). Ours: `i >= array.length` is + // false for i=-1, and `array[-1] === null` is false (it's `undefined`), so + // `get` falls through and returns `undefined` with no trap at all. + assertTraps(() => t.get(-1)); +}); + +// --------------------------------------------------------------------------- +// (a) subtask.drop(-1) must trap AND leave the free list untouched +// --------------------------------------------------------------------------- + +Deno.test("subtask.drop(0xFFFFFFFF) traps without corrupting the handle free list", () => { + const store = new Store(); + const inst = new ComponentInstanceState(0, store); + const drop = createSubtaskDrop(inst); + + assertEq(inst.handles.free.length, 0, "sanity: free list starts empty"); + assertTraps(() => drop(NEG_ONE)); + // Reference: an out-of-range index traps before any table mutation, so the + // free list is untouched. Ours: `Table.remove(-1)` writes `array[-1] = + // null` and pushes -1 onto `free` before the instanceof check traps. + assertEq( + inst.handles.free.length, + 0, + "free list must be untouched by a trapping subtask.drop", + ); +}); + +// --------------------------------------------------------------------------- +// (b) waitable-set.drop(-1) — same shape +// --------------------------------------------------------------------------- + +Deno.test("waitable-set.drop(0xFFFFFFFF) traps without corrupting the handle free list", () => { + const store = new Store(); + const inst = new ComponentInstanceState(0, store); + const drop = createWaitableSetDrop(inst); + + assertEq(inst.handles.free.length, 0, "sanity: free list starts empty"); + assertTraps(() => drop(NEG_ONE)); + assertEq( + inst.handles.free.length, + 0, + "free list must be untouched by a trapping waitable-set.drop", + ); +}); + +// --------------------------------------------------------------------------- +// (b) stream.drop-readable / stream.drop-writable — same shape +// --------------------------------------------------------------------------- + +function mkStreamDropCtx(inst: ComponentInstanceState) { + return { + componentInstance: () => inst, + options: () => { + throw new Error("not used by drop-*"); + }, + streamElem: () => ({ kind: "u8" } as const), + futureElem: () => null, + // deno-lint-ignore no-explicit-any + } as any; +} + +Deno.test("stream.drop-readable(0xFFFFFFFF) traps without corrupting the handle free list", () => { + const store = new Store(); + const inst = new ComponentInstanceState(0, store); + const drop = createStreamDropReadable( + { streamTable: 0 }, + mkStreamDropCtx(inst), + inst, + ); + + assertEq(inst.handles.free.length, 0, "sanity: free list starts empty"); + assertTraps(() => drop(NEG_ONE)); + assertEq( + inst.handles.free.length, + 0, + "free list must be untouched by a trapping stream.drop-readable", + ); +}); + +Deno.test("stream.drop-writable(0xFFFFFFFF) traps without corrupting the handle free list", () => { + const store = new Store(); + const inst = new ComponentInstanceState(0, store); + const drop = createStreamDropWritable( + { streamTable: 0 }, + mkStreamDropCtx(inst), + inst, + ); + + assertEq(inst.handles.free.length, 0, "sanity: free list starts empty"); + assertTraps(() => drop(NEG_ONE)); + assertEq( + inst.handles.free.length, + 0, + "free list must be untouched by a trapping stream.drop-writable", + ); +}); + +// --------------------------------------------------------------------------- +// (c) stream.write with n = 0xFFFFFFFF must trap (Buffer.MAX_LENGTH) +// --------------------------------------------------------------------------- + +Deno.test("stream.write(n=0xFFFFFFFF) traps instead of parking or completing with 0", () => { + const store = new Store(); + const inst = new ComponentInstanceState(0, store); + const { view } = mkMemory(); + const opts: ResolvedOptions = { + stringEncoding: "utf8", + // deno-lint-ignore no-explicit-any + memory: view as any, + realloc: null, + postReturn: null, + callback: null, + async: true, + cancellable: false, + coreType: { params: [], results: [] }, + instance: inst, + }; + const ctx = { + componentInstance: () => inst, + options: () => opts, + streamElem: () => ({ kind: "u8" } as const), + futureElem: () => null, + // deno-lint-ignore no-explicit-any + } as any; + + const newStream = createStreamNew({ streamTable: 0 }, ctx, inst); + const write = createStreamWrite( + { streamTable: 0, options: 0 }, + ctx, + inst, + ); + const packed = newStream() as bigint; + const wi = Number(packed >> 32n); + + const task = new Task( + { params: [], results: [], async: true }, + { async_: true, callback: true, stringEncoding: "utf8", memory: null }, + inst, + () => [], + () => {}, + ); + const thread = new Thread(task, (function* () {})()); + pushCurrentThread(thread); + try { + // Reference: `BufferGuestImpl.__init__` traps `length > Buffer.MAX_LENGTH` + // as a u32 comparison, so 0xFFFFFFFF is always over MAX_LENGTH (2^28-1) + // and this must trap. Ours: `length > BUFFER_MAX_LENGTH` sees the signed + // JS number -1, the comparison is false, and the write silently parks + // (returns BLOCKED) instead. + assertTraps(() => write(wi, 0, NEG_ONE), "MAX_LENGTH"); + } finally { + popCurrentThread(thread); + } +});