Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 44 additions & 3 deletions src/runtime/node/node_process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,26 @@ mod _impl {
if let Some(worker) = vm.worker_ref() {
// was explicitly overridden for the worker?
if let Some(exec_argv) = worker.exec_argv() {
// The exec_argv slice borrows `StringImpl*` owned by the
// parent-thread `WorkerOptions::execArgv` vector. Handing one to
// `BunString::init` and then `to_js` would `String(impl)`-ref it
// from this worker thread and let JSC take further refs on it
// (atomization, rope resolution), which is a cross-thread hazard
// on a StringImpl that is not thread-safe. Copy the bytes into a
// worker-local impl instead, same as the C++ side does for
// `options.name.isolatedCopy()` in createNodeWorkerThreadsBinding.
Comment thread
robobun marked this conversation as resolved.
Outdated
return JSValue::create_array_from_iter(global_object, exec_argv.iter(), |&wtf| {
BunString::init(wtf).to_js(global_object)
// SAFETY: non-null entries borrow live storage in the
// parent `WorkerOptions` (see `WebWorker::exec_argv`).
let impl_ = unsafe { &*wtf };
let s = if impl_.is_8bit() {
BunString::clone_latin1(impl_.latin1_slice())
} else {
BunString::clone_utf16(impl_.utf16_slice())
};
let r = s.to_js(global_object);
s.deref();
r
});
}
}
Expand Down Expand Up @@ -332,7 +350,16 @@ mod _impl {

// argv omits "bun" because it could be "bun run" or "bun" and it's kind of ambiguous
// argv also omits the script name
let mut args_list: Vec<BunString> = Vec::with_capacity(args_count + 2);
// `deref` on every element on scope exit: a no-op for ZigString/Static
// tags, and releases the +1 held by `clone_*` in the worker branch.
Comment thread
robobun marked this conversation as resolved.
Outdated
let mut args_list = scopeguard::guard(
Vec::<BunString>::with_capacity(args_count + 2),
|v| {
for a in &v {
a.deref();
}
},
);

if vm.standalone_module_graph.is_some() {
// Don't break user's code because they did process.argv.slice(2)
Expand Down Expand Up @@ -369,8 +396,22 @@ mod _impl {
}

if let Some(worker) = worker {
// The argv slice borrows `StringImpl*` owned by the parent-thread
// `WorkerOptions::argv` vector. Wrapping one in `BunString::init`
// lets `to_js_array` `String(impl)`-ref it from this worker thread
// and hand JSC a shared impl it may further ref (atomize, resolve a
// rope into), which is a cross-thread hazard on a StringImpl that is
// not thread-safe. Copy the bytes into a worker-local impl instead,
// same as the C++ side does for `options.name.isolatedCopy()`.
Comment thread
robobun marked this conversation as resolved.
Outdated
for &arg in worker.argv() {
args_list.push(BunString::init(arg));
// SAFETY: non-null entries borrow live storage in the parent
// `WorkerOptions` (see `WebWorker::argv`).
let impl_ = unsafe { &*arg };
args_list.push(if impl_.is_8bit() {
BunString::clone_latin1(impl_.latin1_slice())
} else {
BunString::clone_utf16(impl_.utf16_slice())
});
}
} else {
for arg in &vm.argv {
Expand Down
29 changes: 29 additions & 0 deletions test/js/web/workers/worker-argv-cross-thread-fixture.test.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 18 additions & 0 deletions test/js/web/workers/worker.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,24 @@ describe("worker_threads", () => {
expect(process.execArgv).toEqual(original_execArgv);
});

// A web Worker with an explicit argv/execArgv whose worker thread reads
// process.argv, followed by a node:worker_threads Worker in the same
// process, used to crash on Windows (STATUS_STACK_BUFFER_OVERRUN) because
// building process.argv wrapped the parent-thread StringImpl instead of
// copying it. The crash only reproduced under `bun test`, so spawn the
// fixture as a test subprocess; a crash surfaces as a non-zero exit instead
// of taking out this test runner.
test("web Worker argv followed by worker_threads Worker does not crash", async () => {
await using proc = Bun.spawn({
cmd: [bunExe(), "test", path.join(import.meta.dir, "worker-argv-cross-thread-fixture.test.ts")],
env: bunEnv,
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toContain("2 pass");
expect(exitCode).toBe(0);
});

test("worker with eval = false validates the filename", () => {
// eval:false is equivalent to omitting eval, so a bare string that isn't a
// path is rejected synchronously like Node (ERR_WORKER_PATH), rather than
Expand Down
Loading