node:util: fix isError crash on revoked proxies and getPrototypeOf traps - #36913
Conversation
Walkthrough
Changesutil.isError behavior
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: ready to merge from my side; needs a maintainer. This PR now also replaces #37202 (closed), which fixed the same two problems in the same function. Reproduced on bun 1.4.0 canary (8326d1b, Linux x64), all three inputs segfault ( const r = Proxy.revocable({}, {}); r.revoke();
require("node:util").isError(r.proxy);
require("node:util").isError(new Proxy({}, { getPrototypeOf() { throw 1 } }));
require("node:util").isError(new Proxy({}, { getPrototypeOf: () => Error.prototype }));The third one is the Test: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the two-part fix (scope the VMInquiry PropertySlot so its DisallowVMEntry is gone before getPrototype() can run a Proxy trap, and RETURN_IF_EXCEPTION after getPrototype()) is minimal and correct. One inline nit on test isolation, non-blocking.
Extended reasoning...
Overview
Fixes a segfault in util.isError() when passed a revoked Proxy or a Proxy with a getPrototypeOf trap. Two mechanical changes in jsFunctionIsError (src/jsc/modules/NodeUtilTypesModule.cpp): (1) wrap the VMInquiry PropertySlot in a block scope so its DisallowVMEntry guard is destroyed before getPrototype() — which may enter the VM via a Proxy trap — runs; (2) add RETURN_IF_EXCEPTION after getPrototype(), since a throwing trap or revoked proxy returns an empty JSValue whose isCell() is true, leading to a null deref in inherits<>. Three new tests in test/js/node/util/util.test.js cover revoked proxy, throwing trap, and benign traps (null / Error.prototype / proxy-over-Error).
Security risks
None. This is a crash fix in a deprecated Node-compat type predicate; no auth/crypto/permissions/parsing of untrusted structured data. The change strictly narrows the failure surface (crash → propagated exception).
Level of scrutiny
Native C++ JSC exception handling warrants care, but both edits are textbook applications of REVIEW.md's own rules: "Exception checks after every call that can enter JS" and destroying a DisallowVMEntry-holding slot before a call that must enter the VM. I verified:
- The only other
getPrototype()call in this file (jsFunctionIsAsyncFunction) operates on adynamicDowncast<JSFunction>, whose ordinary[[GetPrototypeOf]]cannot throw — no sibling bug. scope.assertNoException()after the VMInquirygetPropertySlotremains valid: VMInquiry lookups do not invoke Proxy traps, so a revoked proxy still doesn't throw there.- The scoped block preserves all early-return semantics (the
Symbol.toStringTag == "Error"fast-true path still returns before falling through togetPrototype).
Other factors
The one nit filed (crash repros should be spawned fixtures per REVIEW.md so a regression fails one test rather than segfaulting the whole 211-test file) is test hygiene only — the fix itself is correct, and a regression would still be caught, just less cleanly. Node semantics match: legacy util.isError uses instanceof Error, which invokes and propagates the getPrototypeOf trap, so throwing rather than returning false is the right behavior.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the spawned-fixture test and disallowVMEntry.reset() idiom address both prior review notes.
What was reviewed:
RETURN_IF_EXCEPTIONaftergetPrototype()covers both throwing traps and revoked proxies;disallowVMEntry.reset()is placed before theslot.isValue()read, which is safe since a value slot doesn't re-enter the VM.- Checked
jsFunctionIsAsyncFunction's unguardedgetPrototype()in the same file — its receiver is a downcastJSFunction, not a Proxy, so it can't throw and isn't a sibling bug. - Test asserts exact per-case output (revoked → TypeError, throwing trap propagates, null/Error.prototype traps honored, proxy-of-Error → true) matching Node's
instanceof Errorsemantics;stderr: "inherit"avoids pipe-buffer concerns while surfacing crash output on regression.
Extended reasoning...
Overview
Three-line C++ fix in jsFunctionIsError (src/jsc/modules/NodeUtilTypesModule.cpp): reset the VMInquiry slot's DisallowVMEntry after the Symbol.toStringTag lookup so the subsequent getPrototype() may invoke a Proxy's getPrototypeOf trap, and add RETURN_IF_EXCEPTION after getPrototype() so a throwing/revoked-proxy trap propagates instead of producing an empty JSValue that the next line dereferences. A spawned-fixture test in test/js/node/util/util.test.js covers five proxy shapes.
Security risks
None. The change tightens exception handling on a deprecated introspection helper; no new user-controlled data reaches native parsing, no allocation, no privilege boundary.
Level of scrutiny
Moderate — native JSC binding code with exception-scope discipline, but the diff is minimal and mechanical. Both added lines are textbook JSC patterns: slot.disallowVMEntry.reset() is the documented way to release a VMInquiry slot's guard before code that may enter the VM, and RETURN_IF_EXCEPTION after a call that can invoke user JS is required by REVIEW.md's memory-safety rules. I confirmed the reset placement is safe: the following slot.getValue() is gated by slot.isValue(), which returns a cached value without VM entry.
Other factors
My earlier review asked for a spawned fixture; d4d1203 moved all five cases into a child process with exact stdout assertions before the exit-code check, so a regression fails this one test cleanly instead of killing the file. The comment-cop note about the paragraph-long comment was addressed in d551170 by switching from a block-scope-plus-comment to the one-line .reset() idiom. I checked the neighboring jsFunctionIsAsyncFunction for the same missing exception check — its getPrototype() receiver is a verified JSFunction, so no trap can fire there. The asserted revoked-proxy message is JSC's literal string and could change on a WebKit bump, but that's consistent with how the suite handles engine messages elsewhere. No outstanding review threads; bug-hunting pass found nothing.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/util/util.test.js`:
- Around line 179-181: Update the child-process setup to use stderr: "pipe",
then await proc.stderr.text() alongside proc.stdout.text() and proc.exited in
the existing Promise.all. Capture all three returned values and include the
stderr output in the asserted result.
🪄 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: aab14221-0aac-4d75-af30-0fa14ecf1b3e
📒 Files selected for processing (2)
src/jsc/modules/NodeUtilTypesModule.cpptest/js/node/util/util.test.js
There was a problem hiding this comment.
LGTM — the earlier feedback (spawned fixture, comment-cop, stderr piping) is all addressed and the fix is a minimal, idiomatic exception-check + DisallowVMEntry release.
What was reviewed:
slot.disallowVMEntry.reset()placement: safe beforeslot.getValue()sinceslot.isValue()guards to the cached-value path (no VM entry).RETURN_IF_EXCEPTIONaftergetPrototype(): correct; the siblinggetPrototypeinjsFunctionIsAsyncFunctionoperates on a downcastJSFunction*(never a Proxy), so no matching fix needed there.- Test: spawned
-efixture, both pipes drained concurrently, exact per-case stdout assertions before exit code — matches harness conventions.
Extended reasoning...
Overview
Two-file change: three added lines in src/jsc/modules/NodeUtilTypesModule.cpp (slot.disallowVMEntry.reset(), a one-line comment, and RETURN_IF_EXCEPTION(scope, {}) after getPrototype()), plus one new subprocess test in test/js/node/util/util.test.js covering five Proxy variants (revoked, throwing trap, null trap, Error.prototype trap, proxy-of-Error).
Security risks
None. This is a crash fix in a deprecated Node compat helper (util.isError). The change adds an exception check and releases a VM-entry guard — strictly narrowing undefined behavior into a catchable JS exception. No new user-controlled data flows.
Level of scrutiny
Medium. It's C++ touching JSC exception-scope discipline, which REVIEW.md calls out as the most-blocked category, so I traced the surrounding calls: toObject on a cell can't throw; getPropertySlot in VMInquiry mode can't enter JS (hence assertNoException() is right); resetting disallowVMEntry before slot.getValue() is fine because the slot.isValue() guard means getValue() reads the cached slot value with no VM entry; and getPrototype() on a Proxy legitimately invokes a JS trap, so both the reset (to permit entry) and the new RETURN_IF_EXCEPTION (to propagate the trap's throw) are required. The only other getPrototype in this file is in jsFunctionIsAsyncFunction, gated by dynamicDowncast<JSFunction>, which a ProxyObject cannot satisfy — so that site can't hit a trap and doesn't need the same fix.
Other factors
All three prior review threads are resolved: my spawned-fixture nit (d4d1203), comment-cop's long-comment flag (d551170 replaced the block-scope-plus-comment with the .reset() idiom JSC itself uses), and CodeRabbit's stderr-drain note (468cbbf). The test now follows CLAUDE.md's subprocess pattern exactly — bunEnv/bunExe, await using, concurrent Promise.all over stdout/stderr/exited, exact .toBe assertions, exit code asserted last. The exact JSC TypeError message is asserted, which is intentional per REVIEW.md's "assert the strongest invariant" guidance.
There was a problem hiding this comment.
LGTM — targeted crash fix with a spawned regression test.
What was reviewed:
slot.disallowVMEntry.reset()placement: guarded byslot.isValue(), so the subsequentgetValuereads a cached value and needs no VM entry; resetting before it is safe.RETURN_IF_EXCEPTIONaftergetPrototypecovers both the revoked-proxy and throwing-trap paths; propagating matches Node'sinstanceof Errorsemantics.- Sibling
getPrototypeinjsFunctionIsAsyncFunctionis on a downcastJSFunction(not a Proxy), so not the same bug class. - Test now runs the crashing inputs in a child, drains stdout+stderr concurrently, and asserts exact per-case output — prior review feedback addressed.
Extended reasoning...
Overview
Two-file change fixing a segfault in util.isError() when passed a revoked Proxy or a Proxy with a getPrototypeOf trap. Production change is 3 lines in src/jsc/modules/NodeUtilTypesModule.cpp: (1) slot.disallowVMEntry.reset() after the VMInquiry Symbol.toStringTag lookup so the later getPrototype() may re-enter the VM to run a Proxy trap, and (2) RETURN_IF_EXCEPTION(scope, {}) after getPrototype() so a throwing/revoked trap propagates instead of falling through to a null-cell deref. The test file gains one subprocess-based regression test covering five proxy variants.
Security risks
None. This closes a null-pointer deref (crash-only, not exploitable for anything beyond DoS on adversarial input to a deprecated API). No new attack surface is introduced; the change tightens exception handling.
Level of scrutiny
Low-to-medium. The C++ change is minimal and mechanical: adding a missing exception check after a call documented to throw, and releasing a DisallowVMEntry guard using the exact idiom JSC itself uses (JSScope.cpp, JSGlobalObject.cpp). The reset() is placed before slot.getValue(), but that call is gated on slot.isValue() so it returns a cached value without VM entry — the ordering is safe. I checked the neighboring jsFunctionIsAsyncFunction's getPrototype call for the same class of bug; it operates on a dynamicDowncast<JSFunction> result which cannot be a Proxy, so it is not affected.
Other factors
All three prior review threads are resolved: my earlier request to move the crashing inputs into a spawned fixture (done in d4d1203), the comment-cop objection to a paragraph-long justifying comment (replaced with the one-line reset() idiom in d551170), and CodeRabbit's request to pipe/drain stderr (done in 468cbbf). The test asserts stderr is empty, then exact stdout (one line per case: revoked → TypeError, throwing trap → propagated, null trap → false, Error.prototype trap → true, proxy-of-Error → true), then exitCode === 0 — the ordering surfaces the child's crash output on regression. The PR's evidence block shows the test failing on both ASAN-debug and release builds without the fix and passing with it, satisfying REVIEW.md's fails-for-the-right-reason requirement.
2ddcc7a to
2897716
Compare
|
Updated 12:50 PM PT - Aug 17th, 2026
✅ @robobun, your commit 871f486d721c5678f612d3b363b2446ded06fc38 passed in 🧪 To try this PR locally: bunx bun-pr 36913That installs a local version of the PR into your bun-36913 --bun |
There was a problem hiding this comment.
LGTM — the earlier feedback (spawned fixture, drop the block-scope comment for slot.disallowVMEntry.reset(), pipe+assert stderr) is all addressed.
Checked: reset() is placed after the lookup and before slot.getValue, which is fine since slot.isValue() guards it to a cached-value read with no VM entry. Also scanned other VMInquiry sites in src/ for the same VM-entry-while-slot-alive pattern — none call getPrototype (or another trap-invoking op) with the slot live. jsFunctionIsAsyncFunction's getPrototype call is on a JSFunction* (proxies filtered by dynamicDowncast), so it's not affected.
Extended reasoning...
Overview
Three-line C++ fix in jsFunctionIsError (src/jsc/modules/NodeUtilTypesModule.cpp): reset the VMInquiry slot's DisallowVMEntry after the Symbol.toStringTag lookup, and add RETURN_IF_EXCEPTION after object->getPrototype(globalObject) (which can invoke a Proxy trap and throw). One new subprocess test in test/js/node/util/util.test.js covering revoked proxy, throwing trap, null-returning trap, Error.prototype-returning trap, and a proxy wrapping an Error.
Security risks
None. This is a crash fix in a deprecated Node compat helper (util.isError); no auth, crypto, or filesystem paths touched. The change strictly narrows the failure mode from a segfault to a propagated JS exception.
Level of scrutiny
Low-to-medium. The native change is a textbook missing-RETURN_IF_EXCEPTION plus the standard JSC idiom for releasing a VMInquiry slot's DisallowVMEntry (cited as matching JSScope.cpp/JSGlobalObject.cpp). Placing the reset before the if (has) { slot.getValue(...) } block is safe because slot.isValue() gates that path to a plain cached-value return. The test follows repo conventions exactly: await using + Bun.spawn, concurrent drain of stdout/stderr/exited, stderr asserted first, exact stdout, exit code last, bunEnv/bunExe.
Other factors
I previously asked that the crashing inputs run in a spawned fixture — done. The comment-cop feedback (paragraph comment on a block scope) was addressed by switching to slot.disallowVMEntry.reset(). CodeRabbit's stderr-pipe feedback was addressed. All inline threads are resolved. The PR body includes gate evidence showing the new test fails under ASAN without the fix (null-cell UBSan report) and passes with it. I checked sibling getPrototype and VMInquiry sites for the same bug class and found none with the same shape.
util.isError segfaulted (null deref at 0x0) when passed a revoked Proxy
or a Proxy whose getPrototypeOf trap throws:
const r = Proxy.revocable({}, {});
r.revoke();
require("node:util").isError(r.proxy);
// panic(main thread): Segmentation fault at address 0x0
Two bugs in jsFunctionIsError:
1. object->getPrototype() can throw (revoked proxy, throwing trap) and
then returns an empty JSValue, which passes isCell(), so the
inherits<ErrorInstance>() check dereferenced a null cell. Add
RETURN_IF_EXCEPTION so the exception propagates like Node's
'e instanceof Error' does.
2. The VMInquiry PropertySlot used for the toStringTag lookup was still
alive at the getPrototype() call. Its DisallowVMEntry member blocks
VM entry, so invoking a getPrototypeOf trap hit
VM::checkVMEntryPermission(): a crash in debug builds, and in release
builds a bogus 'trap should either return an object or null'
TypeError for any proxy with a getPrototypeOf trap. Scope the slot so
it is destroyed before getPrototype() runs.
These inputs segfault an unfixed build, so exercise them as a spawned fixture: a regression now fails this one test with the child's crash output on stderr instead of taking down the whole util.test.js run. The child prints one line per case so the exact semantics (TypeError on a revoked proxy, trap exception propagated, trap results honored) are still asserted.
Use slot.disallowVMEntry.reset() after the lookup (the idiom JSC itself uses in JSScope.cpp and JSGlobalObject.cpp) instead of a commented block scope, so the code reads the same as upstream and needs no explanation.
A regression now fails on the stderr assertion with the child's crash banner in the diff, instead of interleaving it into the runner's output.
2897716 to
cca8419
Compare
There was a problem hiding this comment.
LGTM — the spawned-fixture rework addresses my earlier note, and the two-line native fix is the standard JSC pattern.
What was reviewed:
slot.disallowVMEntry.reset()placement:slot.getValue()afterward is guarded byslot.isValue()so it never needs VM entry; the reset only affects the latergetPrototype()trap call.- Checked other
VMInquiryPropertySlot sites insrc/jsc/for the same live-slot-across-getPrototypeshape — none share it. - Test drains stdout/stderr/exited concurrently, asserts stderr first, exact stdout, then exit code; covers revoked proxy, throwing trap, null trap,
Error.prototypetrap, and proxy-of-Error.
Extended reasoning...
Overview
This PR fixes a segfault in util.isError() when passed a revoked Proxy or a Proxy with a getPrototypeOf trap. The change is 3 lines in src/jsc/modules/NodeUtilTypesModule.cpp (slot.disallowVMEntry.reset() after the VMInquiry lookup, a one-line comment, and RETURN_IF_EXCEPTION after getPrototype()) plus a ~40-line subprocess test in test/js/node/util/util.test.js.
Security risks
None. util.isError is a deprecated read-only type predicate; the change adds an exception check and releases a VM-entry guard earlier. No new attack surface.
Level of scrutiny
Low-to-medium. The native change is two textbook JSC idioms: RETURN_IF_EXCEPTION after a call that can invoke user JS, and disallowVMEntry.reset() to release a VMInquiry slot's guard before re-entering the VM (the PR cites JSC's own JSScope.cpp/JSGlobalObject.cpp for the same pattern). The mechanized gate output in the PR body shows the new test segfaults on unfixed debug+ASAN and release builds and passes on both with the fix, which is exactly the evidence REVIEW.md asks for.
Other factors
All prior review threads are resolved: my earlier request to run the crashing inputs in a spawned child was addressed (the test now uses Bun.spawn with bunExe()/bunEnv, await using, and concurrent pipe draining); the comment-cop paragraph comment was replaced with the direct .reset() idiom; CodeRabbit's stderr-piping request is in. I grepped the other PropertySlot::InternalMethodType::VMInquiry sites in src/jsc/ and none hold a live slot across a subsequent getPrototype() that can trap, so there is no sibling class to fix. jsFunctionIsAsyncFunction's getPrototype() call operates on a verified JSFunction, not a proxy, and has no VMInquiry slot in scope.
…proxy case concurrently
There was a problem hiding this comment.
LGTM — the spawned-fixture and stderr-piping feedback from the earlier passes is all in place now.
What was reviewed:
slot.disallowVMEntry.reset()placement — called beforeslot.getValue(), but that path is gated onslot.isValue()so no VM entry happens there; matches the JSC idiom cited.RETURN_IF_EXCEPTIONaftergetPrototype()— covers both revoked-proxy and throwing-trap paths; propagating matches Node'sinstanceof Errorsemantics.- Test now runs every crashing input in a child, drains stdout/stderr/exited concurrently, asserts stderr → stdout → exitCode in order, and uses
it.concurrent.
Extended reasoning...
Overview
Three-line C++ change to jsFunctionIsError in src/jsc/modules/NodeUtilTypesModule.cpp: reset the VMInquiry slot's DisallowVMEntry guard after the Symbol.toStringTag lookup, and add RETURN_IF_EXCEPTION after object->getPrototype(globalObject). One new subprocess test in test/js/node/util/util.test.js covering revoked proxies, throwing getPrototypeOf traps, non-throwing traps returning null / Error.prototype, and a proxy wrapping a real Error.
Security risks
None. This is a crash fix in a deprecated Node compat helper (util.isError). No new user input reaches native code that wasn't already reaching it; the change only stops dereferencing an empty JSValue and stops holding a DisallowVMEntry across a call that can legitimately enter the VM.
Level of scrutiny
Medium — native C++ touching JSC exception machinery, but the change is minimal and mechanical: one RETURN_IF_EXCEPTION after a call that can throw (textbook REVIEW.md requirement) and one disallowVMEntry.reset() following the exact idiom JSC itself uses. I checked that resetting before slot.getValue() is safe: that branch is gated on slot.isValue(), which returns the stored value without VM entry. The pre-existing scope.assertNoException() after the VMInquiry getPropertySlot is unchanged and the debug-ASAN evidence in the PR confirms it does not fire on the proxy inputs.
Other factors
All three prior review threads (my spawned-fixture request, the comment-cop paragraph-comment note, and CodeRabbit's stderr-piping request) are resolved and reflected in the current diff. The test follows REVIEW.md conventions end to end: crashing inputs run in a spawned child, pipes drained concurrently via Promise.all, assertions ordered stderr → stdout → exitCode, it.concurrent for the independent subprocess. The PR includes gate evidence that the test fails on both release and debug-ASAN without the fix and passes with it, and CI on the current rebase is green for util.test.js on every lane with the two red entries confirmed pre-existing and unrelated.
…error-revoked-proxy
There was a problem hiding this comment.
LGTM — the two-line native fix is the standard JSC idiom for both problems, and my earlier feedback about running the crashing inputs in a spawned fixture has been fully addressed.
What was reviewed:
slot.disallowVMEntry.reset()placement: subsequentslot.isValue()/slot.getValue()on a value slot don't enter the VM, and Proxy under VMInquiry returnshas=falseanyway, so resetting before theif (has)block is safe.RETURN_IF_EXCEPTIONaftergetPrototype()matches Node'sinstanceof Errorsemantics (revoked → TypeError, trap exception propagated, trap result honored).- Test uses
it.concurrent+await using+ concurrent stdout/stderr/exited drain, asserts stderr → stdout → exitCode, and covers all six proxy variants.
Extended reasoning...
Overview
Fixes a segfault in util.isError() when passed a revoked Proxy or a Proxy with a getPrototypeOf trap. Two changes to jsFunctionIsError in src/jsc/modules/NodeUtilTypesModule.cpp: (1) slot.disallowVMEntry.reset() after the VMInquiry Symbol.toStringTag lookup returns, so the later getPrototype() call is permitted to enter the VM to run a Proxy trap; (2) RETURN_IF_EXCEPTION(scope, {}) after getPrototype() so a throwing trap or revoked proxy propagates its exception instead of dereferencing an empty JSValue's null cell. One new subprocess test in test/js/node/util/util.test.js exercises six proxy variants.
Security risks
None. util.isError is a deprecated read-only type check. The change strictly narrows behavior from a null-cell segfault to either a thrown TypeError (revoked proxy), a rethrown trap exception, or the correct boolean — all matching Node's e instanceof Error fallback. No new user-controlled data reaches native code paths that weren't already reachable.
Level of scrutiny
Low-to-moderate. The native change is 3 lines: one comment and two textbook JSC patterns. disallowVMEntry.reset() is exactly the idiom JSC uses when a VMInquiry slot must be followed by code that may run JS (cited in the PR: JSScope.cpp, JSGlobalObject.cpp). RETURN_IF_EXCEPTION after a call that can throw is REVIEW.md-mandated. I traced the code between the reset and getPrototype(): slot.isValue() is a plain type check, slot.getValue() on a value slot returns the stored JSValue without VM entry, and asString(value)->value() (rope resolution) already has CLEAR_IF_EXCEPTION and doesn't run JS. For proxies, VMInquiry getPropertySlot returns false without calling any trap, so has is false and the block is skipped entirely.
Other factors
- All three prior review threads (my spawned-fixture request, comment-cop's paragraph-comment note, CodeRabbit's stderr-pipe suggestion) are resolved and reflected in the current diff.
- The mechgate evidence in the PR body shows the test fails on both debug-ASAN and release builds without the fix (UBSan null-pointer member call / segfault) and passes with it.
- The test follows harness conventions:
bunEnv/bunExe(),await using proc, concurrent pipe drain viaPromise.all, stderr asserted before stdout before exitCode,it.concurrentfor the subprocess spawn, and it lives alongside the existingisErrorcases rather than in a new file. - No CODEOWNERS entries cover the touched paths.
2 participants
Problem
util.isError()crashes the process instead of returning or throwing when given a revoked Proxy or a Proxy with agetPrototypeOftrap. Release builds die withpanic(main thread): Segmentation fault at address 0x0; debug builds abort inVM::checkVMEntryPermission().The deprecated API is not the only way in:
internal/streams/iter/utils.ts(wrapError) runs every value a source throws throughutil.isError, so a program that throws such a Proxy intonode:stream/itersegfaults too (probe in the details block below).Cause 1,
jsFunctionIsErrorinsrc/jsc/modules/NodeUtilTypesModule.cpp: the result ofobject->getPrototype(globalObject)is used with no exception check. When the call throws (revoked proxy, throwing trap) it returns the emptyJSValue, which passesisCell(), soproto.inherits<ErrorInstance>()reads the structure of a null cell.Cause 2, same function: the
VMInquiryPropertySlotused a few lines earlier for theSymbol.toStringTaglookup is still alive whengetPrototype()runs, so itsDisallowVMEntryis still in force when the Proxy tries to call the trap. Debug builds crash there; release builds skip the trap and getundefinedback, which the Proxy turns into a TypeError, which then feeds cause 1. This is why even a well behaved trap (returningError.prototypeornull) crashed.Fix
slot.disallowVMEntry.reset()once thetoStringTaglookup has returned. This is the idiom JSC itself uses when a VMInquiry slot is followed by code that may run JS (JSScope.cpp,JSGlobalObject.cpp,JSInjectedScriptHost.cpp). The only later use of the slot isgetValue()on a slot thatisValue(), which reads a stored value and does not enter the VM.RETURN_IF_EXCEPTIONaftergetPrototype(), so a revoked proxy throws its TypeError and a throwing trap rethrows the trap's own value.util.isError(e)ends ine instanceof Error, which performs the same[[GetPrototypeOf]]on the proxy. It throws on a revoked proxy, propagates the trap's exception unchanged, and honors a trap's return value (nullis not an Error,Error.prototypeis). The new test asserts exactly those outcomes.test/js/node/util/util.test.js,util > isError > handles revoked proxies and getPrototypeOf traps. The crashing inputs run in a child process, one line of output per case: revoked proxy throws a TypeError, a throwing trap propagates its Error, a trap throwing a plain object rethrows that same object (caught === thrown),nulltrap givesfalse,Error.prototypetrap givestrue, a Proxy wrapping an Error givestrue.NodeUtilTypesModule.cppreverted to main: the test fails (the child aborts); the same-object case run on its own aborts with exit 134.USE_SYSTEM_BUN=1(release 1.4.0 canary): the test fails, the child segfaults.This PR replaces #37202, which fixed the same two problems in the same function; its extra test case (the trap's thrown value comes back as the same object) has been folded into the test here.
Not changed here:
jsFunctionIsErrorstill only looks one prototype level deep and atSymbol.toStringTag, which differs from Node'stoString(e) === "[object Error]" || e instanceof Errorfor some non-crashing inputs. That is a behavior question separate from this crash and is left as is.Background
PropertySlotwithInternalMethodType::VMInquiryasks JSC for a lookup with no observable side effects (no getters, no Proxy traps). To enforce that, the slot holds aDisallowVMEntrytoken for its whole lifetime; while any such token exists, JSC refuses to enter the VM to run JavaScript (debug: crash, release: the call is skipped and yieldsundefined).JSValuehere); the caller has to check the throw scope (RETURN_IF_EXCEPTION) before touching the result.JSValueis encoded as 0, which theisCell()fast check accepts, so using it as a cell is a null dereference.Probe: reaching the crash through node:stream/iter
bun 1.4.0 canary (8326d1b):
panic(main thread): Segmentation fault at address 0x0, exit 139.With this PR:
caught: Error: trap, exit 0.[review] gate passed · iteration 5 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 1 rejected · iteration 5
evidence per changed file