-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -928,3 +928,82 @@ | |
| expect(exitCode).toBe(0); | ||
| }); | ||
| }); | ||
|
|
||
| // Run in a child: a regression here segfaults the process instead of throwing. | ||
| describe("property lookup throws while formatting an object", () => { | ||
| it.concurrent("Proxy traps in the prototype chain", async () => { | ||
| const fixture = ` | ||
| { | ||
| // Only "a" throws, so the walk has to carry on past it without the | ||
| // exception still pending when "b" and "c" are looked up. | ||
| const proto = new Proxy( | ||
| { a: 1, b: 2, c: 3 }, | ||
| { | ||
| get(target, key, receiver) { | ||
| if (key === "a") throw new Error("get trap"); | ||
| return Reflect.get(target, key, receiver); | ||
| }, | ||
| }, | ||
| ); | ||
| console.log(Bun.inspect(Object.create(proto))); | ||
| } | ||
| { | ||
| const proto = new Proxy( | ||
| { a: 1 }, | ||
| { | ||
| getPrototypeOf() { | ||
| throw new Error("getPrototypeOf trap"); | ||
| }, | ||
| }, | ||
| ); | ||
| const obj = Object.create(proto); | ||
| obj.x = 1; | ||
| console.log(Bun.inspect(obj)); | ||
| console.log(obj); | ||
| } | ||
| `; | ||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "-e", fixture], | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | ||
|
Check warning on line 971 in test/js/bun/util/inspect.test.js
|
||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Both new subprocess tests set Extended reasoning...What the issue isBoth tests added in the new const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);
Why this violates the repo's review rulesREVIEW.md states this explicitly under Tests reviewers reject:
It is also inconsistent with the file's own conventions: the two neighboring subprocess tests in this file — the huge-sparse-array test (~line 446) and the ASAN mutated-object test (~line 900) — both do Step-by-step: how the pattern can bite
In practice, an actual deadlock is unlikely today: Secondary cost: lost diagnosticsBeyond the deadlock hazard, not draining stderr means that when this test does fail (e.g. the child crashes on a regression), the failure output shows only an empty stdout and a nonzero exit code — the child's stderr, which would contain the actual crash report, is discarded. Asserting a combined FixChange both call sites to: const [stdout, stderr, exitCode] = await Promise.all([
proc.stdout.text(),
proc.stderr.text(),
proc.exited,
]);and either assert on |
||
| expect(stdout).toMatchInlineSnapshot(` | ||
| "{ | ||
| b: 2, | ||
| c: 3, | ||
| } | ||
| { | ||
| x: 1, | ||
| a: 1, | ||
| } | ||
| { | ||
| x: 1, | ||
| a: 1, | ||
| } | ||
| " | ||
| `); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| it.concurrent("lazy property of the Bun object", async () => { | ||
| // Bun.redis builds the default client the first time it is read, and an | ||
| // invalid REDIS_URL makes that throw. Only that property may be left out; | ||
| // the properties visited after it must still be printed. | ||
| const fixture = ` | ||
| const keys = Object.keys(Bun); | ||
| const out = Bun.inspect(Bun); | ||
| console.log(JSON.stringify(keys.filter(key => !out.includes("\\n " + key + ":")))); | ||
| `; | ||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "-e", fixture], | ||
| env: { ...bunEnv, REDIS_URL: "http://not-a-redis-url" }, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | ||
| expect(stdout).toBe('["redis"]\n'); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
| }); | ||
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.