worker_threads: copy argv/execArgv for the worker thread instead of sharing the parent's StringImpls - #37140
worker_threads: copy argv/execArgv for the worker thread instead of sharing the parent's StringImpls#37140robobun wants to merge 1 commit into
Conversation
…haring the parent's StringImpls Worker::create passed the WorkerOptions argv/execArgv Vector<String> contents to the worker as raw WTF::StringImpl pointers. The worker thread wrapped them in worker-heap JSStrings (process.argv / process.execArgv), and any property-key use atomized the shared impl in the worker's thread-local AtomStringTable. When the parent's GC swept the Worker wrapper while the worker thread was still exiting, ~WorkerOptions did the final deref on the parent thread and aborted in AtomStringImpl::remove: ASSERTION FAILED: wasRemoved The string being removed is an atom in the string table of an other thread! In release builds on Windows that abort is a silent 0xC0000409 fastfail; it took down bun test --parallel workers on the Windows CI lanes (whichever test file was on-CPU got the blame, e.g. zlib-estimated-size-gc.test.ts). Copy argv/execArgv into worker-owned UTF-8 at create() (parent thread), the same treatment name and preloadModules already get. The worker now builds its JSStrings from owned bytes, so no parent impl can be atomized on the worker thread, and the borrowed-pointer lifetime coupling to the C++ WorkerOptions is gone. The shell's $N cache and the exec-argv scan drop their per-element WTF conversions since the bytes are already UTF-8.
|
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 (7)
Comment |
|
Windows validation (Server 2019 x64, debug build of this branch):
|
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Closing as a duplicate of #36393, which fixed the same bug first: the worker argv/execArgv StringImpls are shared by pointer with the worker thread, get atomized there, and the parent's final deref aborts in AtomStringImpl::remove (silent 0xC0000409 fastfail on Windows release). The parts of this investigation that were new, the current CI blame pattern and a deterministic regression test that fails on the debug+ASAN build, are posted on #36393 for adoption; the branch stays up for cherry-picking. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| unsafe { bun_core::ffi::slice(ptr, len) } | ||
| .iter() | ||
| .map(|&s| { | ||
| // SAFETY: each element is a live `WTF::StringImpl*` (see above). | ||
| unsafe { &*s }.to_utf8().slice().to_vec().into_boxed_slice() | ||
| }) | ||
| .collect() | ||
| }; |
There was a problem hiding this comment.
🔴 copy_args does unsafe { &*s }.to_utf8() on each WTF::StringImpl* with no null check, but a null impl is reachable: coerceToIsolatedString returns String() for Symbol() (no description), so new Worker('x', {eval:true, argv:[Symbol()]}) (or execArgv) segfaults the parent thread inside WebWorker__create. The pre-PR consumers guarded this exact case (!arg.is_null() in VirtualMachine.rs, if arg.is_null() { continue; } in jsc_hooks.rs); both guards were removed and the copy site that replaces them has none — skip null entries (or push an empty Box<[u8]>) in copy_args.
Extended reasoning...
What the bug is
The new copy_args closure in WebWorker::create iterates the C++ Vector<WTF::String> (received as *const WTFStringImpl, i.e. *const *mut WTFStringImplStruct) and unconditionally dereferences each element:
.map(|&s| {
// SAFETY: each element is a live `WTF::StringImpl*` (see above).
unsafe { &*s }.to_utf8().slice().to_vec().into_boxed_slice()
})WTFStringImpl is pub type WTFStringImpl = *mut WTFStringImplStruct (bun_alloc/lib.rs:984) — a nullable raw pointer. When s is null, unsafe { &*s } is immediate UB, and to_utf8() reads m_hash_and_flags through it → segfault.
How null reaches this code path
JSWorker.cpp:303-305 defines:
auto coerceToIsolatedString = [lexicalGlobalObject](JSValue v) -> String {
String original = v.isSymbol() ? asSymbol(v)->tryGetDescriptiveString().value_or(String()) : v.toWTFString(lexicalGlobalObject);
return original.isolatedCopy();
};For Symbol() (no description), the uid is a null symbol, so tryGetDescriptiveString() returns std::nullopt (every other in-tree caller — BunProcess.cpp, ErrorCode.cpp — branches on has_value() with a fallback string, confirming this). .value_or(String()) yields a null-impl WTF::String, and .isolatedCopy() on a null String stays null. No exception is thrown, so RETURN_IF_EXCEPTION doesn't fire and options.argv.append(str) stores a null-impl entry.
validateArray only checks that the container is an array (min length 0); it does not validate element types. The existing test file even carries // TODO(@190n) get our handling of non-string array elements in line with Node's, acknowledging non-string elements are reachable.
Worker.cpp then reinterpret_casts Vector<WTF::String> to WTF::StringImpl** and passes it as argv_ptr/exec_argv_ptr to WebWorker__create. A WTF::String IS a RefPtr<StringImpl>, so a null-impl String becomes a null StringImpl* array element on the Rust side.
Why this is a regression
The pre-PR code guarded exactly this condition, and this diff removes both guards:
VirtualMachine.rs(removed):!arg.is_null() && is_bootstrap_flag(...)jsc_hooks.rs(removed):if arg.is_null() { continue; }
Before this PR, the null was stored on the WebWorker and either skipped by these guarded scans, or only crashed lazily on the worker thread if process.argv was actually read. After this PR, the deref happens eagerly on the parent thread, inside the Worker constructor, before the worker thread is even spawned — strictly worse. Per REVIEW.md: "Delete defensive code only when you can show the condition cannot occur" and "user-reachable failures are recoverable errors, never panics".
Step-by-step proof
- User runs
new Worker('1', { eval: true, execArgv: [Symbol()] }). JSWorker.cppvalidateArraypasses (it's an array of length 1 ≥ 0).forEachInIterablecallscoerceToIsolatedString(Symbol())→tryGetDescriptiveString()returnsnullopt→String()(null impl) →.isolatedCopy()→ still null. No exception.execArgv.append(String{null}).Worker::createcallsWebWorker__create(..., reinterpret_cast<StringImpl**>(execArgv.begin()), 1, ...). Element 0 is a nullStringImpl*.- Rust
copy_args(exec_argv_ptr, 1)builds a slice[null: *mut WTFStringImplStruct]and doesunsafe { &*null }.to_utf8()→ null-pointer deref → segfault on the parent thread.
Fix
Restore the null skip in copy_args, mirroring the removed guards:
unsafe { bun_core::ffi::slice(ptr, len) }
.iter()
.filter_map(|&s| {
// A `WTF::String` may have a null impl (e.g. `Symbol()` in
// argv/execArgv via `coerceToIsolatedString`); skip it, matching
// the pre-existing consumer guards this replaces.
core::ptr::NonNull::new(s).map(|nn| unsafe { nn.as_ref() }.to_utf8().slice().to_vec().into_boxed_slice())
})
.collect()(or push an empty Box<[u8]> for null if positional alignment matters — for execArgv scanning it doesn't, and Node stringifies Symbol() to "Symbol()", so either is closer than a crash).
| .map(|&s| { | ||
| // SAFETY: each element is a live `WTF::StringImpl*` (see above). | ||
| unsafe { &*s }.to_utf8().slice().to_vec().into_boxed_slice() | ||
| }) |
There was a problem hiding this comment.
🟡 nit: the UTF-8 round-trip here replaces lone surrogates with U+FFFD — to_utf8() on a 16-bit impl routes through convert_utf16_to_utf8_append → decode_utf16_with_fffd, so new Worker(src, {argv:['\uD800foo']}) now sees process.argv[2].charCodeAt(0) === 0xFFFD where it was 0xD800 before (the old direct-wrap path preserved code units exactly). REVIEW.md's boundary-representation section calls out WTF-8 for lone surrogates specifically. Exotic trigger, so not blocking; consider storing Box<[u16]> for 16-bit impls (or a WTF-8-preserving encode) as a follow-up.
Extended reasoning...
What the bug is
The new copy_args closure converts each argv/execArgv WTF::StringImpl to owned bytes via WTFStringImplExt::to_utf8(), and the worker later reconstructs a JSString via BunString::borrow_utf8(arg).to_js(). For a 16-bit impl, to_utf8() (src/bun_core/string/wtf.rs:60) calls strings::to_utf8_alloc(self.utf16_slice()) → convert_utf16_to_utf8_append (src/bun_core/lib.rs:1603). When simdutf returns Status::SURROGATE, that falls back to append_wtf8_from_utf16 (line 1590), whose doc comment reads "Unpaired surrogates are replaced with U+FFFD" and whose body calls decode_utf16_with_fffd — returning (0xFFFD, 1) for a lone lead or trail surrogate (lines 1503-1511). The replacement happens before encoding, so the stored bytes are EF BF BD (UTF-8 for U+FFFD), not the WTF-8 ED A0 80 for U+D800. borrow_utf8().to_js() on the worker side then decodes standard UTF-8 and the original code unit is unrecoverable.
Why this is a regression
Before this PR, node_process.rs did BunString::init(wtf).to_js(), which wraps the parent's StringImpl* directly in a worker-heap JSString — the exact code-unit sequence was preserved byte-for-byte. That direct sharing is precisely the atomization bug this PR fixes, so the fidelity loss is a direct consequence of the change, not a pre-existing behavior. Node.js also preserves lone surrogates in worker argv (options are structured-cloned, which round-trips WTF-16 exactly).
Why nothing prevents it
The PR description says "Behavior parity checked … (unicode, empty strings)", but valid Unicode round-trips fine through this path — only unpaired surrogates hit the FFFD replacement branch, and no test in the diff exercises one. REVIEW.md's "Validate representation at every boundary" section explicitly names "WTF-8 helpers for lone surrogates (real Windows paths contain them)" as the class to guard.
Step-by-step proof
- Parent runs
new Worker(src, { eval: true, argv: ['\uD800foo'] }). WebWorker::create→copy_argsiterates the argv slice; the element is a 16-bitStringImplwith code units[0xD800, 0x66, 0x6F, 0x6F].to_utf8()sees!is_8bit()→to_utf8_alloc([0xD800, 0x66, 0x6F, 0x6F]).- simdutf returns
SURROGATE;append_wtf8_from_utf16callsdecode_utf16_with_fffd([0xD800, ...])→(0xFFFD, 1), thenencode_wtf8_runewritesEF BF BD. Remaining ASCII appends as66 6F 6F. StoredBox<[u8]>=[0xEF, 0xBF, 0xBD, 0x66, 0x6F, 0x6F]. - Worker thread:
create_argv→BunString::borrow_utf8(arg).to_js()decodes those bytes as UTF-8 → JS string"\uFFFDfoo". process.argv[2].charCodeAt(0)yields0xFFFD; on the previous release it yields0xD800.
Impact
Narrow: a JS string containing a lone surrogate passed through worker argv/execArgv is silently altered. The realistic case is a Windows path with an unpaired surrogate (real dirents can contain them) forwarded via argv. Exotic enough not to block a fix for a CI-killing memory-safety abort, but it is a genuine Node-compat / data-fidelity regression introduced by this change.
Suggested fix
Either keep 16-bit impls as Box<[u16]> and go through BunString::borrow_utf16() on the worker side, or use a WTF-8-preserving encode (encode the surrogate code point itself, not U+FFFD) paired with a WTF-8-aware to_js path. Both avoid the atomization bug (worker owns the bytes) while preserving code-unit fidelity.
1 participant
Problem
bun test --parallelworkers on the Windows CI lanes have been dying with a silent NTSTATUS 0xC0000409 (fastfail) since Aug 4, blamed on whichever test file was on CPU at the time, most oftentest/js/node/zlib/zlib-estimated-size-gc.test.ts(builds 88721 through 89988, both the 2019 x64 and 11 aarch64 lanes). Reproducing the crashed shard's parallel bucket on a Windows machine hit the same crash on the first attempt, and a debug build turned the silent fastfail into the real error:Cause
Worker::createhanded theWorkerOptionsargv/execArgv strings to the worker as rawWTF::StringImplpointers (web_worker.rsstored them as borrowedargv_ptr/exec_argv_ptr). The worker thread wrapped those impls in worker-heap JSStrings forprocess.argv/process.execArgv, and any property-key use of one (obj[arg], in the CI casenode:tlsscanning execArgv) atomized the shared impl in place, inserting it into the worker's thread-localAtomStringTable.If the worker thread is still exiting when the parent GC sweeps the
Workerwrapper,~WorkerOptionsperforms the final deref on the parent thread andAtomStringImpl::removehits itsRELEASE_ASSERT(the atom belongs to another thread's table). On Windows release builds the UCRT abort surfaces as a silent__fastfail0xC0000409, which killed the parallel test worker with no output. The race needs the sweep to land between the worker's exit event and its thread-local destruction, which is why it showed up on the slow 3-core CI boxes and not in local runs.This is the same bug class fixed for
options.namein #34140 (Worker.cppnow doesjsString(vm, options.name.isolatedCopy())).Fix
Copy argv/execArgv into worker-owned UTF-8 in
WebWorker__create(on the parent thread, same treatment asnameandpreloadModules). The worker builds its JSStrings from the owned bytes, so no parent-owned impl can ever be atomized on the worker thread, and the borrowed-pointer coupling to the C++WorkerOptionslifetime is gone. Consumers simplify:process.argv/process.execArgvcreation copies bytes into fresh impls (BunString::borrow_utf8(...).to_js)--no-addonsparse hook lose their unsafe per-element WTF derefs$Nlazily-populated UTF-8 cache (vm_args_utf8) is deleted: the bytes are already UTF-8Verification
SCHED_IDLE(chrt -i), and run a full GC on the first microtask after the exit event. Without the fix it aborts inAtomStringImpl::removeevery run (8/8); with the fix it passes (5/5).taskset/chrtare best-effort, so the fixture still exercises the path on other platforms.bun bd test test/js/node/worker_threads/worker_threads.test.ts(92 pass),test/js/web/workers/worker.test.ts(25 pass),test/js/bun/shell/bunshell.test.ts(503 pass),test/js/node/no-addons.test.ts.process.argv/process.execArgvvalues (unicode, empty strings) and shell$0..$Nexpansion inside workers.Windows validation of the original parallel-bucket repro is running and will be posted below.