Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
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
9 changes: 8 additions & 1 deletion src/jsc/bindings/BunObject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,14 @@

static JSValue constructEnvObject(VM& vm, JSObject* object)
{
return uncheckedDowncast<Zig::GlobalObject>(object->globalObject())->processEnvObject();
auto scope = DECLARE_THROW_SCOPE(vm);
// On Windows the env map is built by a JS builtin that can throw when user
// code clobbered a global it depends on. Return empty so reifyStaticProperty
// doesn't putDirect a value while the exception is pending (the lookup would
// report not-found after the structure already transitioned).
Comment thread
robobun marked this conversation as resolved.
Outdated
JSObject* env = uncheckedDowncast<Zig::GlobalObject>(object->globalObject())->processEnvObject();
RETURN_IF_EXCEPTION(scope, {});
return env;

Check warning on line 117 in src/jsc/bindings/BunObject.cpp

View check run for this annotation

Claude / Claude Code Review

Bun.sql/Bun.SQL PropertyCallbacks still report synchronously mid-walk (debug-only)

Two more same-file PropertyCallback siblings still call `reportUncaughtExceptionAtEventLoop` synchronously from inside the property-lookup walk: `defaultBunSQLObject` (Bun.sql / Bun.postgres) and `constructBunSQLObject` (Bun.SQL), each behind `#if BUN_DEBUG` — and unlike the sites this PR fixes, they report *without clearing* the exception first. Debug-only and pre-existing, but per REVIEW.md's "fix the whole class in the same PR" (and after `constructEnvObject` was swept here for the same reaso
Comment thread
robobun marked this conversation as resolved.
}

JSC::EncodedJSValue flattenArrayOfBuffersIntoArrayBufferOrUint8Array(JSGlobalObject* lexicalGlobalObject, JSValue arrayValue, size_t maxLength, bool asUint8Array)
Expand Down
61 changes: 40 additions & 21 deletions src/jsc/bindings/BunProcess.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "bun_dependency_versions.h"
#include <JavaScriptCore/InternalFieldTuple.h>
#include <JavaScriptCore/JSMicrotask.h>
#include <JavaScriptCore/MicrotaskQueue.h>
#include <JavaScriptCore/ObjectConstructor.h>
#include <JavaScriptCore/NumberPrototype.h>
#include "JSCommonJSModule.h"
Expand Down Expand Up @@ -1777,17 +1778,35 @@
return JSC::JSFunction::create(vm, globalObject, processObjectInternalsLoadEnvFileCodeGenerator(vm), globalObject);
}

JSC_DECLARE_HOST_FUNCTION(jsFunctionReportUncaughtException);

Check warning on line 1781 in src/jsc/bindings/BunProcess.cpp

View check run for this annotation

Claude / Claude Code Review

Redundant forward declaration of jsFunctionReportUncaughtException

This forward declaration is redundant — `jsFunctionReportUncaughtException` is already declared in `BunProcess.h:148` (inside `namespace Bun`), which this file includes, and the pre-existing use at line 4448 already relies on that header declaration. The line can just be dropped.
Comment thread
robobun marked this conversation as resolved.
Outdated

// Lazy PropertyCallback builders run while setUpStaticFunctionSlot /
// reifyAllStaticProperties is reifying the property, i.e. in the middle of a
// property lookup whose walk (JSObject::getPropertySlot) holds the object's
// Structure*. Running the uncaught-exception machinery there re-enters JS,
// which reifies further static properties and transitions structures under
// the walk, tripping the stale-Structure assert in Structure::storedPrototype.
// Queue the report as a microtask so it runs after the lookup completes.
Comment thread
robobun marked this conversation as resolved.
Outdated
static void reportLazyPropertyBuilderException(JSC::JSGlobalObject* globalObject, JSC::Exception* exception)
{
auto& vm = JSC::getVM(globalObject);
auto* report = JSC::JSFunction::create(vm, globalObject, 1, String(), jsFunctionReportUncaughtException, JSC::ImplementationVisibility::Private);
JSC::QueuedTask task { nullptr, JSC::InternalMicrotask::BunInvokeJobWithArguments, 0, globalObject, report, JSC::JSValue(exception) };
vm.queueMicrotask(WTF::move(task));
}

// Lazy PropertyCallback builders that enter JS. reifyAllStaticProperties wraps these in
// DeferTerminationForAWhile; a non-termination throw is cleared+reported so the worker's
// reifyAllStaticProperties (node:worker_threads preload) doesn't leave a pending exception.
// DeferTerminationForAWhile; a non-termination throw is cleared (and reported through
// reportLazyPropertyBuilderException) so the worker's reifyAllStaticProperties
// (node:worker_threads preload) doesn't leave a pending exception.
Comment thread
robobun marked this conversation as resolved.
static JSValue callLazyProcessBuilder(VM& vm, JSC::JSGlobalObject* globalObject, JSC::FunctionExecutable* (*generator)(VM&), const JSC::ArgList& args)
{
auto scope = DECLARE_TOP_EXCEPTION_SCOPE(vm);
auto* function = JSC::JSFunction::create(vm, globalObject, generator(vm), globalObject);
auto result = JSC::profiledCall(globalObject, ProfilingReason::API, function, JSC::getCallData(function), globalObject->globalThis(), args);
if (auto* exception = scope.exception()) [[unlikely]] {
(void)scope.tryClearException();
Zig::GlobalObject::reportUncaughtExceptionAtEventLoop(globalObject, exception);
reportLazyPropertyBuilderException(globalObject, exception);
return jsUndefined();
}
return result;
Expand Down Expand Up @@ -2755,15 +2774,15 @@
// v8_use_snapshot: 1
// }
// }
// Lazy property builder: exceptions must not propagate into
// reifyStaticProperty, which performs no exception check.
// Lazy property builder: a throw is cleared, not propagated
// (see callLazyProcessBuilder).
Comment thread
robobun marked this conversation as resolved.
auto scope = DECLARE_TOP_EXCEPTION_SCOPE(vm);
JSC::JSObject* config = JSC::constructEmptyObject(globalObject, globalObject->objectPrototype(), 2);
JSC::JSObject* variables = JSC::constructEmptyObject(globalObject, globalObject->objectPrototype(), 2);
JSC::JSArray* shareableBuiltins = JSC::constructEmptyArray(globalObject, nullptr);
if (auto* exception = scope.exception()) [[unlikely]] {
(void)scope.tryClearException();
Zig::GlobalObject::reportUncaughtExceptionAtEventLoop(globalObject, exception);
reportLazyPropertyBuilderException(globalObject, exception);
return JSC::jsUndefined();
}
variables->putDirect(vm, JSC::Identifier::fromString(vm, "v8_enable_i18n_support"_s), JSC::jsNumber(1), 0);
Expand Down Expand Up @@ -2904,7 +2923,7 @@
auto result = JSC::profiledCall(globalObject, ProfilingReason::API, getStdioWriteStream, callData, globalObject->globalThis(), args);
if (auto* exception = scope.exception()) {
(void)scope.tryClearException();
Zig::GlobalObject::reportUncaughtExceptionAtEventLoop(globalObject, exception);
reportLazyPropertyBuilderException(globalObject, exception);
return jsUndefined();
}

Expand Down Expand Up @@ -2965,7 +2984,7 @@
auto result = JSC::profiledCall(globalObject, ProfilingReason::API, getStdinStream, callData, globalObject, args);
if (auto* exception = scope.exception()) {
(void)scope.tryClearException();
Zig::GlobalObject::reportUncaughtExceptionAtEventLoop(globalObject, exception);
reportLazyPropertyBuilderException(globalObject, exception);
return jsUndefined();
}
return result;
Expand Down Expand Up @@ -3200,13 +3219,13 @@
static JSValue constructEnv(VM& vm, JSObject* processObject)
{
auto* globalObject = uncheckedDowncast<Zig::GlobalObject>(processObject->globalObject());
// Lazy property builder: exceptions must not propagate into
// reifyStaticProperty, which performs no exception check.
// Lazy property builder: a throw is cleared, not propagated
// (see callLazyProcessBuilder).
Comment thread
robobun marked this conversation as resolved.
auto scope = DECLARE_TOP_EXCEPTION_SCOPE(vm);
JSValue env = globalObject->processEnvObject();
if (auto* exception = scope.exception()) [[unlikely]] {
(void)scope.tryClearException();
Zig::GlobalObject::reportUncaughtExceptionAtEventLoop(globalObject, exception);
reportLazyPropertyBuilderException(globalObject, exception);
return JSC::jsUndefined();
}
return env;
Expand Down Expand Up @@ -4253,13 +4272,13 @@

static JSValue Process_stubEmptyArray(VM& vm, JSObject* processObject)
{
// Lazy property builder: exceptions must not propagate into
// reifyStaticProperty, which performs no exception check.
// Lazy property builder: a throw is cleared, not propagated
// (see callLazyProcessBuilder).
Comment thread
robobun marked this conversation as resolved.
auto scope = DECLARE_TOP_EXCEPTION_SCOPE(vm);
JSC::JSArray* array = JSC::constructEmptyArray(processObject->globalObject(), nullptr);
if (auto* exception = scope.exception()) [[unlikely]] {
(void)scope.tryClearException();
Zig::GlobalObject::reportUncaughtExceptionAtEventLoop(processObject->globalObject(), exception);
reportLazyPropertyBuilderException(processObject->globalObject(), exception);
return JSC::jsUndefined();
}
return array;
Expand Down Expand Up @@ -4388,23 +4407,23 @@
// return require.cache.get(Bun.main)
static JSValue constructMainModuleProperty(VM& vm, JSObject* processObject)
{
// Lazy property builder: exceptions must not propagate into
// reifyStaticProperty, which performs no exception check.
// Lazy property builder: a throw is cleared, not propagated
// (see callLazyProcessBuilder).
Comment thread
robobun marked this conversation as resolved.
auto scope = DECLARE_TOP_EXCEPTION_SCOPE(vm);
auto* globalObject = defaultGlobalObject(processObject->globalObject());
auto* bun = globalObject->bunObject();
auto& builtinNames = Bun::builtinNames(vm);
JSValue mainValue = bun->get(globalObject, builtinNames.mainPublicName());
if (auto* exception = scope.exception()) [[unlikely]] {
(void)scope.tryClearException();
Zig::GlobalObject::reportUncaughtExceptionAtEventLoop(globalObject, exception);
reportLazyPropertyBuilderException(globalObject, exception);
return JSC::jsUndefined();
}
auto* requireMap = globalObject->requireMap();
JSValue mainModule = requireMap->get(globalObject, mainValue);
if (auto* exception = scope.exception()) [[unlikely]] {
(void)scope.tryClearException();
Zig::GlobalObject::reportUncaughtExceptionAtEventLoop(globalObject, exception);
reportLazyPropertyBuilderException(globalObject, exception);
return JSC::jsUndefined();
}
return mainModule;
Expand All @@ -4428,13 +4447,13 @@
args.append(JSC::JSFunction::create(vm, globalObject, 1, String(), jsFunctionDrainMicrotaskQueue, ImplementationVisibility::Private));
args.append(JSC::JSFunction::create(vm, globalObject, 1, String(), jsFunctionReportUncaughtException, ImplementationVisibility::Private));

// Lazy property builder: exceptions must not propagate into
// reifyStaticProperty, which performs no exception check.
// Lazy property builder: a throw is cleared, not propagated
// (see callLazyProcessBuilder).
Comment thread
robobun marked this conversation as resolved.
auto scope = DECLARE_TOP_EXCEPTION_SCOPE(vm);
JSValue nextTickFunction = JSC::profiledCall(globalObject, ProfilingReason::API, initializer, JSC::getCallData(initializer), globalObject->globalThis(), args);
if (auto* exception = scope.exception()) [[unlikely]] {
(void)scope.tryClearException();
Zig::GlobalObject::reportUncaughtExceptionAtEventLoop(globalObject, exception);
reportLazyPropertyBuilderException(globalObject, exception);
return JSC::jsUndefined();
}
if (nextTickFunction && nextTickFunction.isObject()) {
Expand Down
12 changes: 11 additions & 1 deletion src/jsc/bindings/ZigGlobalObject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2543,7 +2543,17 @@ void GlobalObject::finishCreation(VM& vm)

m_processEnvObject.initLater(
[](const JSC::LazyProperty<JSC::JSGlobalObject, JSC::JSObject>::Initializer& init) {
init.set(Bun::createEnvironmentVariablesMap(static_cast<Zig::GlobalObject*>(init.owner)).getObject());
auto scope = DECLARE_THROW_SCOPE(init.vm);
JSValue map = Bun::createEnvironmentVariablesMap(static_cast<Zig::GlobalObject*>(init.owner));
// On Windows the map is built by the windowsEnv builtin, which can
// throw when user code clobbered a global it depends on. A
// LazyProperty initializer must still set a value; leave the
// exception pending for the caller.
Comment thread
robobun marked this conversation as resolved.
Outdated
if (scope.exception() || !map || !map.isObject()) [[unlikely]] {
init.set(JSC::constructEmptyObject(init.owner));
return;
Comment thread
claude[bot] marked this conversation as resolved.
}
init.set(map.getObject());
});

m_processObject.initLater(
Expand Down
102 changes: 102 additions & 0 deletions test/js/node/process/process.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2384,3 +2384,105 @@ it("no socket close handler runs after the 'exit' event", async () => {
expect(stdout).toBe("exit\n");
expect(exitCode).toBe(0);
});

describe.concurrent("lazy property builders", () => {
// Lazy process properties are reified in the middle of a property lookup.
// When the builder throws (user code clobbered a global it depends on), the
// error must not be reported from inside the lookup: the uncaughtException
// machinery runs arbitrary JS, which reifies more static properties and
// transitions object structures under the in-progress prototype-chain walk
// (stale-Structure assert in debug builds). The report is deferred to a
// microtask, so the handler observes it after the statement completes.
it("defers a builder failure report until after the property lookup", async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`globalThis.Set = 123;
const order = [];
process.on("uncaughtException", e => order.push("uncaught:" + e.constructor.name));
order.push("before");
order.push("value:" + String(process.allowedNodeEnvironmentFlags));
order.push("after");
process.on("exit", () => console.log(order.join(",")));`,
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stdout).toBe("before,value:undefined,after,uncaught:TypeError\n");
expect(stderr).toBe("");
expect(exitCode).toBe(0);
});

// On Windows process.env is built by a JS builtin, so a clobbered global can
// fail it while another lazy property (Bun.$ evaluates shell.ts, which reads
// process.env) is being reified. The failed builder must leave a usable
// process.env behind, and the deferred report keeps the uncaughtException
// handler (which reifies more Bun properties here) from running while the
// Bun.$ prototype-chain walk is still on the stack.
it("survives a clobbered global breaking the env builder mid-walk", async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`Bun.inspect;
process.on("uncaughtException", e => {
Bun.gc;
Bun.SHA1;
console.log("uncaught:" + e.constructor.name);
});
globalThis.Proxy = 123;
globalThis.Symbol = 123;
try {
Bun.$;
} catch (e) {
console.log("caught:" + e.constructor.name);
}
console.log("alive");`,
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
const expected = isWindows ? "caught:TypeError\nalive\nuncaught:TypeError\n" : "caught:TypeError\nalive\n";
expect(stdout).toBe(expected);
expect(stderr).toBe("");
expect(exitCode).toBe(0);
});

// Bun.env shares the lazy env map. When its construction fails, the builder
// must return empty with the exception pending (like Bun.$) instead of
// handing reifyStaticProperty a value: putDirect would transition the Bun
// object's structure while the failed lookup is reported as not-found, and
// the in-progress walk would advance through a stale Structure*.
// Bun.inspect is pre-reified because the windowsEnv builtin reads
// Bun.inspect.custom while building the map, which transitions the Bun
// object mid-walk on its own; that separate bug is fixed in #37175.
it("Bun.env access with a broken env builder throws instead of crashing", async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`Bun.inspect;
globalThis.Proxy = 123;
try {
console.log("typeof:" + typeof Bun.env);
} catch (e) {
console.log("caught:" + e.constructor.name);
}
console.log("alive");`,
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
const expected = isWindows ? "caught:TypeError\nalive\n" : "typeof:object\nalive\n";
expect(stdout).toBe(expected);
expect(stderr).toBe("");
Comment thread
coderabbitai[bot] marked this conversation as resolved.
expect(exitCode).toBe(0);
});
});
Loading