error: never return an empty JSValue from computeErrorInfoWrapperToJSValue - #34104
Conversation
…WrapperToJSValue ErrorInstance::getOwnPropertySlot does not check for an exception after materializeErrorInfoIfNeeded calls vm.onComputeErrorInfoJSValue, so a TerminationException raised inside the hook trips JSObject::getOwnPropertyDescriptor's EXCEPTION_ASSERT(!scope.exception() || !result). Defer termination across the hook so the request is handled at the next trap checkpoint after the slot is filled. Separately, when the hook throws before the default stack string is computed (e.g. a throwing .message getter), it returned an empty JSValue which materializeErrorInfoIfNeeded putDirect()s into the error's stack slot; the next read of e.stack then dereferences JSValue() and segfaults. Fall back to jsUndefined() so the stored value is always valid. Refs #34095
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Related but not a duplicate: #30823 uses |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The deferral wrapped the profiledCall into user Error.prepareStackTrace, so a worker with an infinite loop there could no longer be interrupted by worker.terminate(). The termination case for ErrorInstance materialization needs a WebKit-side RETURN_IF_EXCEPTION in getOwnPropertySlot instead; only the empty-return guard remains here.
robobun
changed the title
There was a problem hiding this comment.
LGTM — the DeferTerminationForAWhile from my earlier review was dropped in 3dd57f6; what remains is the 2-line guard swapping an empty JSValue for jsUndefined() before it reaches putDirect.
Checked: the guard only fires on the previously-crashing path (empty result when the hook throws); the pending exception from the .message getter still propagates unchanged. The new subprocess test asserts both the throw on the first .stack read and typeof e.stack === "undefined" (not a crash) on the second.
Nit: PR title/description still describe the dropped DeferTerminationForAWhile half.
Extended reasoning...
Overview
After 3dd57f6 addressed my earlier feedback (removing DeferTerminationForAWhile, which would have made a hanging prepareStackTrace uninterruptible by worker.terminate()), the PR is now a 4-line change to computeErrorInfoWrapperToJSValue in src/jsc/bindings/FormatStackTraceForJS.cpp plus a ~24-line spawned regression test in test/js/node/v8/capture-stack-trace.test.js.
The C++ change: when computeErrorInfoToJSValue returns {} (via any of its RETURN_IF_EXCEPTION(scope, {}) paths — e.g. a throwing .message getter while Error.prepareStackTrace is set), return jsUndefined() instead. ErrorInstance::materializeErrorInfoIfNeeded unconditionally putDirects this hook's return value into the stack slot, and an empty JSValue there segfaults the next property read.
Security risks
None. This is a defensive null-guard on an internal hook return value. It does not touch input parsing, auth, crypto, or resource limits. The only observable behavior change is that a scenario which previously crashed the process now stores undefined in .stack while the original exception continues to propagate.
Level of scrutiny
Low-to-moderate. JSC bindings are a sensitive area, but this specific change is a pure guard: the only input state it affects (result is empty) was previously undefined behavior (crash). It cannot alter the non-empty path. The exception state is untouched — the throw still surfaces to the caller, as the test verifies (first === "msg-boom") and as the existing throw inside Error.prepareStackTrace doesnt crash test at line 22 continues to cover.
Other factors
- My prior inline concern is resolved and marked as such in the thread.
- The gate evidence shows the test fails (segfault/UBSan) on ASAN without the fix and passes on both ASAN and release with it; the full 41-test file passes.
- The test follows harness conventions:
bunExe()/bunEnv,Bun.spawnwith-e,Promise.alldraining stdout/stderr/exited concurrently, asserts output/signalCodebeforeexitCode. - The PR title and first section of the description still describe the dropped
DeferTerminationForAWhilechange; that's cosmetic and the timeline documents the reduction, so not blocking. - Overlaps with #30823 (different approach,
tryClearException) — a maintainer will want to reconcile, but this change is strictly a crash fix and safe to land independently.
Reading the property can run a user getter. Before #34104 a pending exception here meant computeErrorInfoToJSValue returned an empty JSValue, which materializeErrorInfoIfNeeded putDirect'd into .stack and the process died on a null JSCell deref (UBSan: "member call on null pointer of type JSC::JSCell"). computeErrorInfoWrapperToJSValue now maps an empty return to jsUndefined(), so propagate and let the wrapper handle it, which is what Node does and what the existing #34095 test right above expects for the .message-throws variant of the same shape.
2 participants
computeErrorInfoWrapperToJSValueis Bun'svm.onComputeErrorInfoJSValuehook, called fromErrorInstance::materializeErrorInfoIfNeededwhen a lazy error property (stack/line/column/sourceURL) is first read. When the hook throws before the default stack string has been computed (e.g. a throwing.messagegetter whileError.prepareStackTraceis set),computeErrorInfoToJSValuereturns{}.materializeErrorInfoIfNeededthenputDirects that empty value into the error'sstackslot, and the next read segfaults:Fall back to
jsUndefined()so the stored value is always valid. The.messagethrow still propagates to the caller.About the
getOwnPropertyDescriptorassertion in #34095ErrorInstance::getOwnPropertySlotin WebKit does not check for an exception aftermaterializeErrorInfoIfNeeded(unlike its siblingsdefineOwnProperty/put, which do). When the hook leaves one pending,JSObject::getOwnPropertyDescriptortrips:An earlier revision of this PR wrapped the hook in
DeferTerminationForAWhileto keep aTerminationExceptionfrom reaching that assertion, but that scope would have covered theprofiledCallinto userError.prepareStackTrace, making an infinite loop there uninterruptible byworker.terminate(). Dropped in 3dd57f6. A proper fix for the termination case is aRETURN_IF_EXCEPTIONinErrorInstance::getOwnPropertySloton the WebKit side.Related
DeferTerminationForAWhileto theprocess.*lazy property builders (bounded C++ initializers, no unbounded user JS), which is the pathtest-worker-message-port-transfer-terminate.jsactually hits during worker bootstrap.tryClearException, which covers a throwingError.prepareStackTracereachinggetOwnPropertyDescriptoron debug/ASAN, at the cost ofe.stackno longer propagating the throw.Verification
New test in
test/js/node/v8/capture-stack-trace.test.jsspawns the.message-throws repro and asserts the first.stackread throwsmsg-boomand a subsequent read returnsundefinedinstead of crashing. Segfaults on the unfixed build, passes with this change. The full test file (41 tests, including the existinge.stack-throws andprepareStackTrace-propagation tests) passes.Refs #34095
[stamp-90s] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file