Skip to content

process: don't hand an empty JSValue to reifyStaticProperty on builder exception #34879

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 1 commit into from
Closed

process: don't hand an empty JSValue to reifyStaticProperty on builder exception #34879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
92 changes: 41 additions & 51 deletions src/jsc/bindings/BunProcess.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,12 +207,24 @@
#endif
}

// A LazyPropertyCallback's result is stored via putDirect with no exception
// check (see reifyStaticProperty), so the builder must never return the empty
// JSValue and must not let an exception escape.
static JSValue clearAndReportLazyPropertyException(JSC::TopExceptionScope& scope, JSC::JSGlobalObject* globalObject)
{
auto* exception = scope.exception();
(void)scope.tryClearException();
Zig::GlobalObject::reportUncaughtExceptionAtEventLoop(globalObject, exception);
return JSC::jsUndefined();
}

static JSValue constructVersions(VM& vm, JSObject* processObject)
{
auto scope = DECLARE_TOP_EXCEPTION_SCOPE(vm);
auto* globalObject = processObject->globalObject();
JSC::JSObject* object = JSC::constructEmptyObject(globalObject, globalObject->objectPrototype(), 24);
RETURN_IF_EXCEPTION(scope, {});
if (scope.exception()) [[unlikely]]
return clearAndReportLazyPropertyException(scope, globalObject);

object->putDirect(vm, JSC::Identifier::fromString(vm, "node"_s), JSC::jsOwnedString(vm, makeAtomString(ASCIILiteral::fromLiteralUnsafe(REPORTED_NODEJS_VERSION))));
object->putDirect(vm, JSC::Identifier::fromString(vm, "bun"_s), JSC::jsOwnedString(vm, String(ASCIILiteral::fromLiteralUnsafe(Bun__version)).substring(1)));
Expand Down Expand Up @@ -277,7 +289,8 @@
release->putDirect(vm, Identifier::fromString(vm, "sourceUrl"_s), jsOwnedString(vm, WTF::String(std::span { Bun__githubURL, strlen(Bun__githubURL) })), 0);
release->putDirect(vm, Identifier::fromString(vm, "headersUrl"_s), jsOwnedString(vm, String("https://nodejs.org/download/release/v" REPORTED_NODEJS_VERSION "/node-v" REPORTED_NODEJS_VERSION "-headers.tar.gz"_s)), 0);

RETURN_IF_EXCEPTION(scope, {});
if (scope.exception()) [[unlikely]]
return clearAndReportLazyPropertyException(scope, globalObject);
return release;
}

Expand Down Expand Up @@ -2512,10 +2525,11 @@
report->putDirect(vm, JSC::Identifier::fromString(vm, "reportOnFatalError"_s), JSC::jsBoolean(false), 0);
report->putDirect(vm, JSC::Identifier::fromString(vm, "reportOnSignal"_s), JSC::jsBoolean(false), 0);
report->putDirect(vm, JSC::Identifier::fromString(vm, "reportOnUncaughtException"_s), JSC::jsBoolean(process->m_reportOnUncaughtException), 0);
report->putDirect(vm, JSC::Identifier::fromString(vm, "excludeEnv"_s), JSC::jsBoolean(false), 0);
report->putDirect(vm, JSC::Identifier::fromString(vm, "excludeEnv"_s), JSC::jsString(vm, String("SIGUSR2"_s)), 0);

Check notice on line 2529 in src/jsc/bindings/BunProcess.cpp

View check run for this annotation

Claude / Claude Code Review

Pre-existing: process.report.signal missing (duplicate excludeEnv key) 

Pre-existing (not introduced by this PR), but visible in the touched hunk: lines 2528-2529 both `putDirect` the key `"excludeEnv"` — the second, whose value is `"SIGUSR2"`, was almost certainly meant to be `"signal"`. As-is, `process.report.signal` is `undefined` and `process.report.excludeEnv` is the string `"SIGUSR2"` instead of boolean `false`. Worth fixing while you're in this function, but shouldn't block the PR.
Comment on lines 2528 to 2529

@claude claude Bot Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 Pre-existing (not introduced by this PR), but visible in the touched hunk: lines 2528-2529 both putDirect the key "excludeEnv" — the second, whose value is "SIGUSR2", was almost certainly meant to be "signal". As-is, process.report.signal is undefined and process.report.excludeEnv is the string "SIGUSR2" instead of boolean false. Worth fixing while you're in this function, but shouldn't block the PR.

Extended reasoning...

What the bug is

In constructProcessReportObject (BunProcess.cpp:2528-2529), two consecutive putDirect calls use the identical key "excludeEnv":

report->putDirect(vm, JSC::Identifier::fromString(vm, "excludeEnv"_s), JSC::jsBoolean(false), 0);
report->putDirect(vm, JSC::Identifier::fromString(vm, "excludeEnv"_s), JSC::jsString(vm, String("SIGUSR2"_s)), 0);

The second call overwrites the first. Node's process.report exposes a signal property that defaults to 'SIGUSR2' and an excludeEnv boolean that defaults to false, so the second line's key was clearly meant to be "signal" — this is a copy-paste slip.

Step-by-step

  1. User evaluates process.report → the lazy PropertyCallback constructProcessReportObject runs.
  2. Line 2528 sets report.excludeEnv = false.
  3. Line 2529 sets report.excludeEnv = "SIGUSR2", overwriting step 2.
  4. No line ever sets report.signal.
  5. Observable result: process.report.excludeEnv === "SIGUSR2" (string, wrong type) and process.report.signal === undefined (missing).

Why nothing prevents it

putDirect on an existing own property silently replaces the slot; it doesn't warn or throw on duplicate keys. There is no test asserting the type of process.report.excludeEnv or the presence of process.report.signal — the existing process.report test just calls JSON.stringify(process.report.getReport()), and this PR's new test only checks typeof process.report === "object".

Impact

Node-compat divergence on process.report: code that reads process.report.signal (e.g., to know which signal triggers report generation) gets undefined, and code that branches on the boolean process.report.excludeEnv sees a truthy string instead of false. Low practical impact since process.report is largely a stub in Bun, but it's a straightforward correctness bug.

Fix

Change the second key to "signal":

report->putDirect(vm, JSC::Identifier::fromString(vm, "excludeEnv"_s), JSC::jsBoolean(false), 0);
report->putDirect(vm, JSC::Identifier::fromString(vm, "signal"_s), JSC::jsString(vm, String("SIGUSR2"_s)), 0);

Relation to this PR

These lines are unchanged context in the diff — git blame attributes them to a commit predating this PR. This PR only replaced the RETURN_IF_EXCEPTION(scope, {}) on the line immediately below with the new clearAndReportLazyPropertyException helper. The bug is unrelated to the exception-handling change; it's flagged only because it sits inside the touched hunk and is trivial to fix while here.

report->putDirect(vm, JSC::Identifier::fromString(vm, "writeReport"_s), JSC::JSFunction::create(vm, globalObject, 1, String("writeReport"_s), Process_functionWriteReport, ImplementationVisibility::Public), 0);
RETURN_IF_EXCEPTION(scope, {});
if (scope.exception()) [[unlikely]]
return clearAndReportLazyPropertyException(scope, globalObject);
return report;
}

Expand Down Expand Up @@ -2551,11 +2565,8 @@
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);
return JSC::jsUndefined();
}
if (scope.exception()) [[unlikely]]
return clearAndReportLazyPropertyException(scope, globalObject);
variables->putDirect(vm, JSC::Identifier::fromString(vm, "v8_enable_i8n_support"_s), JSC::jsNumber(1), 0);
variables->putDirect(vm, JSC::Identifier::fromString(vm, "enable_lto"_s), JSC::jsBoolean(false), 0);
// Node 26's common.gypi evaluates enable_thin_lto/lto_jobs conditions; gyp
Expand Down Expand Up @@ -2649,7 +2660,8 @@
#endif

config->freeze(vm);
RETURN_IF_EXCEPTION(scope, {});
if (scope.exception()) [[unlikely]]
return clearAndReportLazyPropertyException(scope, globalObject);
return config;
}

Expand Down Expand Up @@ -2688,11 +2700,8 @@
JSC::CallData callData = JSC::getCallData(getStdioWriteStream);

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);
return jsUndefined();
}
if (scope.exception()) [[unlikely]]
return clearAndReportLazyPropertyException(scope, globalObject);

ASSERT_WITH_MESSAGE(JSC::isJSArray(result), "Expected an array from getStdioWriteStream");
JSC::JSArray* resultObject = uncheckedDowncast<JSC::JSArray>(result);
Expand Down Expand Up @@ -2749,11 +2758,8 @@
JSC::CallData callData = JSC::getCallData(getStdinStream);

auto result = JSC::profiledCall(globalObject, ProfilingReason::API, getStdinStream, callData, globalObject, args);
if (auto* exception = scope.exception()) {
(void)scope.tryClearException();
Zig::GlobalObject::reportUncaughtExceptionAtEventLoop(globalObject, exception);
return jsUndefined();
}
if (scope.exception()) [[unlikely]]
return clearAndReportLazyPropertyException(scope, globalObject);
return result;
}

Expand Down Expand Up @@ -2814,11 +2820,8 @@
JSC::CallData callData = JSC::getCallData(getControl);

auto result = JSC::profiledCall(globalObject, ProfilingReason::API, getControl, callData, globalObject->globalThis(), args);
if (auto* exception = scope.exception()) [[unlikely]] {
(void)scope.tryClearException();
Zig::GlobalObject::reportUncaughtExceptionAtEventLoop(globalObject, exception);
return jsUndefined();
}
if (scope.exception()) [[unlikely]]
return clearAndReportLazyPropertyException(scope, globalObject);
return result;
} else {
return jsUndefined();
Expand Down Expand Up @@ -3016,11 +3019,8 @@
// reifyStaticProperty, which performs no exception check.
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);
return JSC::jsUndefined();
}
if (scope.exception()) [[unlikely]]
return clearAndReportLazyPropertyException(scope, globalObject);
return env;
}

Expand Down Expand Up @@ -3885,11 +3885,8 @@
// reifyStaticProperty, which performs no exception check.
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);
return JSC::jsUndefined();
}
if (scope.exception()) [[unlikely]]
return clearAndReportLazyPropertyException(scope, processObject->globalObject());
return array;
}

Expand All @@ -3898,7 +3895,8 @@
auto* globalObject = processObject->globalObject();
auto scope = DECLARE_TOP_EXCEPTION_SCOPE(vm);
JSSet* result = JSSet::create(vm, globalObject->setStructure());
RETURN_IF_EXCEPTION(scope, {});
if (scope.exception()) [[unlikely]]
return clearAndReportLazyPropertyException(scope, globalObject);
return result;
}

Expand Down Expand Up @@ -4023,18 +4021,12 @@
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);
return JSC::jsUndefined();
}
if (scope.exception()) [[unlikely]]
return clearAndReportLazyPropertyException(scope, globalObject);
auto* requireMap = globalObject->requireMap();
JSValue mainModule = requireMap->get(globalObject, mainValue);
if (auto* exception = scope.exception()) [[unlikely]] {
(void)scope.tryClearException();
Zig::GlobalObject::reportUncaughtExceptionAtEventLoop(globalObject, exception);
return JSC::jsUndefined();
}
if (scope.exception()) [[unlikely]]
return clearAndReportLazyPropertyException(scope, globalObject);
return mainModule;
}

Expand All @@ -4060,11 +4052,8 @@
// reifyStaticProperty, which performs no exception check.
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);
return JSC::jsUndefined();
}
if (scope.exception()) [[unlikely]]
return clearAndReportLazyPropertyException(scope, globalObject);
if (nextTickFunction && nextTickFunction.isObject()) {
this->m_nextTickFunction.set(vm, this, nextTickFunction.getObject());
}
Expand Down Expand Up @@ -4126,7 +4115,8 @@
object->putDirect(vm, Identifier::fromString(vm, "require_module"_s), jsBoolean(true));
object->putDirect(vm, Identifier::fromString(vm, "typescript"_s), jsString(vm, String("transform"_s)));

RETURN_IF_EXCEPTION(scope, {});
if (scope.exception()) [[unlikely]]
return clearAndReportLazyPropertyException(scope, globalObject);
return object;
}

Expand Down
50 changes: 50 additions & 0 deletions test/js/node/process/process.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1596,6 +1596,56 @@ it.each(["stdin", "stdout", "stderr"])("%s stream accessor should handle excepti
);
});

// JSC's reifyStaticProperty stores a PropertyCallback's return value via
// putDirect with no exception check, so a builder must never return the empty
// JSValue (putDirectInternal asserts on it) or let an exception escape. The
// allocation paths in these builders only fail on OOM, so this is a scope-
// discipline guard rather than a deterministic crash repro.
it("lazy process properties reify under JSC exception-scope validation", async () => {
const properties = [
"versions",
"release",
"report",
"config",
"allowedNodeEnvironmentFlags",
"features",
"_preload_modules",
"env",
"mainModule",
];
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
const names = ${JSON.stringify(properties)};
const types = {};
for (const name of names) types[name] = typeof process[name];
void process.report.getReport();
process.stdout.write(JSON.stringify(types));
`,
],
env: { ...bunEnv, BUN_JSC_validateExceptionChecks: "1" },
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stderr, types: JSON.parse(stdout || "null"), exitCode }).toEqual({
stderr: "",
types: {
versions: "object",
release: "object",
report: "object",
config: "object",
allowedNodeEnvironmentFlags: "object",
features: "object",
_preload_modules: "object",
env: "object",
mainModule: "undefined",
},
exitCode: 0,
});
});

it("process.versions", () => {
expect(process.versions.node).toEqual("26.3.0");
expect(process.versions.v8).toEqual("14.6.202.34-node.20");
Expand Down
Loading