ErrorInstance: keep a lock that the error info callback adds to the error - #644
ErrorInstance: keep a lock that the error info callback adds to the error#644robobun wants to merge 2 commits into
Conversation
…rror materializeErrorInfoIfNeeded() calls VM::onComputeErrorInfoJSValue() and then stores "line", "column", "sourceURL" and "stack" with putDirect(). In Bun the callback runs Error.prepareStackTrace, so user code runs between the two steps. putDirect() checks nothing. It added the properties to an error that the callback froze, and it replaced a "stack" that the callback made non-configurable. Skip each write that an ordinary [[DefineOwnProperty]] would reject: the property exists and is non-configurable, or the property is absent and the error became non-extensible while the callback ran. An error that is non-extensible before the callback still gets the properties, because they count as present from creation.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials 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. WalkthroughBun-specific lazy Error property materialization now records extensibility before the stack-preparation callback. It skips writes to restricted properties when the callback freezes the Error or makes properties non-configurable. ChangesError property materialization
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The guarded materialization behavior preserves callback-defined restricted properties, with no remaining actionable merge risk identified. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives a detailed problem statement, fix, behavior changes, testing results, and implementation context. It does not provide the required Bugzilla bug link or the required reviewed-by line and changed-file/function list from the repository template. Resolution Add the associated Bugzilla URL, include a "Reviewed by NOBODY (OOPS!)." line or actual reviewer, and add the required changed-file and function list. Ensure the pull request is linked to the Bugzilla bug and uses the required repository metadata.
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
|
The preview build carries the ErrorInstance::materializeErrorInfoIfNeeded change that the new tests in capture-stack-trace.test.js cover. Swap the pin for the merged sha before this lands.
…alue A property that the callback made non-configurable keeps its attributes. If it is a plain writable data property, as "stack" is after Object.seal(), it still takes the value, because an assignment could store it. Only a read-only property and an accessor are skipped. A sealed error then has the result of Error.prepareStackTrace as its "stack", as in Node.
There was a problem hiding this comment.
Thanks — commit 6024d7f addresses the seal case I flagged: a DontDelete-but-writable slot now takes the callback's value via putDirect with currentAttributes, so isSealed holds and stack matches Node. I didn't find further issues; given this sits on the object-invariant path a maintainer glance is still worthwhile.
What was reviewed:
putUnlessLockedagainstObject.freeze/seal/preventExtensionsand non-configurable accessor/data redefinitions — each now respects the existing descriptor.becameNonExtensiblegating only newly-absent properties, so pre-existing-non-extensible errors still receive the lazy four as before.ReadOnlyOrAccessorOrCustomAccessorOrValueis the existing composite in PropertySlot.h;structure()->get+putDirectmirrors the pattern used elsewhere in runtime/.
Extended reasoning...
Overview
The PR modifies a single function, ErrorInstance::materializeErrorInfoIfNeeded() in Source/JavaScriptCore/runtime/ErrorInstance.cpp, entirely inside the USE(BUN_JSC_ADDITIONS) block. It replaces four unconditional putDirect() calls (for line, column, sourceURL, stack) with a putUnlessLocked lambda that checks the current structure entry and the object's extensibility before writing, so that user code run via Error.prepareStackTrace cannot have its integrity-level changes (freeze/seal/preventExtensions, non-configurable defines) silently overwritten.
Security risks
None identified. The change is strictly more conservative than the base: where the old code unconditionally wrote and could violate [[DefineOwnProperty]] invariants (writable/configurable properties reappearing on a frozen object), the new code either skips the write or preserves existing attributes. There is no new allocation, no new user-code reentry point, and the lookup uses the standard structure()->get(vm, name, attrs) path. The worst failure mode is a lazy property not being populated in an edge case, which is a compat nit rather than a memory-safety or invariant-violation risk.
Level of scrutiny
Moderate. This is JSC runtime code on the object-model path, where mistakes around property attributes can produce spec-observable invariant violations or confuse inline caches. That said, the change is ~20 lines, localized to a Bun-only lazy-materialization slow path, and only narrows when writes happen. The first revision skipped writes on DontDelete alone, regressing the seal case versus Node; the follow-up commit now writes the value with currentAttributes when the slot is non-configurable but still a writable data property, which was the fix I suggested. I re-read the updated lambda against freeze (ReadOnly|DontDelete → skip), seal (DontDelete only → write with preserved attrs), preventExtensions-during-callback (absent + becameNonExtensible → skip), non-configurable accessor (Accessor|DontDelete → skip), and the pre-existing-non-extensible case (wasExtensible false → becameNonExtensible false → still adds), and each behaves as described.
Other factors
Tests live in the companion Bun PR (test/js/node/v8/capture-stack-trace.test.js) rather than JSTests, since the jsc shell doesn't install the callback — so a maintainer should confirm those pass against this build. The PR description's results table still lists the seal row as "the default string", which appears stale relative to the new code (seal should now yield "P"); worth a quick sanity check by whoever merges. The acknowledged corner (already-non-extensible error that the callback then freezes) remains, but is documented and is no worse than base behavior.
…t, define or delete keeps the lock Follows oven-sh/WebKit#644 at 6024d7f151: a non-configurable property keeps its attributes and takes the value only if it is writable. Adds the read after Error.captureStackTrace on a still-lazy stack, and the setter-not-called check for a locked accessor.
Draft only. Swap for the merged sha before landing.
Problem
ErrorInstance::materializeErrorInfoIfNeeded(ErrorInstance.cpp:432, theUSE(BUN_JSC_ADDITIONS)branch) callsVM::onComputeErrorInfoJSValue()and then storesline,column,sourceURLandstackwithputDirect(). In Bun the callback runsError.prepareStackTrace, so user code runs between the two steps.putDirect()checks nothing. An error that the callback froze gains writable properties, and astackthat the callback made non-configurable is replaced. Node keeps both locks.Error.prepareStackTrace = e => { Object.freeze(e); return "P" }; const a = new Error("a"); a.stack; Object.isFrozen(a)isfalsein Bun andtruein Node.Fix
autobuild-preview-pr-644-6024d7f1(55 pass intest/js/node/v8/capture-stack-trace.test.js). Bun 1.4.3 fails 8 of the 9 new tests.Background
ErrorInstancecreates the four properties on the first lookup of one of them.m_errorInfoMaterializedis set before the callback, so a lookup from inside the callback does not start over.putDirect()is the internal store of JSC. It adds to a non-extensible object and replaces the attributes of a property that exists.m_stackStringbranch below runs no user code before its writes. It is unchanged.Notes
Decision for a maintainer. No user reported this. It was found by audit while working on oven-sh/bun#33412, which is the same class on the
Error.captureStackTracepath and is also open. The rule that the two PRs follow together: a read ofstacknever throws and skips a write that a lock forbids, and an explicitError.captureStackTraceon a locked target throws the TypeError of V8. If Bun's stack machinery is not meant to honor integrity levels, both PRs close.Results with
Error.prepareStackTrace = e => { lock(e); return "P" }, thene.stack, on Bun linked against this change:lockstackafterObject.freezeisFrozenistrueisFrozenistrue,stackis"P"Object.seal"P", still non-configurableisSealedistrueObject.preventExtensions"P"stackis"P"defineProperty(e, "stack", { value: "LOCKED", writable: false, configurable: false })"LOCKED", attributes keptdefineProperty(e, "stack", { value: "LOCKED", writable: true, configurable: false })"P", attributes kept"LOCKED", attributes keptdefineProperty(e, "stack", { get, set, configurable: false })setis not calleddefineProperty(e, "line", { value: -1, configurable: false })"P"linekeeps-1linein V8stackthat the callback made read-only, Node returns the result of the callback. V8 can do that because itsstackis an accessor over an internal slot. A data property cannot take a new value once it is read-only and non-configurable, so the string that the callback froze stays. To match Node there,stackmust become an accessor over a private slot. That is a larger change and nothing asks for it today."P", as Bun does today. Only the attributes are now kept. Node keeps the value of the callback's define.stackexists when the callback runs because Bun stores the default string on the error before it callsError.prepareStackTrace, so thaterror.stackinside the callback is a string as in Node.Error.captureStackTrace(error)on an error whosestackis still lazy. The companion tests cover that row.Reflect.setreturnsfalse), because the freeze holds. V8 runs the callback only on a read.stackaccessor that Bun'sError.captureStackTraceinstalls when it captures no frames (errorInstanceLazyStackCustomGetterin Bun'sFormatStackTraceForJS.cpp), and thenode:vmarrow decoration (writeArrowHeaderStackinNodeVM.cpp). Both are Bun-sideputDirect()calls and need no WebKit change.jscshell does not set the callback, so there is no test underJSTests/. The tests are intest/js/node/v8/capture-stack-trace.test.jsof the Bun PR.USE(BUN_JSC_ADDITIONS)block, so it touches no line that exists upstream.