built-ins: normalize guest indices/pointers/lengths to u32; Table.get traps on a negative index (review) - #304
Merged
Merged
Conversation
… 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.
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
2 participants
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.
Adversarial correctness review of
main@ 396a216 — the u32-normalization cluster (task-core F3, streams F3, and theTablenotes from the CABI/streams tracks).Guest-supplied i32 indices/pointers/lengths arrive signed; the async and stream built-ins used them raw. Two observable effects, both pinned by
builtin_index_normalization_test.ts(6 tests):Table.remove(i)mutated (array[i] = null; free.push(i)) BEFORE the caller's class check trapped, so a guest passing 0xFFFFFFFF (JS -1) pushed -1 onto the free list;Table.get(-1)returnedundefined(JSarray[-1], not thenullsentinel) instead of trapping.stream.writewith n = 0xFFFFFFFF saw a negative length: theBuffer.MAX_LENGTHtrap never tripped and the write parked / completed with 0.Fix:
>>> 0at every built-in entry (so -1 is 0xFFFFFFFF → out of range → trap inTable.getbefore any mutation, matching the reference's u32-indexedTable.get/remove, definitions.py 682-703) plus ani < 0guard inTable.getas defense in depth.Gates:
just test-runtimegreen;just conformance0 failed / 0 stale;just sched-seedsgreen; fulljust gatesgreen on the union of the five review PRs.Automerge armed.