Repository navigation
Conversation
…OVERFLOW Every hostcall now validates each guest pointer against linear memory before it reads or writes memory, touches the host filesystem or registers a descriptor, and reports a pointer that does not fit as WASI_EOVERFLOW, which is what Node (uvwasi) returns. Previously the pointer was dereferenced as-is: most hostcalls threw a RangeError out of the import into the running guest, args_get/environ_get, fd_prestat_dir_name, path_readlink and poll_oneoff truncated the write and reported success, fd_fdstat_get/fd_filestat_get wrote part of the struct before failing, path pointers came back as EINVAL, and path_open with a bad fd pointer opened (or created) the file and leaked the descriptor in FD_MAP. args_get, args_sizes_get, environ_get, environ_sizes_get, clock_res_get, clock_time_get, poll_oneoff and random_get were not wrapped in wrap(), so they are now, and the args/environ pairs share one implementation so the sizes reported by *_sizes_get are exactly what *_get checks and writes.
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Comment |
|
Status: reproduced and fixed; waiting on CI.
|
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #34468, though the two are adjacent and meant to land together. #34468 fixes the iovec decoding used by
The intentional overlap is the bounds helper plus the |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it systematically touches ~25 WASI hostcalls on the guest/host sandbox boundary, refactors the args/environ paths, and adds a compiled .wasm binary artifact, a human look would still be worthwhile.
What was reviewed:
CHECK_BOUNDSmatchesuvwasi_serdes_check_boundssemantics (start-in-bounds even for len 0, u32-only, no overflow viabyteLength - ptr);nsubscriptions * 48overflow is caught by thelen >>> 0 === lenclause.- Every check is placed before the first guest-memory read, fs call, or
FD_MAP/stats.offsetmutation, so rejected calls have no side effects (verified forpath_open,fd_seek,fd_filestat_get). - The
writeStringTable/writeStringTableSizesrefactor preserves the old byte-count arithmetic (NUL now inside the string vs.+ 1before). - Wrapping
clock_res_get/poll_oneoffinwrap()doesn't change their non-WASIErrorthrow paths (wrap re-throws those); test rows work on Windows becauseCHECK_BOUNDSfires before any symlink-dependent fs call.
Extended reasoning...
Overview
This PR adds guest-pointer bounds checking to every pointer-taking hostcall in src/js/node/wasi.ts (Bun's node:wasi shim, derived from wasi-js). It introduces a CHECK_BOUNDS(ptr, len) helper mirroring Node's uvwasi_serdes_check_bounds, calls it once per pointer at the top of ~25 hostcalls with the correct preview1 struct sizes, wraps the 8 previously-unwrapped hostcalls in wrap() so the thrown WASIError becomes an errno, and refactors args_get/environ_get and their _sizes_get twins to share writeStringTable/writeStringTableSizes so reported sizes, checked sizes, and written bytes derive from one source. Tests add a 51-row OOB table asserting EOVERFLOW with no memory/FD_MAP/directory side effects, a 17-call boundary test pinning the accepted edge, and a compiled freestanding wasm guest (source + 1.2 KB binary) exercising the end-to-end path.
Security risks
WASI is a sandbox boundary: the guest's only access to the host is through these hostcalls, and pointer arguments are guest-controlled offsets into linear memory. The change is strictly a hardening — it adds validation where there was none, and the checks are ordered before any host-fs call or FD_MAP mutation, closing the path_open fd leak the PR documents. I checked that no check was placed after a side effect it should guard, and that the >>> 0 guards reject the negative-i32 shape a wasm guest produces for addresses ≥ 2³¹. The nsubscriptions * 48 product in poll_oneoff cannot wrap into a small accepted value because JS numbers don't wrap and the len >>> 0 === len clause rejects anything outside [0, 2³²). No new attack surface is introduced; the risk is regression (an over-strict check rejecting a valid call), which the boundary test guards against.
Level of scrutiny
High. This is a systematic change across the entire WASI hostcall surface in a Node-compat module, it sits on a security boundary, it refactors four hostcalls' bodies, and it ships a binary .wasm artifact whose contents can only be verified by rebuilding from the provided C source. Per the repo's Node/Web-compat guidance, behavior here is defined by Node's node_wasi.cc, and while the PR description cross-references every row against Node v26.3.0, a maintainer should confirm the struct sizes (fdstat 24, filestat 64, prestat 8, subscription 48, event 32) and the decision to keep CHECK_FD before CHECK_BOUNDS (documented divergence from Node's ordering).
Other factors
The test coverage is unusually thorough — every changed hostcall has at least one OOB row, memory-unchanged and FD_MAP-unchanged are asserted, and the boundary test prevents over-tightening. The PR description explicitly scopes out three adjacent issues (fd_prestat_dir_name ENOBUFS, fd_readdir header overrun, ≥2 GiB addressing) and the overlap with #34468. I found no correctness issues, but the breadth of the change and the binary artifact push this past what I'd auto-approve.
|
Two pointers for whoever takes the human look:
|
|
Updated 11:05 AM PT - Aug 15th, 2026
❌ @robobun, your commit 3acd950 has some failures in 🧪 To try this PR locally: bunx bun-pr 39100That installs a local version of the PR into your bun-39100 --bun |
Problem
node:wasihostcalls dereference the pointers a guest passes them without checking that they fit in the guest's linear memory. Node returns the WASI errnoEOVERFLOW(61) for every such pointer; on Bun 1.4.0 the outcome depends on which hostcall and which pointer (full table in the details below):args_get,args_sizes_get,environ_get,environ_sizes_get,clock_res_get,clock_time_get,poll_oneoffandrandom_getare not wrapped inwrap(), so theRangeError: Out of bounds accessfrom theDataView/Uint8Arrayescapes the import and unwinds through the running guest (thepointer-validation-guest.wasmtest below dies this way on 1.4.0). Thefd_*/path_*hostcalls are wrapped, butwrap()only convertsWASIErrors and errors with a stringcode, so theirRangeErrorescapes too.args_get,environ_get,fd_prestat_dir_name,path_readlinkandpoll_oneoffwrite throughBuffer#writeor with a 16 byte stride, so a buffer that runs past the end of memory is silently truncated and the call returns success.fd_fdstat_get,fd_filestat_get,path_filestat_get,fd_readdir,args_sizes_get,environ_sizes_get,poll_oneoffandpath_readlinkstore field by field, so a struct that straddles the end of memory is partially written before the failure.Buffer.from(buffer, ptr, len), whose error has acode, so everypath_*hostcall reports an out-of-bounds path asEINVAL(28).path_openstores the new descriptor last, so an out-of-boundsfdpointer opens the file (creating it underO_CREAT), registers it inFD_MAPand then fails: one leaked host descriptor per call, and the guest never learns the number. The probe in the details leaks 3 descriptors and creates a file on 1.4.0.src/js/node/wasi.tsvalidates a guest pointer before use, except what node:wasi: return EOVERFLOW for out-of-bounds iovecs instead of throwing/logging #34468 adds (see the relationship bullet under Fix).Fix
CHECK_BOUNDS(ptr, len)next toCHECK_FDand calls it at the top of every hostcall that takes a pointer, once per pointer, with the size the hostcall will actually access: the fixed preview1 struct sizes (fdstat_t24,filestat_t64,prestat_t8,u32/u64outputs 4/8,subscription_t48 andevent_t32 per entry forpoll_oneoff), the guest-supplied length for paths and buffers, and forargs_get/environ_getthe same byte counts thatargs_sizes_get/environ_sizes_getreport. The checks run after the fd checks and before the hostcall reads guest memory, calls into the filesystem or touchesFD_MAP, so a rejected call has no side effects (thepath_openleak included).wrap()so theWASIErrorbecomes the errno like everywhere else.proc_exit/proc_raise/sched_yield/sock_*take no pointers and are unchanged.args_get/args_sizes_getandenviron_get/environ_sizes_getnow sharewriteStringTable/writeStringTableSizes, so the sizes reported to the guest, the sizes checked, and the bytes written are computed from the same strings.node_wasi.ccrunsuvwasi_serdes_check_boundson every pointer argument before calling into uvwasi, and that function's rule is the oneCHECK_BOUNDSimplements: the start of the range has to lie inside memory even when the range is empty, and the range must not run past the end. Every row of the table below was checked against Node v26.3.0, including the accepted boundary (ranges that end exactly at the end of memory, and empty ranges on the last byte, still succeed) and the two edges that differ from a plainptr + len <= sizecheck (an empty range starting at the end of memory is rejected; a pointer or length that is not a u32, which is what a wasm guest delivers for an address >= 2**31, is rejected). The compiled guest in the test prints byte for byte the same output under Node.EBADFwhere Node givesEOVERFLOW(same call as node:wasi: return EOVERFLOW for out-of-bounds iovecs instead of throwing/logging #34468);fd_prestat_dir_namewith an in-bounds buffer that is too short still truncates (Node returnsENOBUFS), andfd_readdir's own entry serialization can still run a few bytes pastbuf_lenwhen an entry header does not fit; both are separate from pointer validation and are being reported separately, as is support for addresses >= 2 GiB in larger memories (previously aRangeError, nowEOVERFLOW; the hostcalls would need to reinterpret the i32 as unsigned to actually address them).fd_read/fd_write/fd_pread/fd_pwrite(not touched here) and, as of its latest revision, also checks thefd_seek/fd_tell/path_openoutput pointers and adds awrap()fallback that maps aRangeErrortoEOVERFLOW. The fallback does not reach the 8 hostcalls that are not wrapped, and it cannot undo the truncated or partial writes listed above, which is why the checks here run before the work; the two PRs overlap only on the bounds helper and the three output-pointer lines, and whichever lands second drops its copies.test/js/bun/wasm/wasi.test.js:61from every row, guest memory unchanged after each call, andFD_MAPand the preopened directory unchanged afterwards. On 1.4.0 all 51 rows differ.pointer-validation-guest.wasm(source inpointer-validation-guest.c, 1.2 KB, freestanding, build command in the header) reads argv/environ the way a libc start-up does, then makes seven calls across five hostcalls with the end-of-memory address or0xfffffff0and prints the errnos. Expected output isargs: guest --flag/environ: K=v/errnos: 61 61 61 61 61 61 61; on 1.4.0_startthrowsRangeError: Out of bounds accesswith nothing printed. Node v26.3.0 prints the same three lines.bun bd test test/js/bun/wasm/wasi.test.js: 8 pass. Withsrc/stashed: 2 fail (the table and the guest), 6 pass.USE_SYSTEM_BUN=1: the same 2 fail.oxlint,prettier --checkandtsc --noEmitare clean on the touched files.Background
i32offsets into the instance's linear memory (memory.buffer), and it reports failure by returning an errno number; a guest compiled against wasi-libc cannot catch a JS exception thrown out of an import, so an exception there terminates the program. Bun's implementation issrc/js/node/wasi.ts(derived fromwasi-js), wherewrap()turns errors thrown inside a hostcall into errnos; Node's isnode_wasi.ccon top of uvwasi, a C library.args_get/environ_getfill two guest buffers whose sizes the guest first asks for viaargs_sizes_get/environ_sizes_get: a table of u32 pointers and the NUL-terminated strings they point at.poll_oneoffreadsnsubscriptions48 byte subscription records and writes up to that many 32 byte event records.path_openwrites the new descriptor number through its last argument;FD_MAPis the per-instance table from guest descriptor numbers to host descriptors.errno for each probed shape: Node v26.3.0 / Bun 1.4.0 / this branch
Probe setup:
args: ["argv0", "b"],env: { K: "v" }, one preopen on fd 3 containingfile.txt,link -> file.txtandsubdir,file.txtopened on its own fd, a 1 page memory soEND = 65536,OOB = END + 1000. "memory written" means guest memory differed after a call that did not return success.On 1.4.0 the probe also ends with
FD_MAPgrown from 5 to 8 entries andnew-entrycreated in the preopen (the threepath_openrows); on this branch both are unchanged. Thefd_prestat_dir_name ... too shortandpath_create_directory ... END-1 len 0rows pass bounds checking and exercise behavior this PR leaves alone. Node allocatesnsubscriptionsrecords before bounds-checking, so the "nsubscriptions huge" row has no Node value (the process died with SIGSEGV).