Conversation
…object formatting JSC__JSValue__forEachPropertyImpl skipped a property without clearing the exception scope when getPropertySlot returned false after a static lazy property initializer threw (for example Bun.$ when the global Symbol has been replaced with a non-callable). The stale exception then failed exception scope assertions in debug and ASAN builds as soon as the next lazy property initialized, aborting the process from Bun.inspect, console.log, or an expect() failure message. Clear the exception before the continue, matching JSC__JSValue__forEachPropertyOrdered. With iteration no longer cut short, the debug-only BunSql module load diagnostic also ran with the require exception still pending and routed it through the uncaught exception machinery, which asserts on property gets and marks the process as failed. Print the error with the exception cleared and rethrow it afterwards.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesJSC exception safety
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@test/js/bun/util/inspect.test.js`:
- Around line 945-951: Update the Promise.all call in the proc test to
concurrently consume proc.stderr via proc.stderr.text() alongside stdout.text()
and proc.exited. Preserve the existing stdout and exit-code handling, and do not
add an assertion for empty stderr.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2631e766-f871-400e-8c78-addc3834d37f
📒 Files selected for processing (3)
src/jsc/bindings/BunObject.cppsrc/jsc/bindings/bindings.cpptest/js/bun/util/inspect.test.js
|
Good catch on the undrained stderr pipe: the debug build writes the bun:sql load diagnostic there, so the child could block once the buffer filled. The test now reads stderr in the same Promise.all without asserting on its contents (fe2b6d9). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Print why the internal BunSql module failed to load. The exception must be | ||
| // cleared while printing (formatting the error reads its properties, which | ||
| // must not happen with a pending exception), then rethrown. Print-only: the | ||
| // uncaught-exception machinery would also flag the process as failed. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Ignore exceptions from "Get" proxy traps and throwing lazy | ||
| // property initializers (which report the slot as not found). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
Closing as a duplicate: this fingerprint is another surface of the crash already fixed by #37107 (same Two findings from this session that may be useful to whichever PR lands:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); |
There was a problem hiding this comment.
🔴 stderr is set to "pipe" but never drained — only stdout and exited are awaited. In debug builds this test's child writes several full error dumps to stderr via reportBunSqlModuleLoadError (added in this PR), and an unread pipe can fill and deadlock the child. Include proc.stderr.text() in the Promise.all, matching the neighboring tests in this file (lines 471, 906).
Extended reasoning...
What the bug is
The new test spawns a child with stderr: "pipe" but the Promise.all at line 951 only reads proc.stdout.text() and proc.exited:
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", code],
env: bunEnv,
stdout: "pipe",
stderr: "pipe", // piped…
});
const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); // …but never drainedREVIEW.md's subprocess-test rule states this exact pattern is blocked: "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."
Why this test in particular writes to stderr
This isn't a theoretical concern for this test — it specifically exercises a stderr-writing debug path introduced by this same PR. The child replaces globalThis.Symbol and then inspects Bun twice (Bun.inspect(Bun) + console.log(Bun)). Each property walk hits the lazy sql, postgres, and SQL properties, whose initializers call internalModuleRegistry()->requireId(..., BunSql). The BunSql module calls Symbol(...) at top level, so the load throws.
In debug builds, reportBunSqlModuleLoadError (added in BunObject.cpp by this PR) then calls Bun__logUnhandledException, which routes through VirtualMachine::runErrorHandler and prints a full error dump — message, source snippet, and stack trace — to stderr for each failing property. bunEnv sets BUN_DEBUG_QUIET_LOGS=1, but that gates scoped debug loggers, not the error printer.
Step-by-step
- Child process replaces
globalThis.Symbolwith a URL instance. Bun.inspect(Bun)walks own properties → reifiesBun.sql→ BunSql module load throws"Symbol is not a function".#if BUN_DEBUG→reportBunSqlModuleLoadError→Bun__logUnhandledExceptionwrites error+stack to stderr (fd 2, which is the piped fd).- Same happens for
Bun.postgresandBun.SQL. console.log(Bun)walks the properties again → the lazy properties were never successfully reified (the exception was cleared by the fix inbindings.cpp), so steps 2–4 repeat.- Meanwhile, the parent test never reads the stderr pipe. If accumulated stderr reaches the OS pipe buffer limit (~64KB on Linux), the child's
write(2)blocks andproc.exitednever resolves — the test hangs until the runner times out instead of failing with a useful message.
Why existing code doesn't prevent it
Nothing in the test consumes stderr. await using proc will kill the child on scope exit, but scope exit only happens after the awaited Promise.all resolves — which it won't if the child is blocked writing to a full stderr pipe.
Impact and likelihood
Today the ~6 error dumps are on the order of a few KB — likely under the 64KB threshold, so an actual deadlock is not expected on current CI. However: (1) REVIEW.md flags this exact pattern as merge-blocking and states "everything here has blocked merges"; (2) the two neighboring subprocess tests in this same file (lines ~471 and ~906) already follow the correct drain-all pattern, so this diverges from local convention; (3) the volume of debug-build stderr output is not stable — any future change that makes error dumps more verbose, or adds another lazy property with a debug diagnostic, could tip this into a hang; and (4) not draining stderr discards the diagnostic output entirely, so if this test does fail in CI the most useful information is thrown away.
Fix
One line — drain stderr in the same Promise.all, matching the neighboring tests:
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);Optionally assert on it (or at minimum keep it available for the failure message).
1 participant
Fixes a debug/ASAN assertion abort found by fuzzing (fingerprint
83e57e37b0902557).Repro
The fuzzer hit this through
expect(Bun).toBeDate(), whose failure message pretty-prints the received value. Any inspect of theBunobject reaches the same code.Root cause
Bun.$is a lazy static property. Its initializer runs the shell builtin, which calls the globalSymbol(replaceable by user code), so reification can throw.setUpStaticFunctionSlotthen reports the slot as not found and leaves the exception pending for the caller.In
JSC__JSValue__forEachPropertyImpl(the property walk behindBun.inspect/console.log), the not-found case skipped the property before theCLEAR_IF_EXCEPTION:The stale exception survived into the next property's lazy initialization, and the host function return glue asserts that an exception is pending if and only if the call failed, so debug and ASAN builds abort.
JSC__JSValue__forEachPropertyOrderedalready clears before skipping; this change makes the unordered walk match it.With iteration no longer cut short, a second assert surfaced behind it: the
BUN_DEBUG-only diagnostic for a failedbun:sqlinternal module load (the sql module also callsSymbol(...)at top level) passed the still-pending exception toreportUncaughtExceptionAtEventLoop. That runs property gets with a pending exception (tripping a stale-structure assert ingetPropertySlot) and marks the process as failed, turningconsole.log(Bun)into exit code 1 in debug builds. The diagnostic now clears the exception, prints through the error printer only, and rethrows.Release builds were not crashing here (the asserts compile out), but the walk silently dropped all remaining properties once one lazy initializer threw; that is fixed too.
Test
test/js/bun/util/inspect.test.js: spawns a child that replacesglobalThis.Symboland inspectsBun, asserting clean output and exit 0. Fails with an abort on the unfixed debug build, passes with the fix.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/inspect.test.js