-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Clear exceptions thrown by property lookups during inspect's property walk #38463
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟣 Pre-existing, not blocking: the same
getPrototype(globalObject).getObject()null-deref this hunk fixes has one other site in the tree —src/jsc/bindings/napi.cpp:2080, innapi_get_all_property_names's descriptor-filter loop. A throwing ProxygetOwnPropertyDescriptor/getPrototypeOftrap there produces the same empty-JSValue →.getObject()segfault; worth the same split-and-guard treatment as a follow-up.Extended reasoning...
What & where
REVIEW.md's "Fix the whole class in the same PR — grep for every sibling site sharing the pattern" rule prompts a grep for the exact chained call this PR's second
bindings.cpphunk fixes.getPrototype(globalObject).getObject()has exactly one other occurrence insrc/:src/jsc/bindings/napi.cpp:2080, insidenapi_get_all_property_names:There is no exception check between line 2079 (
getOwnPropertyDescriptor) and line 2085, and no guard ongetPrototype's return value before.getObject()is called on it.Code path that triggers it
napi_get_all_property_namesis called by native N-API modules. The loop at 2077–2085 is reached when:key_mode == napi_key_include_prototypes, andkey_filterincludes any ofnapi_key_enumerable | napi_key_writable | napi_key_configurable(thefilter_by_any_descriptorgate at line 2069).ownerstarts at the user-supplied object (fromtoJS(objectNapi)) and climbs its prototype chain, so a Proxy anywhere in that chain is reachable from JS.Why the existing code doesn't prevent it
The
if (!proto) break;check at line 2081 never runs whengetPrototypereturns an empty JSValue. As the PR description establishes for thebindings.cppcase: an empty JSValue encodes as 0, soisCell()returns true (0 & NotCellMask == 0),asCell()returns nullptr, and->isObject()readsm_typeat offset 5 from nullptr — segfault at address 0x5. The null check is dead code on the throwing path.There is also no
RETURN_IF_EXCEPTION/NAPI_RETURN_IF_EXCEPTIONbetweengetOwnPropertyDescriptor(which invokes the ProxygetOwnPropertyDescriptortrap) andgetPrototype(which invokes the ProxygetPrototypeOftrap).Step-by-step proof
Concrete example — a native addon calls:
where
objis:collectInheritedPropertyKeys(line 2060) enumerates"a"from the proxy target viagetPropertyNames.propKey = "a",owner = obj(the plain child).owner->getOwnPropertyDescriptor(...)on the child returns false (no own"a"), no exception yet.owner->getPrototype(globalObject)returns the Proxy;.getObject()succeeds;owner = proxy.owner->getOwnPropertyDescriptor(...)now invokes the Proxy trap, which throws.ProxyObject::performGetOwnPropertyDescriptorreturns false viaRETURN_IF_EXCEPTION, leaving the exception pending.owner->getPrototype(globalObject)on the Proxy entersProxyObject::getPrototype, whose throw scope observes the pending exception and returns{}(empty JSValue)..getObject()on the empty JSValue:isCell()→ true,asCell()→ nullptr,->isObject()reads offset 5 from nullptr → SIGSEGV at 0x5.A second variant: a stateful
getPrototypeOftrap that succeeds duringcollectInheritedPropertyKeys'getPropertyNameswalk (line 2007) but throws on the per-key descriptor climb hits step 6 directly —getPrototypereturns{}and.getObject()segfaults the same way.Impact
Segfault (crash) of the Bun process when a native N-API module calls
napi_get_all_property_nameswithnapi_key_include_prototypes+ a descriptor filter on an object whose prototype chain contains a Proxy with a throwinggetOwnPropertyDescriptororgetPrototypeOftrap. The surface is much narrower thanBun.inspect(requires a native addon using this specific N-API call with these specific flags), which is why it's flagged as a follow-up rather than a blocker.How to fix
Same treatment as this PR applies at
bindings.cpp:5684–5689: split the chained call, check for an exception aftergetOwnPropertyDescriptorand aftergetPrototype, and bail (NAPI_RETURN_IF_EXCEPTIONor break out of the filter loop) when either is set or when the returned JSValue is empty. Theowner->getOwnPropertyDescriptorat line 2087 (thenapi_key_own_onlybranch) also lacks an exception check and should get one in the same follow-up.Why this is
pre_existingThe PR does not touch
napi.cpp, does not add callers tonapi_get_all_property_names, and lives in a separate subsystem (N-API vs.Bun.inspect). This code has been shaped this way since before the PR. It is the exact bug class the PR fixes and the only other site with the pattern, so REVIEW.md's whole-class rule makes it worth mentioning — but it should not block merging this fix.