fix(wasi): resolve path_open against the preopen dir, not cwd - #30303
Closed
robobun wants to merge 2 commits into
Closed
fix(wasi): resolve path_open against the preopen dir, not cwd#30303robobun wants to merge 2 commits into
robobun wants to merge 2 commits into
Conversation
Every other path_* handler in src/js/node/wasi.ts resolves the guest
path against the preopen dirfd's host path via path.resolve(stats.path, p).
path_open ignored the stats it fetched and did path.resolve(p), which
resolves relative to process.cwd(), so a WASM program reading
'/work/file' under preopens: { '/work': hostDir } would fall back to
<cwd>/file instead of <hostDir>/file.
Capture stats from CHECK_FD and use stats.path as the base when present.
Fixes #30302.
Collaborator
Author
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Disabled knowledge base sources:
WalkthroughThis PR updates WASI ChangesWASI Path Resolution Against Preopened Directories
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/node/wasi.ts (1)
1550-1554:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not swallow
path_openerrors and still return success.The inner
catchlogs and then falls through toWASI_ESUCCESS, which can falsely report success on open/rights failures.Suggested fix
- } catch (e) { - console.error(e); - } + } catch (e) { + throw e; + } return constants_1.WASI_ESUCCESS;🤖 Prompt for 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. In `@src/js/node/wasi.ts` around lines 1550 - 1554, The catch inside the path_open implementation currently just console.error(e) and then falls through to return constants_1.WASI_ESUCCESS, falsely reporting success; update the catch in the path_open function to not swallow the error but instead map the thrown Node error to the proper WASI errno and return that (or rethrow to be handled upstream). Locate the path_open handler (the block that returns constants_1.WASI_ESUCCESS) and replace the bare console.error(e) with logic that derives a WASI errno from the error (use any existing errno-mapping helper or inspect e.code/e.errno) and return that constant instead of WASI_ESUCCESS, or rethrow the original error if callers expect exceptions.
🤖 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/preopen-wasi.c`:
- Around line 77-83: The computed total bytes variable can exceed the actual
buffer capacity because total = plen + nread doesn't account for the clamp in
the copy loop; update the calculation of total before creating write_iov so it
is clamped to the number of bytes actually copied (e.g., total = min(plen +
nread, (u32)sizeof(out_buf)) or compute total = plen + actual_copied) so
fd_write / write_iov only sees the valid byte count; adjust the total variable
used by struct ciovec write_iov and any downstream uses (variables: total, plen,
nread, out_buf, write_iov, fd_write).
In `@test/js/bun/wasm/wasi.test.js`:
- Around line 72-81: The test should ignore the known ASAN startup warning in
the subprocess stderr and assert the process exit code after validating
filesystem/content; update the block that reads proc.stderr.text() and
proc.exited (variables stderr, exitCode, proc) to filter out lines that start
with or contain "WARNING: ASAN interferes" (or similar ASAN startup text) from
stderr before asserting it, perform the file content check against
Bun.file(join(workDir, "output.txt")).text() first, and only then assert
expect(exitCode).toBe(0); ensure the stderr assertion uses the filtered value
(expect(filteredStderr).toBe("")).
---
Outside diff comments:
In `@src/js/node/wasi.ts`:
- Around line 1550-1554: The catch inside the path_open implementation currently
just console.error(e) and then falls through to return
constants_1.WASI_ESUCCESS, falsely reporting success; update the catch in the
path_open function to not swallow the error but instead map the thrown Node
error to the proper WASI errno and return that (or rethrow to be handled
upstream). Locate the path_open handler (the block that returns
constants_1.WASI_ESUCCESS) and replace the bare console.error(e) with logic that
derives a WASI errno from the error (use any existing errno-mapping helper or
inspect e.code/e.errno) and return that constant instead of WASI_ESUCCESS, or
rethrow the original error if callers expect exceptions.
🪄 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: 72f73b7a-42ef-4cbf-ad0e-5511d7f2475b
⛔ Files ignored due to path filters (1)
test/js/bun/wasm/preopen-wasi.wasmis excluded by!**/*.wasm
📒 Files selected for processing (3)
src/js/node/wasi.tstest/js/bun/wasm/preopen-wasi.ctest/js/bun/wasm/wasi.test.js
Comment on lines
+77
to
+83
| for (u32 i = 0; i < nread && plen + i < sizeof(out_buf); i++) { | ||
| out_buf[plen + i] = read_buf[i]; | ||
| } | ||
| u32 total = plen + nread; | ||
|
|
||
| struct ciovec write_iov = { out_buf, total }; | ||
| u32 nwritten = 0; |
Contributor
There was a problem hiding this comment.
Clamp total to the number of bytes actually copied into out_buf.
nread is bounded during copy, but total = plen + nread is not. This can cause fd_write to read past valid output bytes.
Suggested fix
- for (u32 i = 0; i < nread && plen + i < sizeof(out_buf); i++) {
+ u32 copied = 0;
+ for (u32 i = 0; i < nread && plen + i < sizeof(out_buf); i++) {
out_buf[plen + i] = read_buf[i];
+ copied++;
}
- u32 total = plen + nread;
+ u32 total = plen + copied;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (u32 i = 0; i < nread && plen + i < sizeof(out_buf); i++) { | |
| out_buf[plen + i] = read_buf[i]; | |
| } | |
| u32 total = plen + nread; | |
| struct ciovec write_iov = { out_buf, total }; | |
| u32 nwritten = 0; | |
| u32 copied = 0; | |
| for (u32 i = 0; i < nread && plen + i < sizeof(out_buf); i++) { | |
| out_buf[plen + i] = read_buf[i]; | |
| copied++; | |
| } | |
| u32 total = plen + copied; | |
| struct ciovec write_iov = { out_buf, total }; | |
| u32 nwritten = 0; |
🤖 Prompt for 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.
In `@test/js/bun/wasm/preopen-wasi.c` around lines 77 - 83, The computed total
bytes variable can exceed the actual buffer capacity because total = plen +
nread doesn't account for the clamp in the copy loop; update the calculation of
total before creating write_iov so it is clamped to the number of bytes actually
copied (e.g., total = min(plen + nread, (u32)sizeof(out_buf)) or compute total =
plen + actual_copied) so fd_write / write_iov only sees the valid byte count;
adjust the total variable used by struct ciovec write_iov and any downstream
uses (variables: total, plen, nread, out_buf, write_iov, fd_write).
Comment on lines
+72
to
+81
| const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); | ||
|
|
||
| // The WASM program proc_exits(0) on success; non-zero encodes which step | ||
| // failed. See test/js/bun/wasm/preopen-wasi.c. | ||
| expect(stderr).toBe(""); | ||
| expect(exitCode).toBe(0); | ||
|
|
||
| // The preopen points at <cwd>/work, so the output file must land there, | ||
| // with "got: " prefixed to the host-dir input's contents. | ||
| expect(await Bun.file(join(workDir, "output.txt")).text()).toBe("got: hello from host file"); |
Contributor
There was a problem hiding this comment.
Normalize ASAN stderr noise and assert exitCode last.
This subprocess test uses bunEnv, so stderr should ignore the known ASAN startup warning, and exitCode should be asserted after content/filesystem checks.
Suggested fix
- const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]);
+ const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]);
+ const filteredStderr = stderr
+ .split("\n")
+ .filter(line => !line.startsWith("WARNING: ASAN interferes"))
+ .filter(Boolean)
+ .join("\n");
@@
- expect(stderr).toBe("");
- expect(exitCode).toBe(0);
+ expect(filteredStderr).toBe("");
@@
expect(await Bun.file(join(workDir, "output.txt")).text()).toBe("got: hello from host file");
+ expect(exitCode).toBe(0);Based on learnings: assert command exit code last in Bun tests, and filter WARNING: ASAN interferes lines from subprocess stderr when using bunEnv.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); | |
| // The WASM program proc_exits(0) on success; non-zero encodes which step | |
| // failed. See test/js/bun/wasm/preopen-wasi.c. | |
| expect(stderr).toBe(""); | |
| expect(exitCode).toBe(0); | |
| // The preopen points at <cwd>/work, so the output file must land there, | |
| // with "got: " prefixed to the host-dir input's contents. | |
| expect(await Bun.file(join(workDir, "output.txt")).text()).toBe("got: hello from host file"); | |
| const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); | |
| const filteredStderr = stderr | |
| .split("\n") | |
| .filter(line => !line.startsWith("WARNING: ASAN interferes")) | |
| .filter(Boolean) | |
| .join("\n"); | |
| // The WASM program proc_exits(0) on success; non-zero encodes which step | |
| // failed. See test/js/bun/wasm/preopen-wasi.c. | |
| expect(filteredStderr).toBe(""); | |
| // The preopen points at <cwd>/work, so the output file must land there, | |
| // with "got: " prefixed to the host-dir input's contents. | |
| expect(await Bun.file(join(workDir, "output.txt")).text()).toBe("got: hello from host file"); | |
| expect(exitCode).toBe(0); |
🤖 Prompt for 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.
In `@test/js/bun/wasm/wasi.test.js` around lines 72 - 81, The test should ignore
the known ASAN startup warning in the subprocess stderr and assert the process
exit code after validating filesystem/content; update the block that reads
proc.stderr.text() and proc.exited (variables stderr, exitCode, proc) to filter
out lines that start with or contain "WARNING: ASAN interferes" (or similar ASAN
startup text) from stderr before asserting it, perform the file content check
against Bun.file(join(workDir, "output.txt")).text() first, and only then assert
expect(exitCode).toBe(0); ensure the stderr assertion uses the filtered value
(expect(filteredStderr).toBe("")).
Collaborator
Author
|
Duplicate of #27724 (same 2-line fix, same test strategy). Letting the older PR go first. |
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
1 participant
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.
What
node:wasipath_openwas resolving the guest path relative toprocess.cwd()instead of the preopen directory entry it wasgiven. With
preopens: { "/work": hostDir }, a WASM program thatopened
/work/file.txtfell back to<cwd>/file.txt.Reproduction
Issue #30302 — a Rust WASI program built with
rustc --target wasm32-wasip1that reads
DUCKFLIX_WASI_REQUEST(=/work/request.json) fails withENOENT: no such file or directory, open '<cwd>/request.json'. Theissue reporter's workaround was
process.chdir(workspace)beforewasi.start(), which linesprocess.cwd()up with the preopen andpapers over the bug.
Cause
src/js/node/wasi.ts, line 1515 inpath_open:CHECK_FD(dirfd, ...)already returns the preopen dirfd entry, whosestats.pathis the host directory, but the return value was discarded.Every other
path_*handler in the same file uses the stats correctly:path_create_directory,path_filestat_get,path_filestat_set_timespath_link,path_readlink,path_remove_directorypath_rename,path_symlink,path_unlink_fileAll do
path.resolve(stats.path, p); onlypath_opendidn't.Fix
Capture
statsfromCHECK_FDand resolve againststats.path:The
stats.pathguard matches the defensive pattern used by the otherhandlers (
if (!stats.path) return WASI_EINVAL); preopen entriesalways set
pathwhen registered at line 853.Test
test/js/bun/wasm/wasi.test.jsgains a regression test that runs atiny C/WASI program (
test/js/bun/wasm/preopen-wasi.c, pre-built aspreopen-wasi.wasm) whichpath_opensinput.txtunder the preopenand writes
got: <contents>tooutput.txtunder the same preopen.The test plants a decoy
input.txtat the parent process's cwd sothe broken cwd-relative path also resolves — guaranteeing the test
checks that the output lands in the mapped host dir, not just that
it happens to land somewhere.
Gate
USE_SYSTEM_BUN=1 bun test test/js/bun/wasm/wasi.test.js→ fails(ENOENT on
<workDir>/output.txt)bun bd test test/js/bun/wasm/wasi.test.js→ passesFixes #30302