worker: copy argv/execArgv into worker-local StringImpls - #36393
Conversation
When a Worker is created with an explicit argv or execArgv option, the strings are stored in the parent thread's WorkerOptions vector and a raw pointer to that storage is handed to the Rust WebWorker. When the worker thread later builds process.argv/process.execArgv, it wrapped each parent-owned StringImpl* directly in a BunString and handed it to JSC. That lets JSC on the worker thread take further refs on a StringImpl that is not thread-safe, which on Windows release reliably crashed with STATUS_STACK_BUFFER_OVERRUN once a second worker loading node:worker_threads was created before the first was GC'd. Copy each entry's bytes into a fresh worker-local StringImpl instead, mirroring the isolatedCopy() the C++ side already does for options.name in createNodeWorkerThreadsBinding. This showed up in CI as the parallel test batch reporting an unrelated test file (most often test/regression/issue/20875.test.ts on Windows aarch64) as 'worker crashed: exit code 9', because the crashing worker process happened to have that file inflight when the panic fired.
WalkthroughChangesWorker argument safety
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 2:11 PM PT - Jul 29th, 2026
❌ @robobun, your commit ca97075 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 36393That installs a local version of the PR into your bun-36393 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@test/js/web/workers/worker-argv-cross-thread-fixture.test.ts`:
- Around line 21-29: Update the Worker promise in the “worker_threads Worker
after the above” test to reject on the worker’s error event or an exit occurring
before a message is received, while preserving successful message resolution.
Move worker termination into a finally block so the worker is terminated on both
success and failure paths.
- Around line 9-18: Update the worker lifecycle in the test around the Worker
instance and message promise: wrap the await and assertions in try/finally,
terminate the worker in finally, and await termination before the test completes
so teardown is guaranteed even when message handling or assertions fail.
🪄 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: 44e28c41-5001-4057-a1c8-acfc19dd02e1
📒 Files selected for processing (3)
src/runtime/node/node_process.rstest/js/web/workers/worker-argv-cross-thread-fixture.test.tstest/js/web/workers/worker.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/runtime/node/node_process.rs:210-219— nit: theunsafe { &*ptr }; if impl_.is_8bit() { clone_latin1(...) } else { clone_utf16(...) }block (and its ~7-line explanatory comment) is duplicated verbatim betweencreate_exec_argvandcreate_argv. Per REVIEW.md ("The second time a multi-line block appears in your diff, extract a named helper"), consider a small local helper — e.g.fn clone_impl_isolated(impl_: *const WTFStringImpl) -> BunString— carrying the isolated-copy comment once, called from both sites.Extended reasoning...
What
This diff introduces the same multi-line block twice:
In
create_exec_argv(exec_argv worker branch):let impl_ = unsafe { &*wtf }; let s = if impl_.is_8bit() { BunString::clone_latin1(impl_.latin1_slice()) } else { BunString::clone_utf16(impl_.utf16_slice()) };
In
create_argv(argv worker branch):let impl_ = unsafe { &*arg }; args_list.push(if impl_.is_8bit() { BunString::clone_latin1(impl_.latin1_slice()) } else { BunString::clone_utf16(impl_.utf16_slice()) });
Additionally, both sites carry a near-identical 7–8 line explanatory comment about the parent-thread
StringImpl*cross-thread hazard and theisolatedCopy()analogy.Why flag it
REVIEW.md → Code style & idioms reviewers enforce → "Simplest honest shape; deduplicate within your own diff":
The second time a multi-line block appears in your diff, extract a named helper and use it at EVERY parallel site.
This is a direct match: the same 5-line branching block and the same rationale comment appear at two parallel sites introduced by this PR.
Is there an existing helper?
I checked whether an in-tree helper already covers this ("grep for the in-tree helper before hand-writing anything"):
BunString::to_thread_safe()/BunString__toThreadSafeoperates on a&mut BunStringthat is already taggedWTFStringImpl— constructing thatBunStringfrom the raw parent-thread pointer would first requireBunString::init(wtf), which refs the parent impl, defeating the whole point of this PR.WTFStringImplStruct(insrc/bun_alloc/lib.rs) exposesis_8bit(),latin1_slice(),utf16_slice()but no "clone bytes into a fresh impl without touching the source refcount" method.
So there is no existing helper; a small local one (or a new method on
WTFStringImplStruct, e.g.isolated_clone(&self) -> BunString) is the appropriate dedup.Step-by-step
- PR adds worker-branch handling in
create_exec_argv— introduces theis_8bit → clone_latin1/clone_utf16block plus the explanatory comment. - PR adds the same handling in
create_argv— introduces the identical block plus a near-verbatim copy of the comment. - Two occurrences of the same multi-line block within one diff → REVIEW.md rule applies.
- Suggested shape:
Both call sites then collapse to
/// Copy a parent-thread StringImpl's bytes into a fresh worker-local impl /// without touching the source refcount — the isolatedCopy() equivalent /// for a raw `*const WTFStringImpl` we may only read, not ref. fn clone_impl_isolated(ptr: *const WTFStringImpl) -> BunString { // SAFETY: caller guarantees ptr borrows live parent WorkerOptions storage. let impl_ = unsafe { &*ptr }; if impl_.is_8bit() { BunString::clone_latin1(impl_.latin1_slice()) } else { BunString::clone_utf16(impl_.utf16_slice()) } }
clone_impl_isolated(wtf)/clone_impl_isolated(arg), and the long cross-thread-hazard comment lives once on the helper.
Impact / severity
Purely code organization — no behavioral difference, no correctness issue. Marking nit: not worth blocking merge, but it's a specific pattern the repo's review guide calls out, and consolidating the comment on the helper makes the invariant ("never ref a parent-thread StringImpl from the worker") easier to keep enforced if a third site appears later.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/runtime/server/DirectoryRoute.rs:299-321—open_beneathonly jails symlink resolution on Linux (openat2_in_rootwithRESOLVE_IN_ROOT|NO_MAGICLINKS); on macOS/Windows it falls through to plainbun_sys::openatwith noO_NOFOLLOWand no post-open realpath containment check, so a symlink inside the served root (e.g.public/etc -> /etc) letsGET /static/etc/passwdescape the root on those platforms while Linux confines it.resolve_subpath's checks are purely lexical on the URL and cannot see filesystem symlinks — REVIEW.md → Security: "lexical containment is defeated by symlinks — re-verify after realpath, prefer O_NOFOLLOW-style atomic flags." (This file also looks unrelated to the worker-argv fix and may have been staged accidentally; if it's removed per the other comment this is moot for this PR, but the gap should be closed beforeDirectoryRoutelands anywhere.)Extended reasoning...
What the bug is
DirectoryRoute::open_beneath(src/runtime/server/DirectoryRoute.rs:299-321) is the sole gatekeeper between a URL-derived relative path and the filesystem. Its own doc comment promises "openat2(RESOLVE_IN_ROOT|NO_MAGICLINKS)on Linux,openatelsewhere" — and that is exactly the problem. The Linux/Android branch callsbun_sys::openat2_in_root(root_fd, zrel, flags, 0), which usesRESOLVE_IN_ROOT | RESOLVE_NO_MAGICLINKSso every symlink component is re-rooted underroot_fdand cannot escape. Every other platform (macOS, Windows, the BSDs) falls through to plainbun_sys::openat(root_fd, zrel, flags, 0)withO::RDONLY | O::CLOEXEC [| O::NONBLOCK]— noO_NOFOLLOW, noO_NOFOLLOW_ANY(macOS 11+), and no post-openrealpath/fcntl(F_GETPATH)check that the resolved path is still under the root.Why
resolve_subpathdoesn't catch itresolve_subpathis thorough for what it does — it rejects..,%2F, encoded pchars, empty/.segments, NUL, backslash, and colon — but every one of those checks operates on the URL bytes, before any filesystem call. It has no view of whatetcis on disk. A URL of/static/etc/passwddecodes to the relative pathetc/passwd, which is two clean non-dot segments and passes every check.Step-by-step proof
Setup on macOS:
Bun.serve({ routes: { "/static/*": { dir: "./public" } } })where./publiccontainsln -s /etc etc.- Client sends
GET /static/etc/passwd. resolve_subpath(b"/static/etc/passwd", b"/static/", out)strips the prefix →after_prefix = b"etc/passwd". No%, no.., no empty segments, no NUL/\\/:. ReturnsSome((10, false));rel = b"etc/passwd".open_subpath(b"etc/passwd", false)callsopen_beneath(b"etc/passwd").- On macOS the
#[cfg(not(any(linux, android)))]branch runs:bun_sys::openat(root_fd, "etc/passwd\0", O_RDONLY|O_CLOEXEC|O_NONBLOCK, 0). The kernel resolvesetcrelative toroot_fd, finds the symlink, follows it to/etc, then opens/etc/passwd.openatonly scopes the starting directory — it does not confine symlink targets. fstatreports a regular file;S::ISREGis true; the response streams/etc/passwdwithcontent-type: application/octet-streamand a 200.
On Linux the identical request hits
openat2_in_root, which resolves the absolute symlink target/etcrelative toroot_fd(i.e.<public>/etc), so it either loops on itself or fails ENOENT — the client gets a 404. Same input, different security outcome, silently, by platform.Why this matters
REVIEW.md → Security is explicit on both counts:
- "lexical containment is defeated by symlinks — re-verify after realpath, prefer O_NOFOLLOW-style atomic flags over check-then-act."
- "Security checks fail closed and cover every path to the protected effect" — a check that only exists on one
#[cfg]branch does not cover every path.
Whether symlink-following is acceptable for a static server is a design choice (nginx and npm
sendfollow by default). What is not acceptable is that the function is namedopen_beneath, the Linux branch honors that name, and the macOS/Windows branch silently does not. A user who tests containment on Linux and deploys on macOS gets an escape they never observed.How to fix
Pick one policy and enforce it everywhere:
- Confine (match Linux): on macOS add
O_NOFOLLOW_ANY(Big Sur+) to the open flags, or fall back to per-componentopenat(..., O_NOFOLLOW)walking. On Windows, open withFILE_FLAG_OPEN_REPARSE_POINTand reject reparse points, or after opening callGetFinalPathNameByHandleWand verify the result is still under the root's final path. On BSDs,O_RESOLVE_BENEATH(FreeBSD 13+) is the direct analogue. - Follow (match macOS today): drop
RESOLVE_IN_ROOTon Linux so all platforms follow, and document that the served directory is trusted. This is the weaker option and contradicts the function name.
Either way, the platform divergence should be closed before this file lands.
Note on scope
DirectoryRoute.rsis a 712-line new file with no connection to the worker-argvStringImplfix this PR is titled for; it appears to have been staged accidentally (see the separate comment). If the file is removed from this PR, this finding is moot here — but it applies to whatever PR eventually landsDirectoryRoute. - Client sends
-
🔴
src/runtime/server/DirectoryRoute.rs:1— This PR's diff adds a brand-new 712-linesrc/runtime/server/DirectoryRoute.rsimplementing static-directory HTTP serving forBun.serve, which is entirely unrelated to the worker argv/execArgv thread-safety fix — it isn't mentioned in the title, description, or any commit, isn't wired intosrc/runtime/server/mod.rs, and has no callers or tests in this PR. This looks like an accidentalgit addof WIP from another branch; please drop it from this PR and land it separately with its own review and tests.Extended reasoning...
What the issue is
The PR is a focused ~40-line thread-safety fix: copy worker
argv/execArgvStringImplbytes into worker-local storage instead of handing parent-thread-owned impls to JSC. The diff, however, also introduces a completely unrelated new file:diff --git a/src/runtime/server/DirectoryRoute.rs b/src/runtime/server/DirectoryRoute.rs new file mode 100644 index 000000000000..79977e1f11ca --- /dev/null +++ b/src/runtime/server/DirectoryRoute.rs @@ -0,0 +1,712 @@ +//! Serve a directory tree at a URL prefix: `"/static/*": { dir: "./public" }`.This is a 712-line implementation of static-directory HTTP serving for
Bun.serve(aDirectoryRoutetype with path-traversal validation, ETag/Last-Modified handling, range requests, a stat cache, and ~120 lines of unit tests). It has zero relationship to worker argv handling.Step-by-step proof it's an accidental inclusion
- Not mentioned anywhere. PR title: "worker: copy argv/execArgv into worker-local StringImpls". PR description: entirely about the
StringImplcross-thread crash. The three commits on the branch are1ee738b4(the fix),23480cc9([autofix.ci]), and88bf79ae("review: factor clone into a helper; harden fixture error handling") — none mentionDirectoryRoute,Bun.serve, or static file serving. - Not wired into the build.
src/runtime/server/mod.rsdeclaresstatic_route,file_route,file_response_stream,html_bundle, etc. — but has nomod directory_route/mod DirectoryRouteline.grep -r DirectoryRoute src/returns nothing. Cargo does not compile a.rsfile that isn't reachable from amoddeclaration, so this file is a dead orphan — it wouldn't even be type-checked bybun bd. - No history.
git log --all -- src/runtime/server/DirectoryRoute.rsreturns nothing.ls src/runtime/server/on the checked-out branch tip does not list it. This is consistent with a stale hunk in the GitHub diff (e.g. staged from another worktree/branch and later force-pushed away, or a mis-staged file that never landed in the branch's commit graph but is still in GitHub's cached compare). - No tests or callers in this PR. Nothing in
test/references it;ServerConfig.rsdoesn't parse a{ dir: ... }route option; no other file in the diff touches it.
Why this should block
Even though the file is currently unreachable from
mod.rs(so it wouldn't run), merging it still lands 712 lines of unreviewed feature code in-tree under a PR whose review record says "worker argv StringImpl fix". The next PR that wires it intomod.rswill look like a one-line change against "already-merged" code, bypassing the review a new HTTP static-file server (path-traversal validation, symlink handling, RFC 3986 parsing, security-sensitive by REVIEW.md's own §Security) actually needs.REVIEW.md is explicit on both counts:
- "don't ride file-wide standardization on a focused bugfix" — this rides a whole new subsystem on one.
- "New cross-cutting abstractions need maintainer agreement before appearing inside a feature PR."
Fix
Remove
src/runtime/server/DirectoryRoute.rsfrom this PR (git rm/ drop the hunk) and open it as its own PR with themod.rswiring,ServerConfigoption parsing, andtest/js/bun/http/integration tests it needs. The rest of this PR (thenode_process.rschange + worker fixture) is self-contained and unaffected. - Not mentioned anywhere. PR title: "worker: copy argv/execArgv into worker-local StringImpls". PR description: entirely about the
|
CI build 85399: the diff is green for what this PR fixes. Neither The two hard failures are pre-existing main flakes currently hitting many unrelated branches:
Everything else passed on retry alone ( |
There was a problem hiding this comment.
Both prior findings (fixture .test.ts discovery, missing error/exit reject wiring) are addressed. The refcount handling checks out — deref() is a no-op for non-WTFStringImpl tags so the new scopeguard in create_argv is safe on the borrow_utf8/static_ entries, and the exec_argv closure derefs unconditionally so the error path doesn't leak. Deferring only because this is cross-thread StringImpl lifetime in the native runtime; worth a maintainer glance at clone_parent_worker_option_string.
What was reviewed:
clone_latin1/clone_utf16return a fresh worker-local impl at +1;to_jsrefs, caller derefs — matches the existing standalone-graph branch pattern in the same function.- Reading
is_8bit()/latin1_slice()/utf16_slice()off the parentStringImpl*touches only construction-time-immutable fields (flag bit + payload ptr/len), not the refcount. - Fixture rename keeps it out of
isTestStrictdiscovery; wrapper spawns it by explicit path.
Extended reasoning...
Overview
Targeted thread-safety fix in src/runtime/node/node_process.rs: when a worker builds process.argv/process.execArgv from parent-supplied options, it now copies each entry's bytes into a worker-local WTFStringImpl (via BunString::clone_latin1/clone_utf16) instead of wrapping the parent thread's StringImpl* directly. A helper clone_parent_worker_option_string was factored out and applied at both call sites (argv and execArgv). A scopeguard was added to create_argv to release the +1 from the clones. New subprocess-driven fixture + wrapper test in worker.test.ts.
Security risks
None. No user-controlled parsing, auth, or trust-boundary changes — this is internal string ownership across a worker thread boundary.
Level of scrutiny
High — this is native memory-safety code in the cross-thread StringImpl category REVIEW.md flags as most-blocked. I verified: (1) String::deref() is gated on Tag::WTFStringImpl so the scopeguard is a no-op on the ZigString/Static entries pushed by the non-worker branches (src/bun_core/string/mod.rs:512); (2) the exec_argv closure stores to_js()'s JsResult before calling deref(), so the +1 is released even when to_js throws; (3) the clone pattern mirrors the existing scopeguard+clone_utf8+to_js_array sequence already present in the standalone-graph branch of create_exec_argv; (4) latin1_slice/utf16_slice on WTFStringImplStruct read only m_hash_and_flags (8-bit flag set at construction) and m_ptr/m_length — all immutable after construction, so the cross-thread read doesn't race the parent's refcount.
Other factors
My two prior findings and CodeRabbit's were addressed in 88bf79a/c63aedfa: the fixture was renamed to drop .test. so CI's isTestStrict no longer discovers it, both fixture tests now wire error/exit to reject and terminate in finally, and the clone was factored into a named helper. The comment-cop bot flags on the helper doc-comment were pushed back on reasonably (it documents the thread-safety invariant, not a workaround). The two robobun CI failures (worker-transfer-terminate-stress, proxy-stress-protocol) are on x64-asan and unrelated to this diff. Deferring rather than approving because cross-thread WTF string handling is exactly the area where a maintainer with JSC context should confirm the reasoning.
|
This bug was independently root-caused in #37140, now closed as a duplicate of this PR. Two findings from that investigation that may help land this one:
|
|
Superseded by #37075, which landed the same fix: The original symptom this PR was chasing ( Closing. |
2 participants
What
When a Worker is created with an explicit
argvorexecArgvoption, the strings are stored in the parent thread'sWorkerOptionsvector and a raw pointer to that storage is handed to the RustWebWorker. When the worker thread later buildsprocess.argv/process.execArgv, it wrapped each parent-ownedStringImpl*directly in aBunStringand handed it to JSC viato_js_array. That lets JSC on the worker thread take further refs on aStringImplthat is not thread-safe.This change copies each entry's bytes into a fresh worker-local
StringImplinstead, mirroring theisolatedCopy()that the C++ side already does foroptions.nameincreateNodeWorkerThreadsBinding.Why
On Windows release this reliably crashed with
STATUS_STACK_BUFFER_OVERRUN(0xC0000409) in the following sequence, underbun test:Single-character argv entries don't crash because JSC's single-char small-string cache returns a singleton and never touches the input
StringImpl. InsertingBun.gc(true)between the two workers also avoids the crash (it collects the firstWorkerwrapper and itsWorkerOptionsvector).In CI this showed up as the Windows parallel test batch reporting whichever unrelated file was inflight on the crashing worker as
worker crashed: exit code 9(0xC0000409 & 0xff). On Windows aarch64 it was almost alwaystest/regression/issue/20875.test.ts; on x64 it rotated between20875,09279,02499, and08965. The actual trigger is theargv / execArgv optionstest intest/js/web/workers/worker.test.ts, which runs in the same shard.Verification
Windows x64 release,
worker-argv-cross-thread-fixture.test.ts:-1073740791)Windows x64 release, 21-file subset of the CI shard at
--parallel=2:worker crashed: exit code 9test/js/web/workers/worker.test.ts(26 tests) and theexecArgv optionsuite intest/js/node/worker_threads/pass on both Linux debug+ASAN and Windows release.[review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file