From 56d0a5b7842458bb5b82af412a26989e2e08d62e Mon Sep 17 00:00:00 2001 From: Ciro Spaciari MacBook Date: Fri, 17 Jul 2026 15:20:58 -0700 Subject: [PATCH] worker_threads: keep error.code when the thrown value cannot be cloned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A worker that throws something structured-clone can't serialize falls back to sending only the message text, and the parent rebuilds a bare Error from it — losing `code`. Bun's own ResolveMessage is exactly that case: it carries the right code and is not even an Error instance. new Worker(`require("node:internal/freelist")`, { eval: true }) node: code ERR_UNKNOWN_BUILTIN_MODULE bun : code undefined (the worker-side value has the right code) Carry the code alongside the message instead of replacing the value. Replacing it was the first attempt and it broke `support require in eval for a file that doesnt exist`: rebuilding from ResolveMessage's own .message drops Bun's "error: ..." prefix, which that test asserts on. The message text is now untouched — only `code` is added. The read is shared with the value path as Worker::errorCodeOf, under a top exception scope since a `code` getter can run JS. test-worker-internal-modules.mjs: 3 fail -> 3 pass, and 0/2 without the change. Vendored test-worker* 106 pass/2 fail (both pre-existing: arraybuffer-zerofill needs `bun test`, on-process-exit is debug-only). Thrown errors keep their code as before, and test-worker-error-stack-getter-throws still passes. --- src/js/node/worker_threads.ts | 4 ++ src/jsc/bindings/webcore/Worker.cpp | 48 ++++++++++++++----- src/jsc/bindings/webcore/Worker.h | 3 +- .../parallel/test-worker-internal-modules.mjs | 36 ++++++++++++++ 4 files changed, 78 insertions(+), 13 deletions(-) create mode 100644 test/js/node/test/parallel/test-worker-internal-modules.mjs diff --git a/src/js/node/worker_threads.ts b/src/js/node/worker_threads.ts index acac42253063..6e63f675cd3d 100644 --- a/src/js/node/worker_threads.ts +++ b/src/js/node/worker_threads.ts @@ -1357,7 +1357,11 @@ class Worker extends EventEmitter { // if not the message is the actual error const message = event.message; if (message !== "") { + // The value didn't clone, so rebuild from the text — but keep the `code` + // the native side carried over, which is all that survived of it. + const code = error?.code; error = new Error(message, { cause: event }); + if (typeof code === "string") error.code = code; const stack = event?.stack; if (stack) { error.stack = stack; diff --git a/src/jsc/bindings/webcore/Worker.cpp b/src/jsc/bindings/webcore/Worker.cpp index 75b93e0bb15b..6682985171ce 100644 --- a/src/jsc/bindings/webcore/Worker.cpp +++ b/src/jsc/bindings/webcore/Worker.cpp @@ -33,6 +33,7 @@ #include "Event.h" #include "EventNames.h" #include "StructuredSerializeOptions.h" +#include #include #include #include @@ -516,17 +517,45 @@ void Worker::fireEarlyMessages(Zig::GlobalObject* workerGlobalObject) } } -void Worker::dispatchErrorWithMessage(WTF::String message) +void Worker::dispatchErrorWithMessage(WTF::String message, WTF::String code) { - postTaskToParent([protectedThis = Ref { *this }, message = message.isolatedCopy()](ScriptExecutionContext&) { + postTaskToParent([protectedThis = Ref { *this }, message = message.isolatedCopy(), + code = code.isolatedCopy()](ScriptExecutionContext& context) { ErrorEvent::Init init; init.message = message; + // The worker's value would not clone (Bun's ResolveMessage is not even an + // Error), so the parent rebuilds one from the text; hand `code` over on a + // carrier object or it is lost, unlike every other coded worker error. + if (!code.isNull()) { + auto* globalObject = context.globalObject(); + auto& vm = JSC::getVM(globalObject); + if (auto* carrier = JSC::createError(globalObject, message)) { + carrier->putDirect(vm, WebCore::builtinNames(vm).codePublicName(), JSC::jsString(vm, code)); + init.error = carrier; + } + } auto event = ErrorEvent::create(eventNames().errorEvent, init, EventIsTrusted::Yes); protectedThis->dispatchEvent(event); }); } +// A string `code` off an error-ish value, or null. Reading it can run JS (a +// getter/proxy), so it is done under a top scope and any throw drops the code. +String Worker::errorCodeOf(JSC::JSGlobalObject* globalObject, JSValue value) +{ + auto& vm = JSC::getVM(globalObject); + auto scope = DECLARE_TOP_EXCEPTION_SCOPE(vm); + if (!value.isObject()) + return {}; + JSValue codeValue = value.getObject()->getIfPropertyExists(globalObject, WebCore::builtinNames(vm).codePublicName()); + String code; + if (!scope.exception() && codeValue && codeValue.isString()) + code = codeValue.toWTFString(globalObject); + CLEAR_IF_EXCEPTION(scope); + return code; +} + bool Worker::dispatchErrorWithValue(Zig::GlobalObject* workerGlobalObject, JSValue value) { // This is the top of the stack for the worker's error dispatch: both the @@ -565,13 +594,7 @@ bool Worker::dispatchErrorWithValue(Zig::GlobalObject* workerGlobalObject, JSVal // vendored node tests assert on it (e.g. ERR_TRACE_EVENTS_UNAVAILABLE). // Carry a string `code` across the thread boundary manually. If reading // `code` throws (a throwing getter/proxy), drop the code and proceed. - String errorCode; - if (value.isObject() && !scope.exception()) { - JSValue codeValue = value.getObject()->getIfPropertyExists(workerGlobalObject, WebCore::builtinNames(vm).codePublicName()); - if (!scope.exception() && codeValue && codeValue.isString()) - errorCode = codeValue.toWTFString(workerGlobalObject); - CLEAR_IF_EXCEPTION(scope); - } + String errorCode = errorCodeOf(workerGlobalObject, value); return postTaskToParent([protectedThis = Ref { *this }, serialized, errorCode = WTF::move(errorCode).isolatedCopy()](ScriptExecutionContext& context) { auto* globalObject = context.globalObject(); @@ -759,11 +782,12 @@ extern "C" void WebWorker__dispatchError(Zig::GlobalObject* globalObject, Worker globalObject->globalEventScope->dispatchEvent(ErrorEvent::create(eventNames().errorEvent, init, EventIsTrusted::Yes)); switch (worker->options().kind) { case WorkerOptions::Kind::Web: - return worker->dispatchErrorWithMessage(WTF::move(messageStr)); + return worker->dispatchErrorWithMessage(WTF::move(messageStr), {}); case WorkerOptions::Kind::Node: if (!worker->dispatchErrorWithValue(globalObject, error)) { - // If serialization threw an error, use the string instead - worker->dispatchErrorWithMessage(WTF::move(messageStr)); + // If serialization threw an error, use the string instead — but keep + // `code`, which is all the parent can otherwise recover. + worker->dispatchErrorWithMessage(WTF::move(messageStr), Worker::errorCodeOf(globalObject, error)); } return; } diff --git a/src/jsc/bindings/webcore/Worker.h b/src/jsc/bindings/webcore/Worker.h index 22b9e8ab84e8..38a1382b5aae 100644 --- a/src/jsc/bindings/webcore/Worker.h +++ b/src/jsc/bindings/webcore/Worker.h @@ -133,7 +133,8 @@ class Worker final : public ThreadSafeRefCounted, public EventTargetWith void dispatchOnlineEvent(); void dispatchOnline(Zig::GlobalObject* workerGlobalObject); void fireEarlyMessages(Zig::GlobalObject* workerGlobalObject); - void dispatchErrorWithMessage(WTF::String message); + void dispatchErrorWithMessage(WTF::String message, WTF::String code); + static WTF::String errorCodeOf(JSC::JSGlobalObject*, JSC::JSValue); bool dispatchErrorWithValue(Zig::GlobalObject* workerGlobalObject, JSValue value); bool dispatchExit(int32_t exitCode); diff --git a/test/js/node/test/parallel/test-worker-internal-modules.mjs b/test/js/node/test/parallel/test-worker-internal-modules.mjs new file mode 100644 index 000000000000..607eade85c5d --- /dev/null +++ b/test/js/node/test/parallel/test-worker-internal-modules.mjs @@ -0,0 +1,36 @@ +import '../common/index.mjs'; +import tmpdir from '../common/tmpdir.js'; +import assert from 'node:assert/strict'; +import { once } from 'node:events'; +import fs from 'node:fs/promises'; +import { describe, test, before } from 'node:test'; +import { Worker } from 'node:worker_threads'; + +const accessInternalsSource = ` +import 'node:internal/freelist'; +`; + +function convertScriptSourceToDataUrl(script) { + return new URL(`data:text/javascript,${encodeURIComponent(script)}`); +} + +describe('Worker threads should not be able to access internal modules', () => { + before(() => tmpdir.refresh()); + + test('worker instantiated with module file path', async () => { + const moduleFilepath = tmpdir.resolve('test-worker-internal-modules.mjs'); + await fs.writeFile(moduleFilepath, accessInternalsSource); + const w = new Worker(moduleFilepath); + await assert.rejects(once(w, 'exit'), { code: 'ERR_UNKNOWN_BUILTIN_MODULE' }); + }); + + test('worker instantiated with module source', async () => { + const w = new Worker(accessInternalsSource, { eval: true }); + await assert.rejects(once(w, 'exit'), { code: 'ERR_UNKNOWN_BUILTIN_MODULE' }); + }); + + test('worker instantiated with data: URL', async () => { + const w = new Worker(convertScriptSourceToDataUrl(accessInternalsSource)); + await assert.rejects(once(w, 'exit'), { code: 'ERR_UNKNOWN_BUILTIN_MODULE' }); + }); +});