Error.appendStackTrace: don't abort when stackTraceLimit is unset - #35100
Error.appendStackTrace: don't abort when stackTraceLimit is unset#35100robobun wants to merge 3 commits into
Conversation
ErrorInstance::captureStackTrace calls globalObject->stackTraceLimit().value() unconditionally, which aborts when the optional is empty. That state is reachable from JS by assigning a non-number to Error.stackTraceLimit (or deleting it), after which errors carry no native stack trace. Error.appendStackTrace then falls into the captureStackTrace path for the destination and the process dies with SIGABRT. Guard the capture on stackTraceLimit() having a value, and guard the append on the destination actually having a vector so skipping the capture does not turn into a null deref.
Walkthrough
ChangesError stack trace handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:54 AM PT - Jul 22nd, 2026
❌ @robobun, your commit c9787de has 4 failures in
🧪 To try this PR locally: bunx bun-pr 35100That installs a local version of the PR into your bun-35100 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Create the second source error while the limit is set so it carries frames, then unset the limit before creating the destination. That reaches the appendVector call with a null destination vector, which is what the second guard protects against.
|
Added coverage for the second guard in 7b64c89: the |
There was a problem hiding this comment.
LGTM — the follow-up commit addresses the earlier feedback: the c/d case now creates the source with a real limit and the destination after deleting it, so both added guards are exercised.
What was reviewed:
- Traced both guards against the updated fixture — scenario 1 hits the
stackTraceLimit()guard, scenario 2 hits thedestination->stackTrace()guard. - Checked for sibling
ErrorInstance::captureStackTracecallers insrc/that could hit the same.value()abort — this is the only one. - Test shape: subprocess with concurrent pipe drain, asserts
signalCode: nulland exact stdout, matches file conventions.
Extended reasoning...
Overview
Two-line change to errorConstructorFuncAppendStackTrace in src/jsc/bindings/FormatStackTraceForJS.cpp plus a subprocess regression test in test/js/node/v8/capture-stack-trace.test.js. The fix guards destination->captureStackTrace(...) on globalObject->stackTraceLimit() having a value (preventing the bad_optional_access abort inside ErrorInstance::captureStackTrace), and guards the subsequent appendVector on the destination having a stack-trace vector (preventing a null deref when the capture was skipped but the source still has frames).
Security risks
None. Error.appendStackTrace is a Bun-specific API on the Error constructor; the change strictly narrows two conditionals to avoid crashing on already-reachable user input. No new data flow, no allocation, no user-callback surface.
Level of scrutiny
Low-to-medium. This is a targeted fuzzer-found crash fix in a small, self-contained host function. The behavioral change is that appending becomes a no-op when stackTraceLimit is unset (instead of aborting the process), which is strictly an improvement — errors created under an unset limit have no native trace anyway, so there is nothing meaningful to append.
Other factors
- I previously flagged that the original test never reached the second guard's right operand. Commit 7b64c89 restructured the second scenario so
cis created withstackTraceLimit = 10(has frames) anddafterdelete Error.stackTraceLimit(no frames, capture skipped), which the author verified segfaults on a build with only the first guard applied. Both load-bearing clauses are now covered. - Grepped
src/for otherErrorInstance::captureStackTracecall sites and unguardedstackTraceLimit().value()— this is the only in-tree caller, so no sibling sites need the same guard. - The test follows the file's existing subprocess pattern (
Bun.spawnwith-e,bunEnv, concurrentPromise.allpipe drain, combined-object assertion includingsignalCode: null, exit code asserted last). - The gate evidence in the PR description shows the test failing on unpatched debug/release builds and passing with the fix.
|
CI failures across builds #77665 and #77769 are unrelated to this change:
The added regression test ( |
|
Consolidated into #37370, which fixes the same |
1 participant
Found by Fuzzilli (fingerprint
cb5f2d970ad4162c).Assigning a non-number to
Error.stackTraceLimit(or deleting it) clearsJSGlobalObject::m_stackTraceLimittostd::nullopt, and errors created after that point carry no native stack trace.Error.appendStackTracethen took the "destination has no trace" path and calledErrorInstance::captureStackTrace, which doesglobalObject->stackTraceLimit().value()unconditionally; with no value that's abad_optional_accessthat aborts the process (JSC is built without exceptions).Fix: only call
captureStackTracewhenstackTraceLimit()has a value, and only append when the destination actually has a vector (so skipping the capture does not become a null deref when the source still has frames).Related open PRs #32098 and #34713 touch the same function for the self-append and already-materialized cases respectively; this one is orthogonal (still crashes with distinct source/destination and neither materialized).
[stamp-90s] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 2
evidence per changed file