Fix use-after-free in Bun.inspect when user code mutates the object mid-format - #37169
Conversation
…id-format The fast property walk in JSC__JSValue__forEachPropertyImpl invoked the formatter callback from inside Structure::forEachProperty. The callback runs user code (a nested value's inspect.custom, getters on built-in subclasses), and if that code adds or deletes properties on the object being walked, the structure transitions and the PropertyTable rehash frees the index vector the walk is iterating. Collect the entries with no side effects first, then run the callbacks on the snapshot.
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The snapshot-then-iterate fix looks correct and the GC-rooting via MarkedArgumentBuffer is right, but since this is a memory-safety change in the JSC bindings fast-path property walk, a human look would still be worthwhile.
Checked: appending empty JSValue() to MarkedArgumentBuffer for the prototype case decodes back to empty and correctly falls through to getIfPropertyExists; Identifier in the snapshot retains the key's AtomStringImpl so deleted-mid-walk keys stay valid; snapshot vectors are re-initialized on each goto restart (declared inside the block); visitedProperties is still populated during the walk so cross-prototype dedup is preserved; the removed tryClearException() after getDirect(offset) was dead (getDirect on an offset doesn't throw).
Extended reasoning...
Overview
The PR fixes a heap-use-after-free in JSC__JSValue__forEachPropertyImpl (src/jsc/bindings/bindings.cpp). The fast path previously invoked the formatter callback and resolved getters via getIfPropertyExists from inside Structure::forEachProperty, which iterates the structure's PropertyTable index vector. User code reachable from the callback (a nested value's inspect.custom, a subclass getter like Map.prototype.size) can add/delete properties on the parent, causing PropertyTable::rehash to free the vector mid-iteration. The fix snapshots (Identifier, attributes, direct value) triples with no side effects during the walk, then runs getter resolution and the callback over the snapshot afterward. Values are held in a MarkedArgumentBuffer so GC during a callback can't collect them; keys are held as Identifier (refcounted AtomStringImpl). A comprehensive ASAN-gated regression test is added to test/js/bun/util/inspect.test.js.
Security risks
This is a fix for an ASAN-confirmed UAF reachable from untrusted JS via Bun.inspect/console.log. The fix reduces attack surface. No new security-sensitive surface is introduced.
Level of scrutiny
High. Per the repo's own review guidance, native memory safety in JSC bindings is the most-blocked category. The change is small and follows the canonical "collect first, then run user code" pattern (same class as #37168 for deepEquals), but it lives in a hot, GC-interacting path where subtle mistakes (empty-JSValue handling in MarkedArgumentBuffer, key lifetime after property deletion, exception-scope discipline, interaction with the goto restart prototype loop) matter. I traced each of these and they check out, but this is exactly the code a maintainer should sign off on.
Other factors
- The test is well-constructed: ASAN-only via
describe.skipIf(!isASAN), spawns a child withMalloc=1so bmalloc routes through the system allocator, drains stdout/stderr/exited concurrently, asserts exact stdout before exit code, and covers add/delete/Map-size-getter/prototype-walk/GC-churn variants. It also asserts the new snapshot semantics (mid-walk-added properties are not printed, matching Node). - The removed
(void)scope.tryClearException();afterobjectToUse->getDirect(entry.offset())was dead code —getDirecton a structure offset does not enter JS or throw. - Exception propagation is preserved:
CLEAR_IF_EXCEPTIONaftergetIfPropertyExists,RETURN_IF_EXCEPTIONafteriter. - No prior human or bot review comments to address; CodeRabbit was rate-limited.
…id-format (oven-sh#37169) ### Problem `Bun.inspect` and `console.log` have a heap-use-after-free when formatting a value runs user code that mutates the object being formatted. The default-enabled `Symbol.for("nodejs.util.inspect.custom")` hook on a nested value is enough to trigger it: ```js // Malloc=1 <bun-asan> repro.mjs const p = {}; for (let i = 0; i < 8; i++) p['k'+i] = i; let f = 0; p.a = { [Symbol.for('nodejs.util.inspect.custom')]() { if (!f++) for (let i = 0; i < 256; i++) p['n'+i] = i; return 'a'; } }; p.z = 1; console.log(Bun.inspect(p).length); ``` ASAN (with `Malloc=1` so JSC's bmalloc routes through the system allocator): ``` heap-use-after-free READ of size 8 #0 CompactPropertyTableEntry::key() Structure.h #1 PropertyTable::forEachProperty #2 Structure::forEachProperty #3 JSC__JSValue__forEachPropertyImpl bindings.cpp freed by: PropertyTable::destroyIndexVector <- PropertyTable::rehash <- PropertyTable::add <- Structure::addNewPropertyTransition <- JSObject::putDirectInternal ``` ### Cause The fast path of `JSC__JSValue__forEachPropertyImpl` walks the structure's `PropertyTable` with `Structure::forEachProperty` and invokes the formatter callback from inside the walk. The callback recursively formats the property value, which can run user code: a nested value's `inspect.custom`, or a getter on a built-in subclass (for example an overridden `Map.prototype.size`). If that code adds or deletes properties on the parent object, JSC rehashes the shared table, freeing the index vector the outer walk is iterating, and every remaining entry is read from freed memory. The fast-path guard only inspects the parent's structure, and a parent with plain data properties passes it; the hostile hook lives on a nested value. Same bug class as the deepEquals fix in oven-sh#37168, which deliberately excluded this site. ### Fix Collect the entries (key, attributes, direct value) under `forEachProperty` with no side effects, then resolve remaining values and invoke the callback on the snapshot after the walk finishes. The values go in a `MarkedArgumentBuffer` so GC in a callback cannot collect them; keys are retained as `Identifier`s. The snapshot is per structure walk, so the prototype-chain restart loop still re-reads each prototype's live structure. Properties added to the object while it is being formatted are no longer printed: the walk now reflects the object as it was when formatting started. That matches Node, which collects the key list before formatting values. The other `forEachProperty` sites are unaffected: the non-indexed and ordered variants never take this fast path, and the remaining callers run no user code in the callback. ### Verification - New test in `test/js/bun/util/inspect.test.js` (ASAN-only, child spawned with `Malloc=1`) covering: `inspect.custom` adding properties via `Bun.inspect` and `console.log`, deleting properties, a `Map` subclass `size` getter, the prototype fast-walk of an own-property-less object, and GC churn inside the hook with object-valued siblings formatted afterwards. Fails before the fix (ASAN abort, empty stdout), passes after. - `test/js/bun/util/inspect.test.js`: 74 pass. `test/js/bun/console/`: 85 pass, 1 skip. - `inspect-error.test.js` minified-file snapshots and `inspect-error-leak.test.js` fail identically with and without this diff locally (pre-existing, unrelated to property enumeration).
2 participants
Problem
Bun.inspectandconsole.loghave a heap-use-after-free when formatting a value runs user code that mutates the object being formatted. The default-enabledSymbol.for("nodejs.util.inspect.custom")hook on a nested value is enough to trigger it:ASAN (with
Malloc=1so JSC's bmalloc routes through the system allocator):Cause
The fast path of
JSC__JSValue__forEachPropertyImplwalks the structure'sPropertyTablewithStructure::forEachPropertyand invokes the formatter callback from inside the walk. The callback recursively formats the property value, which can run user code: a nested value'sinspect.custom, or a getter on a built-in subclass (for example an overriddenMap.prototype.size). If that code adds or deletes properties on the parent object, JSC rehashes the shared table, freeing the index vector the outer walk is iterating, and every remaining entry is read from freed memory.The fast-path guard only inspects the parent's structure, and a parent with plain data properties passes it; the hostile hook lives on a nested value. Same bug class as the deepEquals fix in #37168, which deliberately excluded this site.
Fix
Collect the entries (key, attributes, direct value) under
forEachPropertywith no side effects, then resolve remaining values and invoke the callback on the snapshot after the walk finishes. The values go in aMarkedArgumentBufferso GC in a callback cannot collect them; keys are retained asIdentifiers. The snapshot is per structure walk, so the prototype-chain restart loop still re-reads each prototype's live structure.Properties added to the object while it is being formatted are no longer printed: the walk now reflects the object as it was when formatting started. That matches Node, which collects the key list before formatting values. The other
forEachPropertysites are unaffected: the non-indexed and ordered variants never take this fast path, and the remaining callers run no user code in the callback.Verification
test/js/bun/util/inspect.test.js(ASAN-only, child spawned withMalloc=1) covering:inspect.customadding properties viaBun.inspectandconsole.log, deleting properties, aMapsubclasssizegetter, the prototype fast-walk of an own-property-less object, and GC churn inside the hook with object-valued siblings formatted afterwards. Fails before the fix (ASAN abort, empty stdout), passes after.test/js/bun/util/inspect.test.js: 74 pass.test/js/bun/console/: 85 pass, 1 skip.inspect-error.test.jsminified-file snapshots andinspect-error-leak.test.jsfail identically with and without this diff locally (pre-existing, unrelated to property enumeration).