Skip to content

Fix debug assertion crashes when lazy Bun properties throw during property enumeration #37307

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 3 commits into from
Closed

Fix debug assertion crashes when lazy Bun properties throw during property enumeration #37307

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
285ecf6
Clear pending exceptions from failed lazy property lookups during pro…
robobun Aug 10, 2026
6109231
[autofix.ci] apply automated fixes
autofix-ci[bot] Aug 10, 2026
65020da
Tighten comment on exception clearing in forEachPropertyImpl
robobun Aug 10, 2026
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
6 changes: 0 additions & 6 deletions src/jsc/bindings/BunObject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,9 +319,6 @@ static JSValue defaultBunSQLObject(VM& vm, JSObject* bunObject)
auto scope = DECLARE_THROW_SCOPE(vm);
auto* globalObject = defaultGlobalObject(bunObject->globalObject());
JSValue sqlValue = globalObject->internalModuleRegistry()->requireId(globalObject, vm, InternalModuleRegistry::BunSql);
#if BUN_DEBUG
if (scope.exception()) globalObject->reportUncaughtExceptionAtEventLoop(globalObject, scope.exception());
#endif
RETURN_IF_EXCEPTION(scope, {});
RELEASE_AND_RETURN(scope, sqlValue.getObject()->get(globalObject, vm.propertyNames->defaultKeyword));
}
Expand All @@ -331,9 +328,6 @@ static JSValue constructBunSQLObject(VM& vm, JSObject* bunObject)
auto scope = DECLARE_THROW_SCOPE(vm);
auto* globalObject = defaultGlobalObject(bunObject->globalObject());
JSValue sqlValue = globalObject->internalModuleRegistry()->requireId(globalObject, vm, InternalModuleRegistry::BunSql);
#if BUN_DEBUG
if (scope.exception()) globalObject->reportUncaughtExceptionAtEventLoop(globalObject, scope.exception());
#endif
RETURN_IF_EXCEPTION(scope, {});
auto clientData = WebCore::clientData(vm);
RELEASE_AND_RETURN(scope, sqlValue.getObject()->get(globalObject, clientData->builtinNames().SQLPublicName()));
Expand Down
8 changes: 5 additions & 3 deletions src/jsc/bindings/bindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5596,10 +5596,12 @@ static void JSC__JSValue__forEachPropertyImpl(JSC::EncodedJSValue JSValue0, JSC:
}

JSC::PropertySlot slot(object, PropertySlot::InternalMethodType::Get);
if (!object->getPropertySlot(globalObject, property, slot))
continue;
// Ignore exceptions from "Get" proxy traps.
bool hasProperty = object->getPropertySlot(globalObject, property, slot);
// Ignore exceptions from "Get" proxy traps and static lazy
// property callbacks, which report the slot as not found.

@github-actions github-actions Bot Aug 10, 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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

CLEAR_IF_EXCEPTION(scope);
if (!hasProperty)
continue;

if ((slot.attributes() & PropertyAttribute::DontEnum) != 0) {
if (property == propertyNames->underscoreProto
Expand Down
20 changes: 20 additions & 0 deletions test/js/bun/util/inspect.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,26 @@
});
});

it("Bun.inspect(Bun) does not crash when the Symbol global is clobbered", async () => {
// Lazy properties on the Bun object (Bun.$, Bun.sql) evaluate internal
// modules that call Symbol() at the top level, so reading them throws when
// the global Symbol is overwritten. The pending exception must not leak into
// the lookup of the next property during the inspect walk.
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`globalThis.Symbol = NaN; Bun.inspect(Bun); try { Bun.sql } catch {} console.log("ok");`,

@coderabbitai coderabbitai Bot Aug 10, 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.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert that Bun.sql triggers the intended lazy-property failure.

Line 591 discards the catch result. If Bun.sql does not throw after Symbol is clobbered, the child still prints "ok" and exits successfully. The test can then pass without exercising the lazy callback that supplies the pending exception.

Track whether the access throws and fail before printing "ok" when it does not.

As per coding guidelines, tests must prove their preconditions and must not silently swallow failures.

Proposed fix 
-      `globalThis.Symbol = NaN; Bun.inspect(Bun); try { Bun.sql } catch {} console.log("ok");`,
+      `globalThis.Symbol = NaN; Bun.inspect(Bun); let sqlThrew = false; try { Bun.sql } catch { sqlThrew = true; } if (!sqlThrew) throw new Error("Bun.sql did not throw"); console.log("ok");`,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
`globalThis.Symbol = NaN; Bun.inspect(Bun); try { Bun.sql } catch {} console.log("ok");`,
`globalThis.Symbol = NaN; Bun.inspect(Bun); let sqlThrew = false; try { Bun.sql } catch { sqlThrew = true; } if (!sqlThrew) throw new Error("Bun.sql did not throw"); console.log("ok");`,
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/js/bun/util/inspect.test.js` at line 591, Update the test command around
Bun.sql so it records whether accessing Bun.sql throws after globalThis.Symbol
is set to NaN, and exits or asserts failure before printing "ok" when no
exception occurs. Preserve the success output only when the intended
lazy-property failure is observed, and avoid silently swallowing the catch
result.

Source: Coding guidelines

],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);

Check warning on line 597 in test/js/bun/util/inspect.test.js

View check run for this annotation

Claude / Claude Code Review

Test pipes stderr but never drains it 

The test sets `stderr: "pipe"` but never drains it — `Promise.all` only reads stdout and awaits `proc.exited`. Per REVIEW.md's "Subprocess tests: drain pipes concurrently" rule (and the immediately-preceding test at line 573), this should be `const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited])` followed by `expect({ stdout, stderr, exitCode }).toEqual({ stdout: "ok\n", stderr: "", exitCode: 0 })`.
Comment on lines +587 to +597

@coderabbitai coderabbitai Bot Aug 10, 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.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Drain proc.stderr before waiting for process exit.

Line 595 configures stderr as a pipe, but Line 597 reads only stdout and proc.exited. If the child reproduces the debug or ASAN failure and writes enough diagnostics, the stderr pipe can fill and block the child. The test can then hang instead of reporting the failure.

Read proc.stderr.text() in the same Promise.all as stdout and proc.exited.

As per coding guidelines, subprocess tests must drain stdout, stderr, and process exit concurrently.

Proposed fix 
-  const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);
+  const [stdout, , exitCode] = await Promise.all([
+    proc.stdout.text(),
+    proc.stderr.text(),
+    proc.exited,
+  ]);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`globalThis.Symbol = NaN; Bun.inspect(Bun); try { Bun.sql } catch {} console.log("ok");`,
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`globalThis.Symbol = NaN; Bun.inspect(Bun); try { Bun.sql } catch {} console.log("ok");`,
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, , exitCode] = await Promise.all([
proc.stdout.text(),
proc.stderr.text(),
proc.exited,
]);
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/js/bun/util/inspect.test.js` around lines 587 - 597, Update the
subprocess test around Bun.spawn and its Promise.all to concurrently consume
proc.stderr.text() alongside proc.stdout.text() and proc.exited, preserving the
existing stdout and exit-code handling while ensuring the configured stderr pipe
is drained.

Source: Coding guidelines

Comment on lines +594 to +597

@claude claude Bot Aug 10, 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.

🟡 The test sets stderr: "pipe" but never drains it — Promise.all only reads stdout and awaits proc.exited. Per REVIEW.md's "Subprocess tests: drain pipes concurrently" rule (and the immediately-preceding test at line 573), this should be const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]) followed by expect({ stdout, stderr, exitCode }).toEqual({ stdout: "ok\n", stderr: "", exitCode: 0 }).

Extended reasoning...

What the issue is

The new test at inspect.test.js:582-600 spawns a subprocess with stderr: "pipe" (line 595), but the Promise.all on line 597 only awaits proc.stdout.text() and proc.exited — proc.stderr is never read. This violates the repository's explicit harness convention documented in REVIEW.md under Subprocess tests: drain pipes concurrently:

Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]) — an unread pipe fills the ~64KB OS buffer and deadlocks the child. assert a combined { stdout, stderr, exitCode } object.

It also diverges from the immediately-preceding test in the same file ("Bun.inspect huge sparse array…", lines 571-579), which follows this pattern exactly and asserts on a combined { stdout, stderr, exitCode } object — so this also runs against Copy harness conventions exactly.

Step-by-step walk-through

  1. Line 587-596: Bun.spawn is called with stdout: "pipe" and stderr: "pipe", so both streams are backed by OS pipes that must be drained by the parent.
  2. Line 597: const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); — stdout is drained, but nothing ever calls proc.stderr.text().
  3. Lines 598-599 assert only on stdout and exitCode; stderr is silently discarded.

If the child wrote ≥64KB to stderr, its write(2) would block on the full pipe buffer and the child would never exit, so proc.exited would never resolve and the test would hang until the runner's timeout. In this specific case the child script (globalThis.Symbol = NaN; Bun.inspect(Bun); try { Bun.sql } catch {} console.log("ok");) writes very little (or nothing) to stderr on the success path, so a deadlock is unlikely in practice.

Why this still matters

Beyond the (small) deadlock risk, dropping the stderr assertion loses diagnostic value for exactly the failure mode this test guards against. The bug being fixed is a debug-build assertion abort, and JSC's releaseAssertNoException failure message goes to stderr. If the fix regresses, the test will fail with expect(exitCode).toBe(0) but the assertion output on stderr — the thing that actually explains the failure — is thrown away. Asserting stderr: "" also catches the case where the child prints a warning or partial crash dump but still exits 0.

Why existing code doesn't prevent it

Nothing in the test harness auto-drains piped streams; if you request stderr: "pipe", you own reading it. The await using proc disposal will close the parent's read end after the test body completes, but that happens after await proc.exited, so it can't unblock a child that's stuck writing.

Suggested fix

Match the preceding test's shape:

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stdout, stderr, exitCode }).toEqual({ stdout: "ok\n", stderr: "", exitCode: 0 });

Alternatively, if stderr is intentionally uninteresting, drop stderr: "pipe" from the spawn options — but given that the failure mode under test writes its abort message to stderr, draining and asserting on it is the better choice.

expect(stdout).toBe("ok\n");
expect(exitCode).toBe(0);
});

describe("console.logging function displays async and generator names", async () => {
const cases = [
function () {},
Expand Down
Loading