process: don't run the uncaught-exception machinery from inside a lazy property lookup - #37258
process: don't run the uncaught-exception machinery from inside a lazy property lookup#37258robobun wants to merge 6 commits into
Conversation
…y property lookup Lazy process properties (env, stdout, stdin, nextTick, config, allowedNodeEnvironmentFlags, finalization, mainModule) are reified in the middle of a property lookup. When a builder threw, it cleared the exception and called reportUncaughtExceptionAtEventLoop synchronously, re-entering JS (process._fatalException lookup, uncaughtException handlers, error printing) while JSObject::getPropertySlot still holds the object's Structure*. That JS reifies more static properties and transitions structures under the walk, tripping the stale-Structure assert in Structure::storedPrototype. Queue the report as a microtask so it runs after the lookup completes. Also make the processEnvObject LazyProperty initializer set a value when createEnvironmentVariablesMap fails (on Windows process.env is built by a JS builtin that user code can break by clobbering globals): returning without init.set violates LazyProperty's contract and aborts the process.
WalkthroughChangesThe PR defers uncaught-exception reporting from process lazy-property builders through microtasks. Environment construction now preserves pending exceptions and uses empty-object fallbacks. Tests cover asynchronous reporting, nested reification failures, and platform-specific Lazy property safety
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
reifyStaticProperty putDirects any non-empty value even when the builder left an exception pending, and setUpStaticFunctionSlot then reports the slot as not-found, so the in-progress prototype-chain walk advances through the pre-putDirect Structure* and trips the storedPrototype assert. Return empty like the other Bun object builders so the failure path never transitions the structure, and the access throws a catchable error instead.
|
Same crash family, different sites; checked each candidate's diff for overlap:
No overlapping hunks with any of the three, so they can land in any order. One of this PR's test fixtures pre-reifies |
|
Updated 9:47 AM PT - Aug 9th, 2026
❌ @robobun, your commit ccbda6a has 1 failures in
🧪 To try this PR locally: bunx bun-pr 37258That installs a local version of the PR into your bun-37258 --bun |
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/node/process/process.test.js`:
- Around line 2471-2485: Extend the subprocess script in the Bun.env
error-handling test so that, after the catch block, it accesses Bun.env again
and logs or asserts that the recovered value is an object. Update the Windows
expected stdout in the existing process test to include this recovery assertion
while preserving the current non-Windows output and alive check.
🪄 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: 9d4fa17f-fa04-4994-9016-3c4068f25264
📒 Files selected for processing (4)
src/jsc/bindings/BunObject.cppsrc/jsc/bindings/BunProcess.cppsrc/jsc/bindings/ZigGlobalObject.cpptest/js/node/process/process.test.js
There was a problem hiding this comment.
I re-reviewed after the fixes in d44e6a6, d5b79ab, 99b139d, and 8b244c7 — everything I flagged earlier is addressed and no new issues were found. Given this touches JSC exception-scope discipline, the LazyProperty contract, and observable uncaughtException timing, and needs to land alongside three overlapping open PRs (#37001/#37175/#37213), a maintainer sign-off is still warranted.
What was reviewed
reportLazyPropertyBuilderException: theQueuedTask/BunInvokeJobWithArgumentspattern matches existing use at ZigGlobalObject.cpp:3609 and WebStreamsMisc.cpp:167; the header declaration in BunProcess.h covers the symbol so the dropped local declaration is correct.m_processEnvObjectinitializer: fallback toconstructEmptyObjecton throw satisfies the LazyProperty must-set contract; both PropertyCallback callers (constructEnv,constructEnvObject) now handle the pending exception.- Confirmed the two remaining
#if BUN_DEBUGreportUncaughtExceptionAtEventLoopsites in BunObject.cpp are deliberately left for the sibling PRs — no overlapping hunks.
Extended reasoning...
Overview
The PR modifies four files: BunProcess.cpp (adds reportLazyPropertyBuilderException helper that queues the report as a microtask instead of calling reportUncaughtExceptionAtEventLoop synchronously; rewires eight lazy-builder call sites), ZigGlobalObject.cpp (m_processEnvObject initializer now sets an empty-object fallback and leaves the exception pending instead of asserting on init.set(nullptr)), BunObject.cpp (constructEnvObject for Bun.env now opens a throw scope and returns empty on exception), and test/js/node/process/process.test.js (four new subprocess tests).
Security risks
None identified. The change is purely about exception-handling ordering and crash avoidance under deliberately hostile global tampering (fuzzer inputs). No auth, crypto, network, or filesystem surface is touched.
Level of scrutiny
High. This is JSC-bindings code that manipulates exception scopes (DECLARE_TOP_EXCEPTION_SCOPE, tryClearException), the LazyProperty contract, and microtask queuing — all areas where subtle mistakes cause use-after-free or assertion crashes rather than test failures. The observable-behavior change (uncaughtException handler now fires after the triggering statement rather than during it) is defensible but is a semantic shift a maintainer should ratify. The Windows-only processEnvObject fallback path can only be fully verified in Windows CI.
Other factors
All three of my prior findings on this PR were addressed (the Bun.env sibling caller, the redundant forward declaration, and the comment-length cleanup). The author gave a reasoned response for leaving the two #if BUN_DEBUG SQL-builder blocks to the three overlapping open PRs, which is a coordination call a human reviewer should confirm. The QueuedTask construction matches two existing in-tree call sites, so the pattern is established. Test coverage is good: four subprocess tests pin the deferred ordering, the env-builder fallback, the Bun.env throw path, and the stdio builder path, with per-platform expected output. CI (#90963) is still building at the time of this review.
There was a problem hiding this comment.
I re-reviewed after the follow-up commits (d44e6a6, d5b79ab, 99b139d, 8b244c7) and didn't find any new issues — the earlier Bun.env sibling and the redundant forward-declaration are addressed, and the added tests cover the recovery path. Given this touches JSC exception handling inside lazy property builders and sits alongside three overlapping open PRs (#37001/#37175/#37213), a maintainer sign-off on the merge order is still worthwhile.
What was reviewed
reportLazyPropertyBuilderExceptionfollows the establishedQueuedTask/vm.queueMicrotaskpattern used inWebStreamsMisc.cppandJSReadRequest.cpp, so the queuedException*is GC-rooted the same way.- The
m_processEnvObjectinitializer's fallback always callsinit.setand leaves the exception pending; both PropertyCallback callers (constructEnv,constructEnvObject) now handle it. - The two remaining
#if BUN_DEBUGsynchronous-report sites indefaultBunSQLObject/constructBunSQLObjectare intentionally left for the other open PRs per the author's response.
Extended reasoning...
Overview
The PR fixes two fuzzer-found crashes in the lazy process.* property builders: (1) a synchronous reportUncaughtExceptionAtEventLoop call from inside getPropertySlot's prototype-chain walk, which re-enters JS and transitions structures under a cached Structure* (debug-build assertion); and (2) the m_processEnvObject LazyProperty initializer calling init.set(nullptr) when the Windows windowsEnv builtin throws (release-build abort). The fix defers the report via a microtask (QueuedTask + vm.queueMicrotask) and gives the initializer an empty-object fallback with the exception left pending. A follow-up commit extended the same handling to the Bun.env builder (constructEnvObject) after my earlier comment. Four subprocess tests pin the deferred ordering and the recovery behaviour on both Windows and POSIX.
Security risks
None. The trigger requires deliberate global tampering (globalThis.Proxy = 123, etc.) in the user's own process; no untrusted input is parsed and no privilege boundary is crossed.
Level of scrutiny
High. This is C++ JSC binding code touching exception scopes, LazyProperty initializer contracts, and microtask queuing — an area where the repo review guide explicitly calls out re-entrancy and GC-rooting hazards. I verified the new reportLazyPropertyBuilderException helper matches the established QueuedTask/BunInvokeJobWithArguments pattern already used at WebStreamsMisc.cpp:167 and JSReadRequest.cpp:56 for passing heap JSValues through the microtask queue, so the Exception* is rooted the same way those call sites' values are.
Other factors
All three of my earlier inline findings are resolved (the Bun.env sibling in d44e6a6, comment trimming in d5b79ab, and the redundant declaration in 99b139d), and CodeRabbit's test-recovery suggestion landed in 8b244c7. The one intentionally-declined item — the two #if BUN_DEBUG blocks in defaultBunSQLObject/constructBunSQLObject — is covered by open PRs #37001/#37160/#37213 per the author, which is a reasonable scoping call but does mean a maintainer should confirm the intended landing order. That merge-order coordination, plus the general sensitivity of JSC re-entrancy changes, is why I'm deferring rather than approving.
|
CI status: 190/196 jobs green on the latest run (build 90985). The one red job is the darwin 14 aarch64 test-bun step failing outside the runner with exit 255, which also fails the same way on main (it was red on both runs of this PR and is tagged pre-existing by the CI tooling); the two warning entries are known-flaky tests that passed when retried alone. No failures touch this PR's changes. The diff is ready for review. |
What does this PR do?
Fixes two crashes, found by fuzzing, in the lazy builders behind
process.*properties when user code clobbers a global the builder depends on.1. The builder failure report re-entered JS from inside the property lookup.
Lazy process properties (
env,stdout,stdin,nextTick,config,allowedNodeEnvironmentFlags, finalization,mainModule) are reified bysetUpStaticFunctionSlot/reifyAllStaticPropertiesin the middle ofJSObject::getPropertySlot, whose prototype-chain walk holds the object'sStructure*. When a builder threw,BunProcess.cppcleared the exception and calledreportUncaughtExceptionAtEventLoopsynchronously. That runs the uncaught-exception machinery (process._fatalExceptionlookup,uncaughtExceptionhandlers, error printing), i.e. arbitrary JS, which reifies more static properties and transitions object structures while the walk is still on the stack. On assertion-enabled builds this trips:The report is now queued as a microtask, so it runs after the lookup completes. Clearing (rather than propagating) stays:
node:worker_threadspreload deletes main-onlyprocesskeys, which bulk-reifies every builder throughreifyAllStaticProperties, and a pending exception there aborted workers in stress tests (see the comment oncallLazyProcessBuilder; the stale "reifyStaticProperty performs no exception check" comments are updated to point there). Observable change: anuncaughtExceptionhandler now sees the builder failure after the statement that triggered reification, not in the middle of it.2. The
process.envinitializer violated LazyProperty's contract on failure.On Windows
process.envis built by thewindowsEnvJS builtin. When it throws (e.g.globalThis.Proxyclobbered),createEnvironmentVariablesMapreturns with the exception pending and them_processEnvObjectinitializer calledinit.set(...getObject())on a non-object:A
LazyPropertyinitializer that returns withoutinit.setaborts release builds too. The initializer now falls back to an empty object and leaves the exception pending, the same pattern #37175 and #37213 use for theutil.inspectinitializers (those PRs cover theutilInspectsites and thewindowsEnvread ofBun.inspect; this PR covers theprocessbuilders and the env initializer, with no overlapping hunks).Both need deliberate global tampering to hit, but fuzzers reach them and crash 2 is a hard crash on release builds.
How did you verify your code works?
Two tests in
test/js/node/process/process.test.js:defers a builder failure report until after the property lookup: fails on the unfixed build on every platform (the handler observesbefore,uncaught:TypeError,value:undefined,after; fixed order isbefore,value:undefined,after,uncaught:TypeError), passes with the fix.survives a clobbered global breaking the env builder mid-walk: on an unfixed Windows debug build this aborts with theLazyProperty::setassertion above; with only the env-initializer fix applied it aborts with thestoredPrototypeassertion (the deferred report is what removes the mid-walk JS); with both fixes it exits cleanly. Verified on Windows x64 (debug) and Linux x64 (debug ASAN).Also ran
test/js/node/process/process.test.js,process-nexttick,process-stdio,process-stdin,run-process-env,worker-transfer-terminate-stress(the scenario that requires builder exceptions to be cleared), andworker-terminate-lifetimeon the debug build: no new failures (the pre-existingUSER-unset and DNS-teardown failures in this container reproduce identically without this diff).no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/process/process.test.js