addErrorInfo: keep a stack hook exception from escaping ParserError::toErrorObject - #535
addErrorInfo: keep a stack hook exception from escaping ParserError::toErrorObject#535robobun wants to merge 1 commit into
Conversation
…toErrorObject Bun's VM::onComputeErrorInfoJSValue hook runs a user Error.prepareStackTrace when addErrorInfo materializes the error info of a new parse error. Callers of ParserError::toErrorObject (constructFunctionSkippingEvalEnabledCheck, ModuleProgramExecutable::getUnlinkedCodeBlock, ScriptExecutable::newCodeBlockFor) treat it as non-throwing and throw the returned error without an exception check. With validateExceptionChecks=1 that aborts on every `new Function` whose source has a syntax error. Materialize under a TopExceptionScope and clear a hook exception there. The parse error is what gets thrown. A termination exception stays pending.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. Because it changes exception-scope handling in the JSC runtime error path (CODEOWNERS routes Source/JavaScriptCore to jsc-reviewers) and the author notes it wasn't built locally, a human familiar with the exception-scope validator should still take a look.
What was reviewed:
#if/#elserestructure — the non-Bun path still just callsmaterializeErrorInfoIfNeeded(vm)and falls through to theputDirectblock, semantically identical to before.DECLARE_TOP_EXCEPTION_SCOPE+clearExceptionExceptTermination()matches the existing pattern atcreateTypeErrorCopy(Error.cpp:293) and the definition in TopExceptionScope.h; termination stays pending as described.- Bun path still returns
errorInstanceearly, so the trailingputDirect(line/sourceURL)remains skipped as before.
Extended reasoning...
Overview
The PR modifies a single function, addErrorInfo(VM&, JSObject*, int, const SourceCode&) in Source/JavaScriptCore/runtime/Error.cpp. Inside the USE(BUN_JSC_ADDITIONS) branch it now wraps errorInstance->materializeErrorInfoIfNeeded(vm) in a DECLARE_TOP_EXCEPTION_SCOPE and calls scope.clearExceptionExceptTermination() afterward, so that an exception thrown by Bun's Error.prepareStackTrace hook (via VM::onComputeErrorInfoJSValue) is swallowed and ParserError::toErrorObject() remains effectively non-throwing for its callers. The #if/#endif blocks are collapsed into a single #if/#else/#endif; the #else arm preserves the original unguarded materializeErrorInfoIfNeeded call for non-Bun builds.
Security risks
None identified. The change does not introduce new inputs, parsing, allocation, or trust boundaries. It clears a user-hook exception rather than propagating it, which is a hardening/correctness fix under validateExceptionChecks=1. clearExceptionExceptTermination() explicitly preserves a pending termination exception (verified in TopExceptionScope.h), so termination semantics are unchanged.
Level of scrutiny
Moderate-to-high. The diff is small and follows an established idiom already used a few lines below in createTypeErrorCopy, and I confirmed the non-Bun preprocessor path is byte-for-byte equivalent in effect. However, JSC's exception-scope validator has subtle destructor/needExceptionCheck rules, Source/JavaScriptCore is CODEOWNER-routed to jsc-reviewers, and the author states they could not build JSC locally and are relying on CI. Those factors together warrant a human reviewer with JSC exception-scope experience signing off rather than an auto-approval.
Other factors
The TopExceptionScope header is already used in this translation unit, so no new include is needed. The Bun-side test lives in the companion oven-sh/bun PR per the description; there is no test change in this repo. Release-build behavior is claimed unchanged (the hook exception was previously overwritten by the subsequent throwException), which is consistent with what I read, but I could not execute a build here to confirm the validator is satisfied.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughChangesError info handling
Merge Risk: ⚪ Minimal · up to This localized change prevents stack-trace hook exceptions from escaping while preserving the original parse error; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description clearly explains the problem, cause, fix, affected callers, behavior, and verification status. It does not include the required Bugzilla link or explicit changed-file summary, but the technical content is substantially complete. Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
Preview Builds
|
|
Bun side: oven-sh/bun#40866 pins the preview build |
1 participant
Problem
validateExceptionChecks=1, anynew Function(source)whose source has a syntax error aborts the process:GeneratorFunction,AsyncFunctionand Bun'svm.SourceTextModulehit the same abort.eval("{")does not: an eval parse error isParserError::EvalError, which never callsaddErrorInfo.ParserError::toErrorObject()callsaddErrorInfo()(Error.cpp:243), which materializes the error info at once. In the Bun fork that runsVM::onComputeErrorInfoJSValue, and Bun's hook calls a userError.prepareStackTrace, so it declares aThrowScopeand can throw. Upstream'scomputeErrorInfocannot throw, so everytoErrorObject()caller throws the returned error with no exception check in between (FunctionConstructor.cpp:226,ModuleProgramExecutable.cpp:68,ScriptExecutable.cpp:320,UnlinkedFunctionExecutable.cpp:238).Fix
addErrorInfo()materializes under aTopExceptionScopeand clears a hook exception there.clearExceptionExceptTermination()leaves a termination pending, andVM::throwExceptionkeeps a pending termination over the parse error.toErrorObject()non-throwing, which is the contract all of its callers rely on. The parse error is what gets thrown. That is what Node does too: V8 does not runprepareStackTracewhile it builds aSyntaxError.VM::throwException(parseError)replaced it. Bun'snode:vmcallers oftoErrorObject()already clear that exception by hand (NodeVM.cpp, NodeVMScript.cpp); this covers the callers inside JSC.ThrowScope.cppandTopExceptionScope.cpp(aTopExceptionScopedestructor does not simulate a throw, so the caller'sthrowExceptionsees no pending check). I could not build JSC in this environment, so CI is the build check. The Bun side is oven-sh/bun (test intest/js/bun/jsc/exception-checks.test.ts), which pins the preview build of this PR.Background
validateExceptionChecks=1makes everyThrowScopedestructor markVM::m_needExceptionCheck. The nextThrowScopeconstructor, or athrowExceptionof a non-Exceptionvalue, asserts if the bit is still set.VM::exception()andclearException()clear it.TopExceptionScope(the oldCatchScope) only verifies in its destructor. Code that must not propagate an exception uses it, ascreateTypeErrorCopyin the same file does.ErrorInstancecomputesstack,line,columnandsourceURLlazily.materializeErrorInfoIfNeededbuilds them from the captured frames. The Bun fork lets the embedder do that throughVM::onComputeErrorInfoJSValue, which is whereError.prepareStackTraceruns.