Repository navigation
Conversation
poll_oneoff wrote each event as userdata, errno and type followed by 5 bytes of padding and advanced the output pointer by 16 bytes. A preview1 event is 32 bytes (fd_readwrite follows at offset 16), and guests read events[i] at out + 32 * i, so with more than one subscription the second event was written into the first event's fd_readwrite field and the second event's slot was never written. Write the whole record (with fd_readwrite zeroed) in one place after the subscription switch and advance by 32.
|
Warning Review limit reached
Next review available in: 9 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 selected for processing (2)
Comment |
|
Status: fix and test pushed, waiting on CI. Reproduced on the released build by calling |
There was a problem hiding this comment.
LGTM — corrects the poll_oneoff event stride from 16 to the preview1-mandated 32 bytes and consolidates the two duplicated write blocks into one.
What was reviewed:
- Verified the refactor is behavior-preserving apart from the stride: the clock arm's
typebyte was alreadyWASI_EVENTTYPE_CLOCK, thedefaultarm still early-returns before the shared write, and movingshortPause()before the write is inert. - Checked field offsets (userdata@0, errno@8, type@10, nbytes@16, flags@24, size 32) against
__wasi_event_t/ uvwasi serdes. - Confirmed the test avoids the separate
waitTimeNsBigInt/Number bug by using a 0ns relative timeout, and avoids the 50ms stdin pause via a no-opsleep. - Padding bytes (11-15, 26-31) are left as-is rather than zeroed — that's fine per ABI (guests don't read padding) and not a regression.
Extended reasoning...
Overview
This PR fixes a layout bug in src/js/node/wasi.ts's poll_oneoff implementation: event records were being written with a 16-byte stride instead of the 32 bytes the WASI preview1 ABI specifies for __wasi_event_t. With ≥2 subscriptions (the common case for wasi-libc poll()/select()), event N+1 was written into event N's fd_readwrite field and the guest read garbage from the actual event N+1 slot. The fix hoists the record write out of the two switch arms into one block that writes all five fields at their spec offsets and advances sout by 32. A new test in test/js/bun/wasm/wasi.test.js exercises four subscriptions covering fd_read, fd_write, a valid clock, and an invalid clockid, with the output area pre-filled with 0xaa so both under-write and over-write show up.
Security risks
None. This is a serialization-layout fix inside the JS-implemented WASI shim. The guest already controls the sout pointer into its own linear memory; the change only alters which offsets within the guest-provided buffer are written and by how much the pointer advances. No host filesystem, credential, or sandbox-escape surface is touched.
Level of scrutiny
Low-to-medium. The diff is ~20 net lines in a self-contained function, mechanically derived from the published preview1 struct layout (cross-referenced against wasi-libc's sizeof(__wasi_event_t) == 32 and uvwasi's UVWASI_SERDES_SIZE_event_t == 32). I traced each removed write against the new shared block: the clock arm previously wrote WASI_EVENTTYPE_CLOCK as the type byte, which equals type inside that case; the fd arm previously wrote WASI_ENOSYS as errno, now assigned to e; nevents += 1 and the sin - last_sin != 48 sanity check are preserved; the default: early-return means e is never read uninitialized. Reordering shortPause() to before the write is harmless — it only calls this.sleep and touches no memory or pointers.
Other factors
- The test is well-constructed: 0ns relative timeout keeps
waitTimeNsat 0 so the known BigInt-minus-Number bug a few lines below (explicitly called out as separate, #34470) is not tripped;sleep() {}neutralizes the stdinshortPause(); the spare 32-byte record after the last event guards against overrun; assertions are exact-valuetoEqualon all five decoded fields. - No CODEOWNERS entry covers
src/js/node/wasi.ts. - The only stylistic nit is that padding bytes at offsets 11-15 and 26-31 remain whatever the guest left there rather than being zeroed; the ABI treats them as padding and the old code left far more garbage, so this is not a regression and not worth blocking on.
|
On the padding bytes (11 to 15 and 26 to 31): leaving them alone is deliberate. The ABI treats them as padding, and Node's |
|
Updated 12:14 PM PT - Aug 15th, 2026
🔄 @robobun, the build for your commit |
Problem
node:wasi'spoll_oneoffwrites each event as userdata (8) + errno (2) + type (1) + 5 bytes of padding and advances the output pointer by 16 (src/js/node/wasi.ts, thesout += 8/2/1/5sequences in both the clock arm and the fd_read/fd_write arm, lines 1538 to 1560 on main).eventis 32 bytes, so the guest readsevents[i]fromout + 32 * i. With two or more subscriptions, event 1 is written atout + 16, which isevents[0].fd_readwrite, and the memory ofevents[1]is never written: the guest sees whatever was there before.poll()andselect()submit the fd subscriptions plus one clock subscription for the timeout. A single subscription (sleep()) hides the bug, which is whyhello-wasi.wasmstyle tests never caught it.Fix
switchnow only compute the errno for the event; one block after theswitchwrites the whole record (userdata @0, errno @8, type @10,fd_readwrite.nbytes@16 and.flags@24, both zero) and advancessoutby 32.eventin the preview1 ABI (sizeof(__wasi_event_t) == 32in wasi-libc,UVWASI_SERDES_SIZE_event_t == 32in the uvwasi serdes Node uses to write these records), so this is also what Node'snode:wasiproduces.fd_readwritematches what uvwasi reports for an fd event that carries an errno (this implementation always reports fd events asENOSYS); clock events ignore the field, so zeroing it there only makes the output deterministic.switchis behavior preserving: both arms wrote userdata, errno and type and bumpednevents, and the clock arm's type byte was thecaselabel, i.e.type. It also leaves one place that encodes the record layout.test/js/bun/wasm/wasi.test.js, "poll_oneoff writes one 32-byte preview1 event record per subscription". Four subscriptions (fd_read, clock, fd_write, clock with an invalid clockid) cover both arms and theEINVALclock path; the event area plus one spare record is pre-filled with0xaa, then the four decoded records are compared with onetoEqualand the spare record is checked to still be0xaa. Fails on the released build (event 0 reportsnbytes: 2n, slots 2 and 3 are still0xaa), passes with this change.fd_readwritezeroed (output below).waitTimeNs -= Bun.nanoseconds() - timeOriginBigIntTypeErrora few lines further down is a separate bug, fixed in node:wasi: fix poll_oneoff TypeError on clock subscriptions #34470; this change does not touch those lines. node: implement node:wasi v26 WASI class surface (+23 tests) #35709 (a largernode:wasirework, currently a draft against a non-main branch) contains the same stride fix as part of its diff.Background
wasi_snapshot_preview1) is the syscall ABI that wasm programs built with wasi-libc, Rust'swasm32-wasip1target, etc. import;node:wasiimplements those imports on top of the host. Structs are passed through the guest's linear memory at fixed little-endian offsets, so the host has to write exactly the layout the guest's C headers describe.poll_oneoff(in, out, nsubscriptions, nevents)is the one blocking primitive in preview1:inpoints atnsubscriptions48-bytesubscriptionrecords (userdata u64, type u8, then a clock or fd payload),outat a guest-allocated array ofnsubscriptions32-byteeventrecords (userdata u64 @0, errno u16 @8, type u8 @10,fd_readwrite { nbytes u64 @16, flags u16 @24 }), and the host stores the number of records it filled in throughnevents. wasi-libc buildssleep(),poll()andselect()on top of it.ENOSYS. This PR does not change that; it only fixes the shape of what gets written.Freestanding wasm guest (fd_read on fd 0 + monotonic clock, timeout 0), event buffer pre-filled with 0xaa
Released 1.4.0:
With this change:
Node 26 running the same guest also lays its records out 32 bytes apart (it reports only the clock event, since uvwasi actually polls the fd; that difference is pre-existing and out of scope here).