Skip to content

fix(wasi): fix random_get and implement WASI#getImportObject - #34036

Closed
vouillon wants to merge 2 commits into
oven-sh:mainfrom
vouillon:claude/wasi-random-get-and-import-object
Closed

vouillon wants to merge 2 commits into
oven-sh:mainfrom
vouillon:claude/wasi-random-get-and-import-object

Conversation

@vouillon

Copy link
Copy Markdown

What does this PR do?

This fixes several bugs in WASI function random_get and implements WASI#getImportObject

How did you verify your code works?

Some tests have been added. I have also run actual WASI programs that where failing previously (but working with ofher Wasm engine).

@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.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai

coderabbitai Bot commented Jul 12, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

WASI now validates supported versions, selects version-specific import namespaces, and fills guest memory with chunked cryptographic randomness. Tests cover namespace selection, invalid versions, WASM instantiation, bounds handling, chunking, and sentinel preservation.

WASI runtime behavior

Layer / File(s) Summary
Versioned WASI import objects
src/js/node/wasi.ts, test/js/bun/wasm/wasi.test.js
The constructor validates "unstable" and "preview1" versions, getImportObject() selects the matching namespace, and tests verify invalid versions and WASM instantiation.
Chunked random_get memory writes
src/js/node/wasi.ts, test/js/bun/wasm/wasi.test.js
random_get validates bounds, fills guest memory in 65,536-byte chunks, returns WASI_ESUCCESS, and tests verify large buffers, EINVAL, and untouched surrounding bytes.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main changes: fixing random_get and adding WASI#getImportObject.
Description check ✅ Passed The description includes both required sections and covers the change plus verification at a high level.
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.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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: 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 27-53: Strengthen the random_get test around wasiImport.random_get
by stubbing or spying on the RNG to produce deterministic bytes, then assert it
is invoked with the expected chunk sizes covering all 70000 bytes. Replace the
partial some(...) checks with verification that every byte in the requested
buffer matches the deterministic output, while preserving the guard-byte
assertions.
🪄 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: 79aca729-b82f-4dc5-b231-bfe46d54fcec

📥 Commits

Reviewing files that changed from the base of the PR and between 2e2230a and a86adbf.

📒 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
@vouillon
vouillon force-pushed the claude/wasi-random-get-and-import-object branch from a86adbf to abeded2 Compare July 12, 2026 12:11

@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 `@src/js/node/wasi.ts`:
- Around line 1854-1861: Update random_get to validate bufPtr and bufLen against
this.memory.buffer.byteLength before constructing the Uint8Array, returning the
appropriate WASI errno for invalid or out-of-bounds guest memory instead of
allowing RangeError to escape. Preserve the existing chunked
crypto.getRandomValues behavior for valid ranges.
🪄 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: c602003e-f4e7-4d2a-8791-4afca48e4034

📥 Commits

Reviewing files that changed from the base of the PR and between a86adbf and abeded2.

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

Comment thread src/js/node/wasi.ts
vouillon added 2 commits July 12, 2026 14:33
random_get called crypto.getRandomValues(this.memory.buffer, bufPtr,
bufLen), but getRandomValues takes a single integer-typed TypedArray, so
the ArrayBuffer + extra args never filled the buffer. It also returned
bufLen instead of WASI_ESUCCESS, which the guest reads as an errno, and
would throw QuotaExceededError for requests over 65536 bytes.

Fill a Uint8Array view over [bufPtr, bufPtr+bufLen) in 64 KiB chunks and
return WASI_ESUCCESS.
Adds Node's getImportObject(), which returns the import object keyed by
the WASI namespace for the configured version:
{ wasi_snapshot_preview1: this.wasiImport } by default, or
{ wasi_unstable: this.wasiImport } for version: 'unstable'.

Track the previously-ignored version constructor option and validate it
via $ERR_INVALID_ARG_VALUE.

Fixes oven-sh#28534
@vouillon
vouillon force-pushed the claude/wasi-random-get-and-import-object branch from abeded2 to 3ba0c89 Compare July 12, 2026 12:34

@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 `@src/js/node/wasi.ts`:
- Around line 1856-1859: Update the validation immediately before the Uint8Array
construction in the WASI buffer path to reject bufPtr and bufLen unless both are
finite safe integers and non-negative. Include these integer checks in the
existing bounds validation so invalid values return WASI_EINVAL before creating
the view, while preserving valid boundary handling.
🪄 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: 1d06bc14-ae3e-4bf0-9143-0108d3d4cf48

📥 Commits

Reviewing files that changed from the base of the PR and between abeded2 and 3ba0c89.

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

Comment thread src/js/node/wasi.ts
@robobun

robobun commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Thank you for this!

The core random_get bug (wrong memory view + missing WASI_ESUCCESS return) was fixed in #34431: src/js/node/wasi.ts:1848-1854 now creates the Uint8Array view over the WASM memory buffer correctly and returns WASI_ESUCCESS.

The getImportObject() addition and the other enhancements (>65536 chunking, bounds checks, version option) are new functionality rather than bug fixes. If you'd like to pursue those separately we'd be happy to take a look, but closing this one as the titular bug is fixed on main. Thanks again for the thorough patch!

@robobun robobun closed this Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants