Clear the exception left by a failed lazy property in forEachProperty - #39428
Clear the exception left by a failed lazy property in forEachProperty#39428robobun wants to merge 1 commit into
Conversation
getPropertySlot() returns false with an exception pending when a static lazy property's initializer throws. JSC__JSValue__forEachPropertyImpl skipped straight to the next property in that case, so the next lazy property was reified with that exception still set: an assertion in debug builds, and in release builds setUpStaticFunctionSlot reported every following property as missing until an already reified one cleared the exception. Clear it before moving on, like forEachPropertyOrdered already does. Also drop the debug-only reporting in the Bun.sql builders: it reported the exception while it was still pending, which made the property lookups inside the reporter trip over the same thing.
|
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 (3)
💤 Files with no reviewable changes (1)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 1 remains after this review. WalkthroughChangesBun.inspect exception handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Closing this as a duplicate of #29642. That PR already carries both hunks from here (the same clear-before- |
There was a problem hiding this comment.
LGTM — a targeted reorder so the pending exception from a failed lazy-property lookup is cleared before continue, matching what forEachPropertyOrdered already does.
What was reviewed:
- Confirmed the
bindings.cpphunk mirrors the existing pattern atbindings.cpp:5809-5814inforEachPropertyOrdered(callgetPropertySlot, clear, then branch). - Checked the removed
#if BUN_DEBUGblocks inBunObject.cpp— they calledreportUncaughtExceptionAtEventLoopwith the exception still pending;RETURN_IF_EXCEPTIONimmediately below already propagates it, and no other caller uses that report-while-pending pattern. - Tests spawn subprocesses, drain pipes concurrently, assert stdout before exit code, and cover both the release-build symptom (properties after
redisdropped) and the debug-build assertion (stack-overflow unwind).
Extended reasoning...
Overview
Three files touched: a 4-line reorder in src/jsc/bindings/bindings.cpp inside JSC__JSValue__forEachPropertyImpl, removal of two debug-only reportUncaughtExceptionAtEventLoop blocks in src/jsc/bindings/BunObject.cpp (defaultBunSQLObject / constructBunSQLObject), and two new subprocess tests in test/js/bun/util/inspect.test.js.
The root cause is well diagnosed in the PR description: when getPropertySlot returns false because a lazy-property initializer threw, the old code hit continue before CLEAR_IF_EXCEPTION, leaving the exception pending for the next iteration. The fix stores the return value, clears unconditionally, then branches — exactly what the sibling forEachPropertyOrdered at bindings.cpp:5809-5814 already does.
Security risks
None. This is exception-handling ordering inside an inspection/formatting path; no untrusted input parsing, allocation sizing, or auth surface is involved.
Level of scrutiny
Low-to-medium. The core change is a mechanical reorder that brings one loop body in line with its ordered twin a few lines below. The BunObject.cpp deletions remove debug-only code that violated the "clear before calling reportUncaughtExceptionAtEventLoop" convention the PR description says every other caller follows; the RETURN_IF_EXCEPTION immediately after already surfaces the error to the caller, so no diagnostic is lost — it just moves from an event-loop uncaught report to the actual throw site.
Other factors
The two new tests are well-constructed per the repo's review guidelines: it.concurrent subprocess spawns with bunEnv spread, concurrent pipe drains, stdout asserted before exit code, and both cover distinct observable symptoms (the REDIS_URL test needs no stack tricks and asserts specific properties by name; the stack-overflow test unwinds frame-by-frame and checks the first completing inspect plus a fully-unwound one). The PR description documents that both fail on release and debug builds pre-fix and pass post-fix across eight debug runs, and that the sibling slot-lookup sites (JSC__JSValue__getPropertyValue, JSPropertyIterator) were audited and already handle the exception. The known related issue on process lazy builders is explicitly scoped out with a rationale. No prior review comments on the PR.
What does this PR do?
Fixes a fuzzer-found assertion failure (
ExceptionScope::releaseAssertNoException, "Unexpected exception observed") in the native property walk behindBun.inspect/console.log.Reduced repro: catch a stack overflow and, with the stack still almost full, inspect an object that has lazily initialized static properties:
The fuzzer got there through
new CompressionStream(globalThis), whoseERR_INVALID_ARG_VALUEmessage inspects the argument.Root cause:
Bun.$is a static lazy property whose initializer runs JS. With no JS stack left it throws,reifyStaticPropertystores nothing, andsetUpStaticFunctionSlotreturns false with the exception still pending (that is the contract in our JSC fork: the caller ofgetPropertySlothas to look at the exception).JSC__JSValue__forEachPropertyImpldidso the
continueskipped the clear. The next property in the table,Bun.Archive, was then reified with the exception still set, and its Rust-sideto_js_host_callwrapper asserts on exactly that. In release builds the damage is different:setUpStaticFunctionSlotchecksvm.exceptionForInspection()after reifying, so with a stale exception every following lazy property is reified but reported as missing, until the walk reaches a property that was already reified (that lookup takes the normal path and clears the exception). With an invalidREDIS_URL,Bun.inspect(Bun)on current release builds prints nothing afterRedisClient:redislegitimately fails, andsecrets,writeand thezstd*functions go missing with it.The fix is the hunk in
bindings.cpp: callgetPropertySlot, clear, thencontinue, which is whatJSC__JSValue__forEachPropertyOrderedalready does a few lines further down. I checked the other slot lookups in the bindings:JSC__JSValue__getPropertyValuereturns on the exception, andJSPropertyIteratorgoes throughfrom_js_host_call_generic, so the exception propagates there.With that fixed, the same repro hit a second assertion (
Structure::storedPrototype) further along the same unwind. TheBun.sql/Bun.SQLbuilders had a debug-only block that reported therequirefailure throughreportUncaughtExceptionAtEventLoopwhile the exception was still pending. The reporter looks upprocess._fatalException, which is itself a lazy property, so that lookup ran into the same stale-exception situation one level down. Every other caller ofreportUncaughtExceptionAtEventLoopclears first. The block is removed:RETURN_IF_EXCEPTIONright below it already hands the error to whoever touchedBun.sql, which is the more useful place for it to show up anyway.Not changed here: the lazy builders on
processuse a different policy (callLazyProcessBuilderclears, reports the error as uncaught and storesundefined). Running the same unwind againstprocessleavesprocess.nextTick,process.stdoutand a few others permanentlyundefinedand prints six RangeErrors. That is pre-existing and a separate piece of code; tracking it separately.How did you verify your code works?
Two tests added to
test/js/bun/util/inspect.test.js:only the property that failed is left out:REDIS_URLset to garbage, soBun.redisthrows, then checks thatsecrets,writeandzstdDecompressare still printed. No stack tricks involved.initializers that run out of stack: the reduced repro above. It unwinds a stack overflow one frame at a time and checks the firstBun.inspect(Bun)call that returns;Archive(declared right after$) andversionhave to be in the output.Both fail before the fix: on the release build they fail on the printed output (
secrets: falseetc., and aBun {dump that starts atinspect:), on a debug build both child processes die on the assertions above. Both pass withbun bd test, and I ran them eight times in a row on the debug build. After the fix the first completing inspect at the bottom of the unwind is missing exactly$,sql,postgresandSQL(the four builders that run JS); before, it was missing 85 of the 115 properties.The fuzzer script itself (bounded to the deepest 1500 frames of the unwind, the full version inspects
globalThisat every one of ~50k frames and takes ages on a debug build) now runs to completion on the debug build; the unfixed debug binary still aborts on it. Also raninspect.test.js,BunObject.test.ts, thetest/js/bun/consoletests and the sql adapter tests on the debug build.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