Skip to content

node:wasi: return EOVERFLOW for out-of-bounds iovecs instead of throwing/logging - #34468

Open
robobun wants to merge 3 commits into
mainfrom
claude/e4be467d/wasi-iovec-oob
Open

robobun wants to merge 3 commits into
mainfrom
claude/e4be467d/wasi-iovec-oob

Conversation

@robobun

@robobun robobun commented Jul 17, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • node:wasi does not bounds-check the iovecs a guest hands to fd_write, fd_read, fd_pwrite and fd_pread. Depending on which pointer is bad, the hostcall either throws RangeError: Out of bounds access into the host program, or truncates the buffer to fit, performs the I/O anyway, returns success and prints { buf, bufLen, total_memory } to the host's stdout plus a console.warn to its stderr. Node returns the WASI errno EOVERFLOW (61) for all of these.
  • Output pointers are only touched after the work has been done, so a valid iovec with an out-of-bounds nwritten writes the bytes to the host fd (or file, for fd_pwrite) and then throws; path_open with an out-of-bounds fdPtr opens the file and records it in FD_MAP and then throws, leaking a host descriptor per call; fd_seek moves the offset and then throws; fd_tell switches a descriptor that was still using the host's file position to an explicit offset of 0 and then throws.
  • The path_* hostcalls read the guest path with Buffer.from(buffer, ptr, len), which throws a RangeError carrying ERR_BUFFER_OUT_OF_BOUNDS when the range does not fit; wrap() looks that code up in the errno table first and returns EINVAL (28) where Node returns EOVERFLOW.
  • Cause: getiovs() in src/js/node/wasi.ts reads the iovec array and builds the Uint8Arrays without checking the array pointer, buf + len, or the output pointer against memory.buffer.byteLength; no hostcall checks its output pointer before acting; and wrap() only maps WASIErrors and errors with a string .code to errnos.

Repro against the released 1.4.0:

import { WASI } from "node:wasi";
const wasi = new WASI({ version: "preview1" });
wasi.setMemory(new WebAssembly.Memory({ initial: 1 }));
const view = new DataView(wasi.memory.buffer);

wasi.wasiImport.fd_write(2, 65534, 1, 256);          // bun: throws RangeError          node: 61
view.setUint32(0, 65530, true); view.setUint32(4, 4096, true);
wasi.wasiImport.fd_write(2, 0, 1, 256);              // bun: 0, writes 6 bytes, logs    node: 61

Fix

  • getiovs() validates the iovec array, every buf/len pair and the nread/nwritten pointer before returning, and throws WASIError(WASI_EOVERFLOW) on the first violation, so the four callers never start I/O with a bad pointer. The truncation and the console.log/console.warn output are gone.
  • path_open checks fdPtr before opening anything, fd_seek checks newOffsetPtr before moving the offset and fd_tell checks offsetPtr before recording one. Together with getiovs() these cover every hostcall that changes host or descriptor state before storing through a guest pointer.
  • wrap() maps the two shapes an out-of-bounds pointer produces inside a wrapped hostcall, a bare RangeError from DataView/TypedArray and ERR_BUFFER_OUT_OF_BOUNDS from Buffer.from, to EOVERFLOW ahead of the generic errno-code lookup. That gives Node's errno for the path_* calls and for struct outputs such as fd_fdstat_get; other coded errors (fs errors, ERR_OUT_OF_RANGE) still go through the table as before.
  • The bounds rule is the one uvwasi applies (uvwasi_serdes_check_bounds: the start of a range has to lie inside memory even for an empty range, and the range must not run past the end), so the exact boundary matches Node: ranges ending at byteLength are accepted, an empty range starting at byteLength is rejected. Pointers and lengths that are not a u32 (a wasm guest passing a value >= 2**31 delivers a negative i32) are rejected the same way; without that, a negative iovsLen returned success and a negative nwritten did the write before failing. Verified case by case against Node v26.3.0 (table below).
  • Left for a follow-up change, so this one stays on the calls that have side effects: args_get, args_sizes_get, environ_get, environ_sizes_get, clock_res_get, clock_time_get, random_get and poll_oneoff are not wrapped and still throw on a bad pointer (clock_* and poll_oneoff have open PRs against them, node:wasi: fix clock_res_get endianness and missing refreshMemory #34471 and node:wasi: fix poll_oneoff TypeError on clock subscriptions #34470); the struct-writing calls (fd_fdstat_get, fd_filestat_get, ...) return the errno but do not preflight, so a struct straddling the end of memory is partially written; and the Buffer#write sites (fd_prestat_dir_name, path_readlink, args_get, environ_get) still clamp instead of failing. The checkBounds helper added here is what that change needs.
  • Not changed on purpose: the fd is still checked before the pointers, so a bad fd combined with a bad pointer returns EBADF where Node returns EOVERFLOW. fd_pread reporting a doubled nread is a separate pre-existing bug (node:wasi: fix fd_pread reporting doubled nread #34472).
  • Verification:
    • test/js/bun/wasm/wasi.test.js, six new tests: a subprocess test covers 15 out-of-bounds shapes across fd_write/fd_read/fd_fdstat_get and asserts nothing reaches the host's stdout/stderr; one pins the accepted boundary (ranges ending exactly at the end of memory, empty ranges on the last byte); one feeds fd_read through a getStdin callback and checks a rejected call consumed nothing; one covers fd_pwrite/fd_pread through a preopen and checks the file and guest memory are untouched; one covers path_open (including O_CREAT), fd_seek and fd_tell and checks FD_MAP, the directory and the offset are untouched; one covers path_filestat_get/path_create_directory with the path itself out of bounds.
    • USE_SYSTEM_BUN=1 bun test test/js/bun/wasm/wasi.test.js: 5 of the 6 new tests fail on the current release (the boundary test passes there by design, it guards against over-strict checks).
    • bun bd test test/js/bun/wasm/wasi.test.js: 11 pass with this branch's wasi.ts; 5 fail with main's; the fd_tell and path_* assertions fail with the previous revision of this branch.

Background

  • WASI preview1 hostcalls take guest pointers as i32 offsets into the instance's linear memory (memory.buffer). An iovec is 8 bytes in that memory: a u32 buf offset and a u32 len. fd_write(fd, iovs, iovsLen, nwritten) reads iovsLen iovecs starting at offset iovs, writes their contents, and stores the byte count at offset nwritten. path_open stores the new descriptor number at fdPtr; fd_seek and fd_tell store the offset at their pointer argument.
  • Hostcalls report failures as a returned errno, never as a JS exception; a guest can only observe the errno. Bun's implementation (src/js/node/wasi.ts, derived from wasi-js) gets that by having wrap() convert errors thrown inside a hostcall into errnos. Node's implementation is node_wasi.cc on top of uvwasi, which bounds-checks every pointer argument up front and returns EOVERFLOW (61) when one does not fit in memory.
  • A descriptor in FD_MAP has no offset until the guest seeks or tells; until then reads and writes use the host's file position, afterwards they are positional at the recorded offset. That is why fd_tell recording an offset on a failed call is observable.
errno table: node v26.3.0 vs bun 1.4.0 vs this branch
case                                        node   bun 1.4.0                    this branch
iovec array pointer OOB                      61    throws RangeError             61
empty iovec array at byteLength              61    0                             61
iovsLen 0x10000000                           61    28 + debug object on stdout   61
iovsLen / pointer negative                   28*   throws RangeError             61
iov.buf OOB                                  61    28 + debug object + warn      61
iov.len overruns memory                      61    0, truncated bytes written    61
zero-length iov at byteLength                61    0                             61
nwritten OOB (valid iovec)                   61    bytes written, then throws    61
nwritten 3 bytes before the end              61    throws RangeError             61
fd_read with nread OOB                       61    input consumed, then throws   61, nothing consumed
fd_fdstat_get buf entirely OOB               61    throws RangeError             61
buf / iovec array / nwritten ending at end    0    0                              0
empty array or empty iov on the last byte     0    0                              0
fd_pwrite with nwritten OOB                  61    file modified, then throws    61, file untouched
fd_pread with len overrunning memory         61    0, memory filled              61, memory untouched
path_open with fdPtr OOB                     61    fd opened, then throws        61, nothing opened
path_open O_CREAT with fdPtr OOB             61    file created, then throws     61, nothing created
fd_seek with newOffsetPtr OOB                61    offset moved, then throws     61, offset unchanged
fd_tell with offsetPtr OOB                   61    offset recorded, then throws  61, offset unchanged
path_filestat_get / path_create_directory
  with the path OOB                          61    28                            61
bad fd and bad pointer                       61    8                              8

* Node's argument validation for direct JS callers; a wasm guest cannot pass these.
history

The first version of this PR (reviewed in July) did the same thing with ptr + len > byteLength checks and tests for fd_write/fd_read only. It stopped applying after #36474 trimmed wasi.ts, so it was rebuilt on top of main, switched to uvwasi's exact boundary rule, and its tests were extended to fd_pwrite/fd_pread, negative pointers and the accepted boundary. Review of that revision pointed out that the wrap() fallback turned path_open's and fd_seek's post-side-effect RangeErrors into silent errnos, which added the up-front checks for those two calls. A second pass found the same pattern in fd_tell, found that Buffer.from's coded RangeError was mapped to EINVAL, and found that the fd_read rows were satisfied by the wrap() fallback alone, which added the last commit and the getStdin test. #35941 proposed a subset of the same change without tests and was closed in favour of this one.


no test proof · iteration 6 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/wasm/wasi.test.js

@robobun

robobun commented Jul 17, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 12:13 PM PT - Aug 15th, 2026

🔄 @robobun, the build for your commit 22e632a3 (Build #98191) was cancelled — waiting for the next build...

@coderabbitai

coderabbitai Bot commented Jul 17, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

WASI iovec decoding now validates guest memory bounds and returns WASI_EOVERFLOW for invalid accesses. Tests cover malformed inputs through fd_write and fd_read.

Changes

WASI overflow handling

Layer / File(s) Summary
Validate iovec memory bounds
src/js/node/wasi.ts
wrap and getiovs validate guest pointers and iovec buffers, mapping out-of-bounds accesses to WASI_EOVERFLOW; read and write call sites pass output pointers for validation.
Exercise malformed iovecs
test/js/bun/wasm/wasi.test.js
A spawned guest program tests six malformed fd_write and fd_read cases, asserting errno 61, clean stderr, expected stdout, and successful exit.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the main WASI behavior change for out-of-bounds iovecs.
Description check ✅ Passed The description thoroughly explains the problem, fix, scope, and verification, although it uses headings different from the repository template.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/js/node/wasi.ts`:
- Around line 673-675: Remove the global RangeError-to-WASI_EOVERFLOW conversion
unless all affected operations preflight their guest output ranges first. In the
fd_write flow, validate every iovec and the nwritten output pointer before
performing host writes, ensuring out-of-bounds output pointers return
WASI_EOVERFLOW without producing output; add a regression covering a valid iovec
with OOB nwritten.

In `@test/js/bun/wasm/wasi.test.js`:
- Around line 108-128: Update the stderr assertion in the WASI subprocess test
around the overrun cases to avoid requiring stderr to be exactly empty. Seed the
guest buffer’s overrun tail with a unique marker, then assert stderr does not
contain that marker while retaining the existing stdout and exit-status checks
for WASI behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1c1a8fa4-b55e-49db-828c-a87886a8a9af

📥 Commits

Reviewing files that changed from the base of the PR and between 6618e7f and aba32dc.

📒 Files selected for processing (2)
  • src/js/node/wasi.ts
  • test/js/bun/wasm/wasi.test.js

Comment thread src/js/node/wasi.ts Outdated
Comment thread test/js/bun/wasm/wasi.test.js Outdated
Comment thread test/js/bun/wasm/wasi.test.js Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the test-shape nit from my earlier pass was applied in 7726bcc, and both CodeRabbit threads are resolved.

What was reviewed:

  • getiovs() bounds arithmetic: all values stay in JS double range (u32 + u32, u32 * 8), so no silent wrap; the >>> 0 guards catch negative/non-integer inputs from both wasm i32 and direct JS callers.
  • All four getiovs call sites (fd_write/fd_pwrite/fd_read/fd_pread) now preflight the output pointer before any host I/O, closing the write-then-RangeError leak.
  • The RangeError → WASI_EOVERFLOW fallback in wrap() is ordered after .code/WASIError checks so it can't shadow a more specific errno.
Extended reasoning...

Overview

Two-file Node.js-compat fix in the pure-JS node:wasi shim. getiovs() now bounds-checks the iovec-array pointer, each buf + bufLen, and the nread/nwritten output pointer before any host I/O, throwing WASIError(WASI_EOVERFLOW) on violation instead of the previous behavior (raw RangeError escaping to host JS, or truncating and writing anyway while dumping a debug object to stdout/stderr). wrap() gains a RangeError → WASI_EOVERFLOW fallback so every wrapped hostcall stays total. A subprocess test exercises six OOB shapes across fd_write and fd_read and asserts errno 61 with clean stderr/stdout.

Security risks

The change hardens a guest→host boundary: a hostile WASI guest could previously (a) crash the host with an uncaught RangeError, (b) make the host emit arbitrary debug noise to real stdout/stderr, and (c) get truncated bytes written through to a host fd on an invalid iovec. All three are closed. No new attack surface is introduced — the added checks only reject inputs that were previously mishandled, and the arithmetic (iovs + iovsLen * 8, buf + bufLen, outPtr + 4) cannot overflow in JS number space since every operand is ≤ 2³².

Level of scrutiny

Medium. node:wasi is a documented "quick hack" JS shim (per the file header) pending a native rewrite, not a hot path. The diff is small (~40 net lines in src/), mechanical, and strictly narrows behavior to match Node's uvwasi. No native code, no memory-safety surface.

Other factors

  • All review feedback is resolved: CodeRabbit's preflight-before-I/O concern was addressed in 2f1b8a6 (output-pointer check moved into getiovs, with a seeded-marker regression case), the stderr-assertion concern was withdrawn, and my own combined-object nit was applied in 7726bcc.
  • The test uses the same combined { stdout, stderr, exitCode } assertion shape as the neighboring test in the file, drains both pipes concurrently, runs under bunEnv, and covers both the single-iovec fast path and the multi-iovec loop plus fd_read.
  • Verified the four getiovs callers are the complete set; no other site was missed.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the combined-object assertion from my earlier comment is applied in 7726bcc.

What was reviewed:

  • getiovs() preflight: iovs + iovsLen*8, outPtr + 4, and per-iovec buf + bufLen are checked against byteLength before any I/O; JS-number arithmetic here can't wrap, and all four callers (fd_write/fd_pwrite/fd_read/fd_pread) now pass the output pointer.
  • Multi-iovec path validates every entry before returning, so a valid-then-OOB pair can't cause a partial host write.
  • wrap()'s new RangeError → EOVERFLOW runs after the .code check, so fs errors still map through ERROR_MAP; residual RangeErrors in wrapped hostcalls are DataView/Buffer bounds failures, for which errno is strictly better than throwing into host JS.
  • Test covers single/multi iovec, both fd_write and fd_read, and the nwritten-OOB no-side-effect case with a seeded marker.
Extended reasoning...

Overview

Two-file change to the vendored wasi-js shim (src/js/node/wasi.ts) plus a subprocess test in test/js/bun/wasm/wasi.test.js. getiovs() now bounds-checks the iovec array pointer, each buf + bufLen, and the nread/nwritten output pointer up front and throws WASIError(WASI_EOVERFLOW) on any violation, replacing the previous behavior of console.loging a debug object, silently truncating the buffer, and writing the truncated bytes anyway. wrap() gains a RangeError → WASI_EOVERFLOW fallback so any remaining DataView/Buffer OOB across the ~40 wrapped hostcalls returns an errno instead of throwing into host JS. All four getiovs callers are updated to pass the output pointer.

Security risks

None introduced; the change is a net hardening. The pre-fix code let a guest force the host to console.log/console.warn debug objects and to write a truncated view of guest memory to a real host fd while reporting success — both are removed. The new preflight rejects OOB before any host-side effect. The RangeError catch is ordered after the string .code check, so it cannot mask fs errno errors, and the only realistic RangeError sources inside wrapped hostcalls are DataView/typed-array constructions over guest memory, for which EOVERFLOW is the intended mapping (matches Node's uvwasi).

Level of scrutiny

Medium. This is a Node-compat correctness fix in a JS shim (no native code, no GC/lifetime concerns). The arithmetic is done in JS doubles on u32-range values so it cannot wrap. I checked the >>> 0 !== x guards against wasm-i32-as-signed inputs: a pointer ≥ 2^31 arriving as a negative JS number now returns EOVERFLOW instead of throwing RangeError from ToIndex, which is strictly better and consistent with how the rest of this file already handles pointers. The EINVAL → EOVERFLOW errno change on OOB buf was verified against Node in the PR description.

Other factors

All prior review threads are resolved: CodeRabbit's preflight-before-I/O concern was addressed in 2f1b8a6 (output pointer now validated inside getiovs) and confirmed; the stderr-strictness thread was withdrawn; my own combined-assertion nit was applied in 7726bcc. The test drains both pipes concurrently, uses bunEnv, exercises both the single- and multi-iovec code paths and both read/write entry points, and seeds a LEAKED marker to make the no-side-effect assertion for the OOB-nwritten case explicit.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/bun/wasm/wasi.test.js`:
- Around line 98-99: Extend the WASI test helpers around call to support
fd_pread and fd_pwrite with the correct offset and output-pointer argument
order, while retaining fd_read and fd_write coverage. Add cases for
out-of-bounds nread and both positional hostcalls using a valid seekable
descriptor, covering sibling entry points and their error paths across the
complete variant matrix.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6ff2a85e-f612-414b-8fde-1ee32a4e55b5

📥 Commits

Reviewing files that changed from the base of the PR and between aba32dc and d0b00ba.

📒 Files selected for processing (2)
  • src/js/node/wasi.ts
  • test/js/bun/wasm/wasi.test.js

Comment thread test/js/bun/wasm/wasi.test.js Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — my earlier nit about the combined {stdout, stderr, exitCode} assertion was applied in 7726bcc, and both CodeRabbit findings are resolved.

What was reviewed:

  • getiovs() bounds-checks the iovec array, each buf + bufLen, and the nread/nwritten output pointer before any host I/O — no write-then-error path remains for the four iovec-based hostcalls.
  • Arithmetic in the checks (iovs + iovsLen * 8, buf + bufLen) stays within safe-integer range for all u32 inputs, so no JS-number overflow bypass.
  • The wrap() RangeError → WASI_EOVERFLOW fallback only tightens the hostcall-totality invariant; it cannot regress previously-succeeding calls.
  • Test covers all six OOB shapes across fd_write and fd_read, uses the file-local combined-object subprocess pattern, and seeds a marker to catch a write-before-error regression.
Extended reasoning...

Overview

This PR fixes node:wasi's getiovs() helper (used by fd_write/fd_pwrite/fd_read/fd_pread) to bounds-check the iovec array pointer, each iovec's buf + bufLen, and the nread/nwritten output pointer before any host I/O, throwing WASIError(WASI_EOVERFLOW) on violation. It removes the previous truncate-then-write behaviour and its console.log/console.warn debug output, and adds a RangeError → WASI_EOVERFLOW mapping to wrap() as a safety net for the ~40 other wrapped imports. A subprocess test in test/js/bun/wasm/wasi.test.js exercises six malformed shapes and asserts errno 61 with empty stderr. Two files touched, ~50 net lines.

Security risks

The change is a strict hardening: previously a hostile WASI guest could (a) crash the host with an uncaught RangeError, (b) make the host emit a {buf, bufLen, total_memory} object to stdout and a console.warn to stderr at will, and (c) get truncated bytes actually written to a host fd for an out-of-bounds iovec. All three are eliminated. The new outPtr preflight (added after CodeRabbit's review) ensures no host write occurs before the output-pointer store is known to be in-bounds, so there is no write-then-error partial side effect. I checked the arithmetic: iovs + iovsLen * 8 and buf + bufLen cannot exceed Number.MAX_SAFE_INTEGER for any u32 inputs, so the comparisons cannot be bypassed via JS-number precision loss. The >>> 0 guards correctly reject negative-when-signed i32 params.

Level of scrutiny

Medium-low. This is pure JS in a vendored/adapted module (src/js/node/wasi.ts, originally from wasi-js) implementing a Node compat surface that the file header itself calls a "quick hack" pending a native rewrite. The fix is localised to one helper plus its four call sites and one line in wrap(), matches Node.js/uvwasi's errno for every tested shape (verified in the PR description), and follows validate-first-then-act. No native code, no GC, no threading.

Other factors

All prior review feedback is closed: CodeRabbit's write-then-error concern was addressed in 2f1b8a6 by preflighting outPtr inside getiovs() (with a dedicated regression case seeding LEAKED\n), CodeRabbit withdrew its stderr-assertion concern, and my own nit about the combined {stdout, stderr, exitCode} form was applied in 7726bcc. The test lives in the existing wasi.test.js, drains pipes concurrently, uses bunEnv, and matches the neighboring test's assertion shape. The behaviour change from WASI_EINVAL to WASI_EOVERFLOW for the buf-OOB case is intentional and Node-aligned per the PR's side-by-side verification output.

@robobun

robobun commented Jul 17, 2026 •

Copy link
Copy Markdown
Collaborator Author

Rebased onto main (the July head stopped applying after #36474) and reworked in the process, see the PR description. Current head is 22e632a, three commits:

  • 64ea37c: getiovs() bounds checks with uvwasi's exact boundary rule (verified against Node v26.3.0), wrap() fallback, tests for fd_write/fd_read/fd_pwrite/fd_pread, negative i32 pointers and the accepted boundary.
  • 4c3063f: review pointed out that with wrap() mapping RangeError to an errno, path_open with an out-of-bounds fdPtr leaked the descriptor before failing and fd_seek moved the offset before failing; both check their output pointer first, with a test that FD_MAP, the directory and the offset stay untouched.
  • 22e632a: a second review pass found the same pattern in fd_tell, found that Buffer.from's ERR_BUFFER_OUT_OF_BOUNDS was being mapped to EINVAL instead of EOVERFLOW for the path_* calls, and found that the fd_read assertions were satisfied by the wrap() fallback alone. Fixed the first two; added an fd_read test through getStdin that checks nothing is consumed, plus fd_tell and path_* assertions. The hostcalls that are not wrapped at all, the partial struct writes, and the Buffer#write clamping sites are listed in the description as a follow-up change rather than folded in here.

Reproduced on the current release with USE_SYSTEM_BUN=1 bun test test/js/bun/wasm/wasi.test.js: 5 of the 6 new tests fail there (the boundary test passes by design). bun bd test test/js/bun/wasm/wasi.test.js: 11 pass with this branch's wasi.ts, 5 fail with main's, and the fd_tell/path_* assertions fail with the previous revision of this branch.

CI on 22e632a (build 98191): test/js/bun/wasm/wasi.test.js passed on every lane. The six failures in that build (bun-patch on Windows, inspect-error-leak and 13696 in the ASAN parallel batch, test-cluster-shared-leak, two napi node-gyp builds on Windows arm64) each passed on retry or when run alone and are outside anything this PR touches; the same was true of build 97763 on the previous head. Ready to merge from my side.

#35941 proposed a subset of this change and was closed in favour of this PR. The pre-existing fd_pread doubled-nread bug noted in review is fixed separately in #34472.

baima365-web added a commit to baima365-web/bun that referenced this pull request Jul 26, 2026
…ccess

- Validate iovs pointer is within memory bounds
- Validate total iovec array size doesn't overflow  
- Bounds-check each buf + bufLen combination
- Throw WASI_EOVERFLOW instead of silent truncation or RangeError

Fixes oven-sh#34468
@robobun
robobun force-pushed the claude/e4be467d/wasi-iovec-oob branch from d0b00ba to 907bb9d Compare August 15, 2026 08:05
Comment thread src/js/node/wasi.ts Outdated
Comment thread src/js/node/wasi.ts Outdated
@robobun
robobun force-pushed the claude/e4be467d/wasi-iovec-oob branch from 907bb9d to 2bfe5f5 Compare August 15, 2026 08:08
Comment thread src/js/node/wasi.ts Outdated
Comment thread src/js/node/wasi.ts

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the path_open/fd_seek output-pointer preflight from the earlier 🔴 finding is now in place (f258143) with a regression test asserting no fd is opened, no FD_MAP entry is added, no file is created, and stats.offset is not mutated on the EOVERFLOW path. The 🟣 fd_pread double-count is pre-existing and tracked as #34472.

What was reviewed:

  • checkBounds matches uvwasi_serdes_check_bounds semantics; iovsLen * 8 stays in the safe-integer range and negative i32 pointers/lengths are rejected via >>> 0.
  • All six getiovs callers plus path_open/fd_seek now preflight their output pointer before any host side effect; boundary test pins that ranges ending exactly at byteLength still succeed.
  • The two comment-cop flags look stale — the only comments this diff adds are one-liners.
Extended reasoning...

Overview

Two files: src/js/node/wasi.ts (~45 net lines: adds checkBounds, rewrites getiovs to validate the iovec array/entries/output pointer before building views, maps residual RangeError to WASI_EOVERFLOW in wrap(), and preflights fdPtr in path_open and newOffsetPtr in fd_seek) and test/js/bun/wasm/wasi.test.js (+257 lines, four new tests). Removes the previous console.log/console.warn + truncate-and-proceed behaviour in getiovs.

Security risks

WASI is a sandbox boundary, so guest-supplied pointers are untrusted. This change tightens validation — every pointer is now bounds-checked before any host I/O, and the RangeError → EOVERFLOW fallback keeps hostcalls total instead of throwing into host JS. My earlier concern (the fallback masking a post-openSync RangeError in path_open, letting a guest leak host fds while receiving errno 61) is fixed in f258143 exactly as suggested: checkBounds(fdPtr, 4, …) runs before fs.openSync, and checkBounds(newOffsetPtr, 8, …) runs before fd_seek mutates stats.offset. The new test asserts FD_MAP is unchanged, created.txt does not exist, and fd_tell still reports 0 after a rejected seek. No new attack surface; the sandbox is strictly stricter.

Level of scrutiny

Medium. This is Node-compat pure-JS in the vendored wasi-js shim — no native/GC/threading concerns. The bounds rule is copied from uvwasi and the PR description tabulates errno-for-errno agreement with Node v26.3.0. It has been through three review rounds (CodeRabbit ×2, my prior review), each addressed with a follow-up commit and matching test.

Other factors

  • Tests follow the file's existing conventions (combined {stdout, stderr, exitCode} assertion, tempDir, bunEnv, subprocess for stdout/stderr assertions) and cover the accepted boundary so an over-strict check would fail too.
  • iovsLen >>> 0 !== iovsLen gates before iovsLen * 8, and JS numbers keep u32 * 8 exact, so the length passed to checkBounds cannot overflow.
  • The pre-existing fd_pread double-count I flagged is explicitly out of scope per the PR description (#34472) and the diff does not touch those lines.
  • The two unresolved github-actions comment-cop notes predate f258143 and target paragraph-long comments; the only comments this diff adds are single lines, so they read as stale bot output rather than outstanding feedback.

…ing or logging

getiovs() read the iovec array without checking that it lies inside linear
memory, so an out-of-bounds array pointer threw a RangeError out of the
hostcall into the host, and an iovec whose buffer overran memory was
truncated, written anyway, and reported with console.log/console.warn output
on the host's stdout/stderr. The nread/nwritten output pointer was only
touched after the I/O had happened, so an out-of-bounds one left the bytes
written and then threw.

Validate the iovec array, every buf/len pair and the output pointer up front
with the same rule uvwasi applies (the start of a range must lie inside
memory, even for an empty range, and the range must not run past the end) and
return WASI_EOVERFLOW, which is what Node returns for each of these shapes.
wrap() additionally maps a RangeError escaping any other hostcall to
WASI_EOVERFLOW so a bad guest pointer always comes back as an errno.
…before the open or seek

With wrap() turning a RangeError into WASI_EOVERFLOW, path_open with an
out-of-bounds fd pointer opened the file, recorded it in FD_MAP and only then
failed storing the new fd, so every such call leaked a host descriptor while
reporting an error; fd_seek likewise moved the offset before failing. These
are the only hostcalls that change host or descriptor state before storing
through a guest pointer, so check those pointers up front like getiovs does.
@robobun
robobun force-pushed the claude/e4be467d/wasi-iovec-oob branch from f258143 to 2ec5790 Compare August 15, 2026 14:38
Comment thread src/js/node/wasi.ts Outdated
…tell's pointer first

The path hostcalls read the guest path with Buffer.from(buffer, ptr, len),
which throws a RangeError carrying ERR_BUFFER_OUT_OF_BOUNDS when the range
does not fit in memory; wrap() looked the code up in ERROR_MAP first and so
returned EINVAL where Node returns EOVERFLOW. Handle the pointer RangeErrors
before the errno lookup.

fd_tell recorded an explicit offset of 0 before storing through its result
pointer, so a rejected call still switched the descriptor from the host's
file position to positional reads. Check the pointer first, like fd_seek.
@robobun
robobun force-pushed the claude/e4be467d/wasi-iovec-oob branch from 2ec5790 to 22e632a Compare August 15, 2026 14:42

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the earlier path_open/fd_seek side-effect-before-error finding is addressed, and 22e632a extends the same preflight to fd_tell and fixes the ERR_BUFFER_OUT_OF_BOUNDS → EINVAL mismap.

What was reviewed:

  • checkBounds matches uvwasi's rule (start strictly inside memory even for len 0); ptr >>> 0 === ptr rejects negative i32s and iovsLen * 8 cannot overflow into a false pass.
  • wrap() reorder: WASIError first is behaviour-preserving (it has no .code); the RangeError branch only claims bare and ERR_BUFFER_OUT_OF_BOUNDS, so ERR_OUT_OF_RANGE and fs errno codes still reach the table.
  • Every wrapped hostcall that mutates host or descriptor state before its output store now preflights that pointer; the remaining non-preflighted callers (fd_fdstat_get, fd_filestat_get, …) have no irreversible side effect before the store and are named as follow-up.
  • Six new tests cover the rejected shapes, the accepted boundary, and the no-side-effect contract for each preflighted call; the pre-existing fd_pread double-count is tracked in #34472 and this PR's test correctly avoids asserting nread there.
Extended reasoning...

Overview

Two files: src/js/node/wasi.ts (built-in JS module, ~60 lines net) and test/js/bun/wasm/wasi.test.js (+312 lines, six new tests). The production change adds a checkBounds(ptr, len, byteLength) helper mirroring uvwasi_serdes_check_bounds, rewrites getiovs() to validate the iovec array, every buf/len pair and the caller's nread/nwritten pointer before returning, adds up-front checkBounds calls to path_open (fdPtr), fd_seek (newOffsetPtr) and fd_tell (offsetPtr), and reorders wrap() so WASIError is matched first and a bare RangeError / ERR_BUFFER_OUT_OF_BOUNDS maps to WASI_EOVERFLOW ahead of the generic errno-code lookup. The old console.log({buf, bufLen, total_memory}) + truncate + console.warn path in getiovs() is deleted.

Security risks

None introduced. The change tightens the WASI hostcall boundary: previously an out-of-bounds fdPtr on path_open opened a real host fd and recorded it in FD_MAP before throwing (and, with the earlier revision's wrap() fallback alone, would have leaked it silently per call); now the pointer is validated before fs.openSync runs. fd_seek/fd_tell no longer mutate the descriptor's offset on a rejected call. The RangeError fallback in wrap() is scoped to e.code == null || e.code === "ERR_BUFFER_OUT_OF_BOUNDS", so fs errors (which carry an errno .code) and ERR_OUT_OF_RANGE still route through ERROR_MAP as before.

Level of scrutiny

Medium. This is a Node.js-compat shim in built-in JS, not native code, and the reference behaviour (Node v26.3.0 / uvwasi) is documented case-by-case in the PR description. The change has been through three review rounds: CodeRabbit's initial pass, my 🔴 finding on path_open/fd_seek (fixed in 4c3063f), a second pass that caught fd_tell and the Buffer.from code mismap (fixed in 22e632a), and comment-cop on comment length (also 22e632a). All threads are resolved.

Other factors

  • Correctness of checkBounds: ptr >>> 0 === ptr rejects negatives and non-integers; ptr < byteLength enforces uvwasi's start-inside-memory rule for empty ranges; len <= byteLength - ptr cannot underflow because ptr < byteLength guarantees the RHS is ≥ 1. iovsLen * 8 stays well inside safe-integer range for any u32 and is rejected by the length comparison when too large.
  • The wrap() reorder is behaviour-preserving for WASIError (no .code, so it never matched the string-code branch before either).
  • Tests use tempDir, bunEnv, drain pipes concurrently, assert the combined {stdout, stderr, exitCode} object per this file's convention, cover the accepted boundary (guards against over-strict checks) and the no-side-effect contract for each preflighted call. The PR description records USE_SYSTEM_BUN=1 failing 5/6 new tests and bun bd test passing 11/11.
  • The pre-existing fd_pread double-count I flagged as 🟣 is unchanged by this diff and tracked in #34472; the fd_pread test here deliberately does not assert nread after the successful read so it does not depend on that fix.
  • Explicitly deferred (unwrapped args_get/environ_get/clock_*/poll_oneoff, partial struct writes, Buffer#write clamping) do not regress from current behaviour and reuse the checkBounds helper added here.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant