Handle exception from getPrototype in forEachProperty - #28918
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSuppress exceptions thrown by Proxy Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted null-deref fix with a matching regression test.
Extended reasoning...
Overview
Two files changed: a 5-line fix in bindings.cpp and a new regression test in expect-proxy-prototype-crash.test.ts. The fix is isolated to the slow path of JSC__JSValue__forEachPropertyImpl.
Security risks
None. This is a crash fix that prevents a null dereference; it does not touch auth, crypto, or permission code.
Level of scrutiny
Low — the change is mechanical and directly mirrors the fast path pattern already present a few hundred lines above. Capturing the return value of getPrototype, checking scope.exception(), clearing it, and breaking is the standard JSC idiom for handling exceptions during prototype-chain walks.
Other factors
The Fuzzilli repro is clearly described, the test exercises the exact crash scenario in a subprocess (so a crash would be caught as a non-zero exit code), and no bugs were reported by the hunting system. The fast-path precedent gives high confidence the fix is correct.
112c673 to
8daf971
Compare
There was a problem hiding this comment.
LGTM — minimal targeted fix matching the existing fast-path pattern, with a regression test.
Extended reasoning...
Overview
Fixes a null-dereference crash in the slow path of JSC__JSValue__forEachPropertyImpl (bindings.cpp). The change is 5 lines: store the result of getPrototype, check for a pending exception, clear it, and break out of the loop. A regression test is added in the test suite.
Security risks
None. This is purely defensive error handling — it prevents a crash by bailing out of prototype-chain traversal when an exception occurs, rather than continuing with a null pointer.
Level of scrutiny
Low. The fix is mechanical and mirrors the existing fast-path logic (described in the PR) that already handles this case correctly. The pattern (scope.exception() → tryClearException() → break) is idiomatic in this codebase. The change cannot introduce regressions because the only behavioral difference is breaking out of the prototype walk early on exception, rather than crashing.
Other factors
- No bugs found by the automated hunting system.
- coderabbitai raised no actionable concerns.
- Regression test directly reproduces the Fuzzilli crash scenario and verifies exit code 0 and correct stdout.
bd411d0 to
8daf971
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟣
src/bun.js/bindings/bindings.cpp:5364-5375— The PR fixes the slow-path loop's exception handling, but misses an analogous unhandled exception in the fast-path initialization: ifobject->getPrototype(globalObject)at line 5170 throws (e.g. from a ProxygetPrototypeOftrap), the exception is not cleared,fasthas already been set tofalse, so the entire fast-path block (includingCLEAR_IF_EXCEPTIONat line 5257) is skipped, and execution falls into the slow path with the exception already pending. The firstRETURN_IF_EXCEPTIONin the slow path (line 5276) then fires and returns early without clearing the exception, leaking it to callers. Ascope.tryClearException()call is needed after the initializationgetPrototypecall, matching the pattern used in the inner fast-path loop (line 5257) and the fix applied in this PR.Extended reasoning...
What the bug is: At line 5170 in the fast-path initialization block,
object->getPrototype(globalObject)is called insideif (JSValue proto = ...). This pattern safely avoids dereferencing a null pointer if the call returns an empty JSValue — but if the call throws an exception (setting it in the scope), the exception is left pending after the if-statement exits. There is noCLEAR_IF_EXCEPTIONor equivalent after this call.\n\nThe specific code path: The fast-path initialization block checksif (fast)and thenif (structure->outOfLineSize() == 0 && structure->inlineSize() == 0). When this is true,fast = falseis set first (line 5168), thengetPrototypeis called. IfgetPrototypethrows,protois an empty JSValue (falsy), the if-body at lines 5171–5175 is skipped. Sincefastis alreadyfalse, the entire fast-path enumeration block (lines ~5184–5261, which includes theCLEAR_IF_EXCEPTION(scope)at line 5257) is skipped. Execution falls through to the slow path at line 5263 with the exception still pending.\n\nWhy existing code doesn't prevent it: The PR description claims 'The fast path at line 5121 already handles this correctly withCLEAR_IF_EXCEPTION.' However, thatCLEAR_IF_EXCEPTIONat line 5257 is inside the inner fast-path loop (insideif (anyHits)in the fast-path enumeration block), not after the initializationgetPrototypecall at line 5170. Whenfastis set tofalsebefore the call, that entire block is bypassed entirely.\n\nWhat the impact would be: With the pending exception, the slow path enters the while loop at line 5269. ThegetOwnPropertyNamesorgetOwnNonIndexPropertyNamescall executes (or may itself behave unexpectedly with a pending exception), thenRETURN_IF_EXCEPTION(scope, void())at line 5276 fires immediately. This returns early without ever reaching the cleanup block at lines 5378–5381 that callsscope.tryClearException(). The exception then propagates out offorEachPropertyImplto its Zig caller unexpectedly.\n\nHow to fix it: Addscope.tryClearException()(orCLEAR_IF_EXCEPTION(scope)) after the fast-path initializationgetPrototypecall at line 5170, matching the pattern already used at line 5257 with the comment '// Ignore exceptions from Proxy "getPrototype" trap.' and the fix this PR applied in the slow-path loop.\n\nStep-by-step proof: (1) Create an objectobjwith no inline or out-of-line properties (empty object{}works). (2) Set its prototype to a Proxy whosegetPrototypeOftrap throws. (3) CallforEachPropertyImplonobj. (4)structure->outOfLineSize() == 0 && structure->inlineSize() == 0is true →fast = false(line 5168). (5)object->getPrototype(globalObject)calls the Proxy trap → throws → returns empty JSValue. (6) The if-body (5171–5175) is skipped. (7) The fast-path block (5184–5261) is skipped becausefast == false. (8) We reach the slow path (line 5263) with exception pending. (9)RETURN_IF_EXCEPTIONat line 5276 fires → function returns early. (10) Exception propagates to caller — the caller receives an unexpected exception.
| const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stdout).toBe("OK\n"); | ||
| expect(exitCode).toBe(0); |
There was a problem hiding this comment.
🟡 The test pipes stderr and awaits it in Promise.all but discards the result with , , destructuring and never asserts expect(stderr).toBe(''). Per the project's test/CLAUDE.md conventions, tests spawning Bun processes should assert expect(stderr).toBe('') before asserting on stdout and exit code, so unexpected crash output, warnings, or diagnostic messages printed to stderr won't silently pass.
Extended reasoning...
Convention violation: stderr is collected but never asserted
The test correctly pipes stderr (stderr: 'pipe') and collects it in Promise.all, which is good — it prevents the stderr stream from being orphaned. However, the destructuring on line 23 uses , , to discard the value: const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]). The collected stderr string is then never referenced again, and no assertion like expect(stderr).toBe('') appears before the stdout/exitCode checks.
The project's documented convention
test/CLAUDE.md provides an explicit canonical example for spawning Bun child processes in tests. That example destructures [stdout, stderr, exitCode] and asserts expect(stderr).toBe('') before asserting on stdout and exit code. This ordering ensures that any unexpected output on stderr (crash diagnostics, warnings, panics, sanitizer messages) causes an immediate, descriptive test failure rather than a silent pass.
Why it matters for a crash regression test specifically
This is a regression test for a null-deref / UBSan crash. While a hard crash will produce a non-zero exit code (caught by the existing expect(exitCode).toBe(0)), there are several scenarios where stderr output would slip through without a failure:
- Incomplete fixes that print a diagnostic/warning to stderr but still exit 0 with correct stdout
- Sanitizer-instrumented builds that log a UBSan error to stderr but continue execution
- Proxy
getPrototypeOftraps that print unexpected output before the fix cleanly handles the exception
The closely related pretty-format-overflow.test.ts crash regression test demonstrates this sensitivity by explicitly checking expect(stderr).not.toContain('panic'), expect(stderr).not.toContain('integer overflow'), and expect(stderr).not.toContain('SIGTRAP').
Concrete proof of the gap
- The patched code adds
scope.tryClearException(); break;— on a build with extra diagnostics enabled, it could print a warning to stderr before breaking - The test's
Promise.allcollectsproc.stderr.text()— that string could contain e.g."[warn] exception cleared in forEachProperty\n" - Because of
, ,, the string is discarded; neitherexpect(stderr).toBe('')nor any other assertion on it exists - The test still passes:
stdout === 'OK\n'andexitCode === 0 - The regression in stderr goes undetected
Fix
Change line 23 from:
const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);to:
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);and add before the existing assertions:
expect(stderr).toBe("");
8daf971 to
d3935d7
Compare
![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 the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/test/expect-proxy-prototype-crash.test.ts`:
- Line 25: Remove the flaky stderr equality assertion in the test by deleting
the line that asserts expect(stderr).toBe("") and rely on the process exit code
as the crash indicator; update the test (in
expect-proxy-prototype-crash.test.ts) to only assert on exitCode (or use
expect(code).toBe(...) / expect(exitCode).not.toBe(0) as appropriate) and leave
any stderr output unasserted to avoid ASAN-only failures.
🪄 Autofix (Beta)
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: dcac1f15-929d-4ab2-9715-567307d9b093
📥 Commits
Reviewing files that changed from the base of the PR and between bd411d0283f255e8a8baea435d374d4602c451d7 and d3935d73b349a7574a03ea6c5bd7a9517b93202e.
📒 Files selected for processing (2)
src/bun.js/bindings/bindings.cpptest/js/bun/test/expect-proxy-prototype-crash.test.ts
d3935d7 to
d9a4684
Compare
d9a4684 to
1e25eaa
Compare
There was a problem hiding this comment.
LGTM — my previous concerns (Bun.jest(Bun) → Bun.jest() for ASAN compatibility, and not asserting empty stderr) have been addressed.
Extended reasoning...
Overview
Two-file change: a targeted null-deref fix in the slow-path prototype-chain walk of JSC__JSValue__forEachPropertyImpl in bindings.cpp, plus a regression test. The C++ change adds an exception check after getPrototype on both the fast path (via CLEAR_IF_EXCEPTION) and the slow path (by storing the result, checking scope.exception(), clearing it, and breaking), exactly mirroring the existing pattern in the function.
Security risks
None beyond the crash itself being fixed. The change suppresses exceptions from Proxy getPrototypeOf traps during property enumeration — this is the correct defensive behavior for an internal formatting utility, not a security-sensitive code path.
Level of scrutiny
Low-to-medium. The fix is a single well-understood pattern (check-and-clear exception after a fallible JSC call) applied to a code path that was already using the same pattern on the fast path. The logic is straightforward and the diff is small.
Other factors
My two previous inline concerns have been addressed in subsequent commits: Bun.jest(Bun) was corrected to Bun.jest() (matching the expect-stack-overflow-crash.test.ts pattern) to avoid ASAN build failures, and the expect(stderr).toBe('') assertion was omitted to prevent ASAN-output flakiness. The test correctly verifies exit code 0 and stdout output as crash detection signals.
1e25eaa to
a12259f
Compare
There was a problem hiding this comment.
LGTM — prior concerns addressed.
Extended reasoning...
Overview
Minimal two-file change: a targeted null-deref fix in JSC__JSValue__forEachPropertyImpl (slow path of the prototype-chain walk in bindings.cpp) plus a regression test.
Security risks
None beyond the crash itself, which is being fixed. The fix correctly suppresses a thrown exception from a Proxy getPrototypeOf trap by checking scope.exception() and calling scope.tryClearException() before breaking — matching the existing fast-path pattern. No auth, crypto, or permissions code involved.
Level of scrutiny
Low. The change is five lines of C++ following an established pattern already used in the same function (both the fast path and the CLEAR_IF_EXCEPTION idiom elsewhere in the loop). The fix is logically sound.
Other factors
Two rounds of review feedback were incorporated: (1) the test was updated from Bun.jest(Bun) to Bun.jest() to avoid a failure-mode on ASAN builds, and (2) the flaky expect(stderr).toBe('') assertion was removed per the CodeRabbit suggestion (commit d9a4684). Both inline comments are marked resolved. No new bugs were found by the automated bug hunting pass.
8d40304 to
dd063fa
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/bun.js/api/cron.classes.ts:1— This PR accidentally includessrc/bun.js/api/cron.classes.ts, an unrelated empty stub (export default []) for an in-progress cron feature. The cmake/Sources.json globsrc/bun.js/{api,node,test,webcore}/*.classes.tspicks it up as input togenerate-classes.ts, which explicitly throwsAggregateErrorwhen any input file has an empty default export — breaking fresh builds. This file should be removed from this PR.Extended reasoning...
What the bug is
src/bun.js/api/cron.classes.tswas added as part of this PR with contentexport default [];— a placeholder stub for a future cron JS class binding. It is completely unrelated to the stated goal of this PR (handling exceptions fromgetPrototypeinforEachProperty). Every other*.classes.tsfile insrc/bun.js/api/contains actual class definitions via thedefine()helper fromclass-definitions.The specific code path that triggers the build failure
cmake/Sources.jsoncontains aZigGeneratedClassesSourcesentry with glob patternsrc/bun.js/{api,node,test,webcore}/*.classes.ts. This pattern matchessrc/bun.js/api/cron.classes.tsand passes it as an argument tosrc/codegen/generate-classes.ts. In that script at line 2773:if (\!(result?.default?.length ?? 0)) { errors.push(new TypeError(`Missing classes in "${file}". Expected export default [ define(...) ] ...`)); continue; }
For
export default [],result.default.length === 0, so\!(0 ?? 0)istrueand the error is pushed. Then at line 2794,if (errors.length) { throw new AggregateError(errors, 'Failed to generate classes'); }terminates the entire codegen step.Why existing code does not prevent it
The build system uses a glob, not an explicit file list, so any new
*.classes.tsfile added tosrc/bun.js/api/is automatically picked up. There is no guard that ignores empty stubs — the generator treats an empty array as an error by design, since every classes file is expected to export real class definitions.Impact
The ZigGeneratedClasses codegen step fails, which blocks
cmake --buildfor any fresh checkout of this branch. Any CI job that does a clean configure will hit this. Pre-configured build caches may mask the failure, explaining why initial CI may have passed, but any fresh build environment — including contributor setups, release pipelines, and Docker-based builds — will break.Step-by-step proof
- Developer clones the repo at this commit and runs
cmake -S . -B build && cmake --build build - CMake globs
src/bun.js/api/*.classes.tsand findscron.classes.ts - It passes
src/bun.js/api/cron.classes.tsas an argument tobun src/codegen/generate-classes.ts - The script imports the file;
result.defaultis[], soresult.default.length === 0 \!(0 ?? 0)evaluates totrue; aTypeErroris pushed to the errors array- After processing all files,
errors.length > 0, sonew AggregateError(errors, 'Failed to generate classes')is thrown - The codegen step exits non-zero; CMake aborts the build
How to fix it
Remove
src/bun.js/api/cron.classes.tsfrom this PR. The cron feature work should live in a separate, properly-described PR once the class bindings are actually implemented. - Developer clones the repo at this commit and runs
dd063fa to
ffb6ab8
Compare
|
The fix in this PR is verified correct against the Fuzzilli repro (UBSan null member call at |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟣
src/bun.js/bindings/bindings.cpp:5238-5249— This is a pre-existing bug in the same forEachPropertyImpl function touched by this PR: the slow-path property loop at lines 5165-5169 places CLEAR_IF_EXCEPTION(scope) after a continue, so when getPropertySlot() throws AND returns false (e.g. from a Proxy [[Get]] trap), the exception is never cleared. The next loop iteration then calls getPropertySlot with a stale pending exception, violating JSC ThrowScope invariants. Fix by calling scope.tryClearException() unconditionally before the if-check, matching the pattern already used in the sibling forEachPropertyOrdered function.Extended reasoning...
What the bug is and how it manifests
In the slow-path property loop of
forEachPropertyImpl(lines 5165-5169 inbindings.cpp), after callingobject->getPropertySlot(globalObject, property, slot), the code takes one of two paths:- If
getPropertySlotreturnstrue(property found): falls through toCLEAR_IF_EXCEPTION(scope)on line 5169. - If
getPropertySlotreturnsfalse(property not found): hitscontinueon line 5167, skipping theCLEAR_IF_EXCEPTIONentirely.
A Proxy can make
getPropertySlotboth throw and returnfalse(the[[Get]]trap throws, the engine returnsfalseas the not-found sentinel). When that happens in the false-return branch, the exception stays pending in the JSC ThrowScope for the remainder of the loop.The specific code path that triggers it
// lines 5165-5169 (slow path, inside forEachPropertyImpl) JSC::PropertySlot slot(object, PropertySlot::InternalMethodType::Get); if (!object->getPropertySlot(globalObject, property, slot)) continue; // exception still pending if getPropertySlot threw // Ignore exceptions from "Get" proxy traps. CLEAR_IF_EXCEPTION(scope); // only reached when getPropertySlot returned true
The comment on line 5168 explicitly states the intent is to suppress Proxy trap exceptions, but the implementation only clears them on the true-return path.
Why existing code does not prevent it
The
CLEAR_IF_EXCEPTIONmacro is a conditional clear. By jumping over it viacontinue, the exception remains live. JSC ThrowScope rules require that no JSC function be called while an exception is pending. The next iteration callsgetPropertySlotagain on the same ThrowScope: debug builds fire anassertNoExceptionassertion; release builds may early-exit or exhibit undefined behaviour.What the impact would be
Any enumeration of an object whose property names include an entry where a Proxy
[[Get]]trap throws can trigger this code path. In debug builds this is a hard assertion failure. In release builds the stale exception propagates through subsequentgetPropertySlotcalls, potentially causing premature loop termination or a phantom exception reaching the caller.Step-by-step proof with a concrete example
- Object
ois a Proxy-wrapped object where thegettrap for key"foo"throwsTypeError. getOwnPropertyNames(o)returns["foo"].- Slow-path loop iterates: property =
"foo", callso->getPropertySlot(..., "foo", slot). - The Proxy
gettrap fires and throws.getPropertySlotreturnsfalse. if (!false)triggerscontinue. Exception is still pending in scope.- Next iteration: calls
getPropertySlotagain. - JSC detects an active exception in the ThrowScope: debug assertion fires, or release-mode early-exit occurs.
How to fix it
Mirror the pattern already used correctly in the sibling function
forEachPropertyOrdered(lines 5335-5340):// forEachPropertyOrdered — correct pattern bool hasProperty = object->getPropertySlot(globalObject, property, slot); (void)scope.tryClearException(); // unconditional clear if (!hasProperty) { continue; }
Apply the same structure to the slow-path loop in
forEachPropertyImpl: capture the return value, callscope.tryClearException()unconditionally, then branch on the captured value. This matches the intent stated in the comment at line 5168 and makes the false-return path behave the same as the true-return path with respect to exception state. - If
…get_all_property_names Hold the current link in the prototype chain as a JSValue under an EnsureStillAliveScope so a Proxy trap that runs JS (and may GC) during property enumeration can't free it out from under us. Also guard the equivalent getPrototype().getObject() pattern in napi_get_all_property_names, propagating Proxy trap exceptions as napi_pending_exception. Supersedes #29071 #28991 #28919 #28918 #28882 #28854 #28530 #28325.
|
Superseded by #29642, which combines the |
…get_all_property_names Hold the current link in the prototype chain as a JSValue under an EnsureStillAliveScope so a Proxy trap that runs JS (and may GC) during property enumeration can't free it out from under us. Also guard the equivalent getPrototype().getObject() pattern in napi_get_all_property_names, propagating Proxy trap exceptions as napi_pending_exception. Supersedes #29071 #28991 #28919 #28918 #28882 #28854 #28530 #28325.
…get_all_property_names Hold the current link in the prototype chain as a JSValue under an EnsureStillAliveScope so a Proxy trap that runs JS (and may GC) during property enumeration can't free it out from under us. Also guard the equivalent getPrototype().getObject() pattern in napi_get_all_property_names, propagating Proxy trap exceptions as napi_pending_exception. Supersedes #29071 #28991 #28919 #28918 #28882 #28854 #28530 #28325.
…get_all_property_names Hold the current link in the prototype chain as a JSValue under an EnsureStillAliveScope so a Proxy trap that runs JS (and may GC) during property enumeration can't free it out from under us. Also guard the equivalent getPrototype().getObject() pattern in napi_get_all_property_names, propagating Proxy trap exceptions as napi_pending_exception. Supersedes #29071 #28991 #28919 #28918 #28882 #28854 #28530 #28325.
…get_all_property_names Hold the current link in the prototype chain as a JSValue under an EnsureStillAliveScope so a Proxy trap that runs JS (and may GC) during property enumeration can't free it out from under us. Also guard the equivalent getPrototype().getObject() pattern in napi_get_all_property_names, propagating Proxy trap exceptions as napi_pending_exception. Supersedes #29071 #28991 #28919 #28918 #28882 #28854 #28530 #28325.
…get_all_property_names Hold the current link in the prototype chain as a JSValue under an EnsureStillAliveScope so a Proxy trap that runs JS (and may GC) during property enumeration can't free it out from under us. Also guard the equivalent getPrototype().getObject() pattern in napi_get_all_property_names, propagating Proxy trap exceptions as napi_pending_exception. Supersedes #29071 #28991 #28919 #28918 #28882 #28854 #28530 #28325.
…p or getPrototypeOf throws during the property walk (#29642) ### Problem - `Bun.inspect()`, `console.log()` and `expect()` failure output crash with `Segmentation fault at address 0x5` (debug builds: UBSan "member call on null pointer of type 'JSC::JSCell'" in `JSCJSValueCell.h`) when a property lookup throws while an object is being formatted. Release repro: ```js const proto = new Proxy({ a: 1 }, { getPrototypeOf() { throw new Error("boom"); } }); console.log(Object.create(proto)); ``` and likewise with a Proxy whose `get` trap (or a getter reached through a Proxy) throws for one property. - The same happens with a lazily initialized property of the `Bun` object whose initializer throws (fuzzer sample: `globalThis.Symbol` replaced, then `Bun.inspect(Bun)`; the builtin behind `Bun.$` calls `Symbol("cwd")`). Release builds print `Bun` with most of its properties missing (72 of 115 on the shipped build); debug builds abort with `ASSERTION FAILED: Unexpected exception observed` / `Symbol is not a function. (In 'Symbol("cwd")' ...)` when the next lazy property is built. - Plain-JavaScript variant of the same leak: `console.log` of a module namespace during an import cycle, when one export is still in its temporal dead zone, throws `ReferenceError: Cannot access 'x' before initialization` out of `console.log` (the stale exception is picked up while the next export is formatted). `util.inspect` prints such an export as `<uninitialized>`. - Cause, in the slow path of `JSC__JSValue__forEachPropertyImpl` (`src/jsc/bindings/bindings.cpp`): - `object->getPropertySlot()` reports a throwing Proxy trap, a throwing lazy initializer or a TDZ namespace export as "not found" with the exception still pending, and the loop `continue`d before the `CLEAR_IF_EXCEPTION` below it. The following lookups and formatting callbacks then run with that exception pending (the dropped properties, the rethrown `ReferenceError`, the debug assertion). - When the walk moves to the next prototype, `iterating->getPrototype(globalObject).getObject()` runs `getObject()` on the empty `JSValue` that `getPrototype` returns when it threw (either because of the stale exception above or because the `getPrototypeOf` trap itself throws). The empty value passes `isCell()`, so this reads the type byte of a null cell: the fault at address 5. - `napi_get_all_property_names` (`src/jsc/bindings/napi.cpp`, descriptor filter loop) has the same `getPrototype().getObject()` chain after an unchecked `getOwnPropertyDescriptor`, so a Proxy trap throwing there returned `napi_ok` with an exception pending in own-only mode and segfaulted in include-prototypes mode. - `defaultBunSQLObject` / `constructBunSQLObject` (`src/jsc/bindings/BunObject.cpp`) had a debug-only block that handed a sql module load failure to `reportUncaughtExceptionAtEventLoop` while the exception was still pending on the VM, so `globalThis.Symbol = NaN; Bun.sql` (and the fuzzer sample above, once the walk gets past `Bun.$`) aborted debug builds with `ASSERTION FAILED: ... object->structure() == this` in `Structure::storedPrototype` instead of throwing. ### Fix - `bindings.cpp`: clear the exception after `getPropertySlot` regardless of its result, which is what the ordered variant `JSC__JSValue__forEachPropertyOrdered` already does; read the next prototype into a `JSValue`, clear the exception and stop the walk when it is empty. (An earlier revision also held the prototype being walked under an `EnsureStillAliveScope`; dropped, since the raw pointer is used after every call into JS in the loop body and so is live across them anyway.) - Behaviour change to note: a property whose lookup throws is now left out of the output instead of the whole `console.log` / `Bun.inspect` call throwing or crashing. For TDZ namespace exports this differs from `util.inspect`'s `<uninitialized>`; printing that marker would be a formatter feature on top of this fix and is not attempted here. - `napi.cpp`: check for an exception after `getOwnPropertyDescriptor` and after `getPrototype` and return `napi_pending_exception`, which is what Node returns for these cases. - `BunObject.cpp`: drop the debug-only report. The exception is propagated to the reader by the `RETURN_IF_EXCEPTION` right below it, so debug builds now behave like release builds (`Bun.sql` throws). - Why this is the right place: the formatter deliberately swallows errors thrown by individual properties (getters, traps) and prints the rest of the object; these two sites were the only ones in the walk that acted on a "not found" result or a prototype value before clearing the exception that produced it. Skipping just the property (or stopping at just the prototype) whose lookup threw is the existing behaviour for every other throw site in this function. - Verification: - `test/js/bun/util/inspect.test.js`, "Bun.inspect when a property lookup throws" (5 spawned cases): a Proxy `get` trap, a getter behind a Proxy, a `getPrototypeOf` trap, a throwing lazy `Bun` property, and a two-file import cycle with a TDZ export. Without the fix (shipped release build and an unfixed debug build) all five fail: the Proxy children segfault / fail UBSan, the `Bun` child prints `[false,false,...]` on release and aborts on debug, the cycle child exits 1 with the `ReferenceError`; with it each prints everything except the one property whose lookup threw. - `test/js/bun/util/BunObject.test.ts`, "a lazy property whose builtin fails to load throws from the read": `Bun.$` / `sql` / `SQL` / `postgres` with `Symbol` broken throw a `TypeError` on two consecutive reads. Aborts on an unfixed debug build (the `BunObject.cpp` hunk); passes on release either way, as the removed block is debug-only. The fixture builds `process.env` before breaking `Symbol` because the `$` builder reads it, and building it on Windows reifies another `Bun` property mid-lookup, which on a Windows debug build would hit the separate `storedPrototype` assertion that #37001 fixes (verified on Linux only). - `test/napi/napi.test.ts`: `getOwnPropertyDescriptor` trap throwing in own-only and include-prototypes mode (compared against Node), and a `getPrototypeOf` trap that throws on the second call so the check after `getPrototype` is the one that fires. - Repros above and the tests also run clean under `BUN_JSC_validateExceptionChecks=1`. ### Background - `forEachPropertyImpl` is the property walk behind Bun's native formatter. It collects the property names of the object and of up to five prototypes, looks each one up through the original object with `getPropertySlot`, and hands the value to a callback that formats it. Errors thrown by individual properties are swallowed on purpose so that one bad getter does not make `console.log` throw. - A JSC exception is "pending" state on the VM, not C++ unwinding. A function that throws returns a failure value (`false`, or the empty `JSValue`) and leaves the exception on the VM; until something clears or rethrows it, most JSC entry points return early as soon as they are called, and debug builds assert when a function that did not throw is observed returning with an exception pending. `CLEAR_IF_EXCEPTION` drops the pending exception. - The empty `JSValue` (`JSValue()`) is encoded as 0. `isCell()` is true for it, so `getObject()` on it dereferences a null cell pointer rather than returning null; callers have to test the value itself first. - Lazy properties of the `Bun` object are entries in a static property table whose value is produced by a builder the first time the property is read (`PropertyCallback`). Some builders evaluate built-in JavaScript modules (the shell for `Bun.$`, the sql module for `Bun.sql`), so they can throw when that module fails to evaluate, and JSC reports that to the reader as "property not found" plus a pending exception. ### Consolidated duplicates Found repeatedly by the fuzzer (fingerprint `d678cafe50a2ad6e`). Earlier round, folded in here in April: #29071 #28991 #28919 #28918 #28882 #28854 #28530 #28325. This round, closed in favour of this PR: #30099 #30245 #37160 #37175 #37213 #37256 #37428 #38700 #38921 #39363 #39365 #39380 #39412 #39413 (and #39411, closed earlier). The `BunObject.cpp` hunk and the `BunObject.test.ts` test come from #30245 / #37428; the same hunk is also part of #37001, which fixes the underlying `storedPrototype` assertion in JSC. Related fixes that are not part of this bug and stay open on their own: #39382 (a custom inspect function when `node:util` fails to load), #37202 (`util.isError` with a throwing `getPrototypeOf` trap), #37331 (`forEachPropertyOrdered` when the callback throws), #37001 (stale structure in `JSObject::getPropertySlot`), #32263 (additional checks in the same napi loop). <!-- robobun:evidence:begin --> --- **no test proof** · iteration 11 · 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 test/napi/napi.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: robobun <robobun@users.noreply.github.com>
1 participant
Fuzzilli found a null deref in
JSC__JSValue__forEachPropertyImplwhen formatting an object whose prototype chain contains a Proxy.When walking the prototype chain,
iterating->getPrototype(globalObject)can throw (e.g. from a ProxygetPrototypeOftrap or from side-effects of the walk), and returns an emptyJSValue. Calling.getObject()on that empty value invokesasCell()which returns null, then dereferences it — UBSan catches the null member call.The fast path at line 5121 already handles this correctly with
if (JSValue proto = ...)which is false for empty values, followed byCLEAR_IF_EXCEPTION. The slow path at line 5241 did not — check the exception and bail out of the walk.Fuzzilli repro (minimized):
Fingerprint:
9e09c9e7bd37325a