inspect: resolve property values live after the mutation-safe key snapshot - #37178
inspect: resolve property values live after the mutation-safe key snapshot#37178robobun wants to merge 6 commits into
Conversation
…pshot Follow-up to #37169, which fixed the use-after-free by snapshotting both keys and values before running formatter callbacks. Freezing the values diverged from Node, from previous Bun releases, and from the slow path in the same function: a hook that overwrote a later-ordered sibling printed the stale value, and a deleted property was printed anyway. Keep the key-list snapshot (that is what the use-after-free fix requires) but resolve each value by name on the live structure in the post-walk loop, the same way the prototype path already did. Deleted properties are omitted, overwritten ones report the current value, and accessor redefinitions show [Getter], matching Node and the slow path. This also removes the MarkedArgumentBuffer value buffer, which is no longer needed.
WalkthroughChangesThe inspection enumerator now snapshots property identifiers and resolves values and attributes during the second pass. Tests cover mutations involving overwrites, deletions, re-additions, accessors, inherited properties, and multiple formatter paths. Inspection enumeration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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. Because it touches the JSC property-walk path that #37169 hardened against use-after-free (removing the MarkedArgumentBuffer and switching to live getDirect by-name lookups), a maintainer look would still be worthwhile.
What was reviewed:
- The
forEachPropertylambda now only appendsIdentifiers — no user code can run during iteration, so the UAF invariant from #37169 is preserved. - Removing
MarkedArgumentBufferis safe: values are no longer held across iterations; each is fetched immediately before its callback and covered by the existingEnsureStillAliveScope. - The
objectToUse != objectprototype path still falls through togetIfPropertyExists(attributes stays 0,!propertyValueshort-circuits). - New test cases (overwrite / delete / delete+re-add / redefine-as-getter) exercise the live-value semantics; the redefine case correctly hits the
Accessorattribute check becausegetDirect(vm, name, attrs)reads attributes live.
Extended reasoning...
Overview
This PR is a follow-up to #37169, which fixed a use-after-free in Bun.inspect's fast property walk by snapshotting entries before invoking formatter callbacks. That fix captured both keys and values, which regressed observable behavior: mutations to not-yet-formatted siblings (via inspect.custom hooks) were ignored because the frozen value was printed. This PR changes the snapshot to capture only the key list, then resolves each value by name via getDirect(vm, propertyName, attributes) in the post-walk loop. It touches ~20 lines in src/jsc/bindings/bindings.cpp and extends the ASAN-gated mutation test in test/js/bun/util/inspect.test.js with four new scenarios.
Security risks
None identified. The change is confined to the inspect/console.log formatter's property enumeration and does not touch auth, crypto, network, or filesystem paths. It does sit adjacent to a memory-safety fix, so the primary risk would be reintroducing the UAF — but the forEachProperty lambda now does strictly less than before (only appends Identifiers, no getDirect(offset) call), so the invariant that no user code runs during PropertyTable iteration is trivially preserved. The removed MarkedArgumentBuffer was rooting values held across the walk; since values are no longer held across the walk, there is nothing left to root, and each fetched value is covered by EnsureStillAliveScope for the callback duration.
Level of scrutiny
High. This is C++ JSC bindings code in the exact function that #37169 hardened against a use-after-free, and it removes a GC-rooting buffer. Per the repo's review guidelines, memory safety is the most-blocked category and changes here warrant maintainer eyes. The reasoning is sound and the diff simplifies the code, but I don't think this qualifies as "simple, mechanical, or obvious" enough for auto-approval.
Other factors
- The by-name
getDirect(vm, PropertyName, unsigned&)lookup is a pure structure lookup that cannot run user code, and it queries the object's current structure (not the pre-walk cachedstructurepointer), so mutations between callbacks are handled correctly. - The prototype-walk branch (
objectToUse != object) still works:propertyValuestays empty,attributesstays 0, and the!propertyValuedisjunct routes to the existinggetIfPropertyExistsfallback. - The new test assertions are placed in the existing subprocess fixture and assert exact stdout, which will fail on current main (stale
z: 1) per the PR description. The existing ASAN UAF cases from #37169 remain in place. - No prior human reviews or outstanding comments on the PR.
A getDirect miss for a key deleted mid-format skipped the property entirely, but the slow path resolves it through the prototype chain and prints the inherited value it was shadowing. Fall through to getIfPropertyExists instead: truly-gone keys are still omitted, shadowed ones match the slow path and previous releases. Also pin the overwrite/delete/re-add/redefine/shadow behavior across the fast, slow, and sorted formatter paths with a test that runs on every build.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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 `@src/jsc/bindings/bindings.cpp`:
- Around line 5493-5495: Update the fallback in the surrounding property lookup
logic to use a PropertySlot-based lookup instead of
objectToUse->getIfPropertyExists(), preserving inherited GetterSetter values as
accessors rather than invoking them. Add a test covering deletion of own z when
the prototype defines get z(), and verify the live result retains the accessor
representation (such as [Getter]).
🪄 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: 3f882403-564a-47a1-83ac-ae872efc731a
📒 Files selected for processing (2)
src/jsc/bindings/bindings.cpptest/js/bun/util/inspect.test.js
The deleted-key fallback used getIfPropertyExists, which invokes an inherited getter and prints its result; the slow path keeps the accessor and prints [Getter]. Resolve through a PropertySlot with the slow path's accessor handling instead, so the getter is not called and all paths agree.
There was a problem hiding this comment.
My earlier inline concern (inherited accessor invocation via getIfPropertyExists) is addressed in bdd3758 — the fallback now goes through getPropertySlot with the slow path's isAccessor() / isCacheableGetter() handling, and the throwing-getter row in the new matrix test pins it. I found no further issues on this revision. Given this is the memory-safety-sensitive fast property walk in the JSC bindings, a human look is still worthwhile.
What was reviewed:
- GC safety after dropping
MarkedArgumentBuffer—Identifieris refcounted (not GC-managed) and each value is fetched fresh underEnsureStillAliveScope, so nothing needs marking. - Exception handling around
getPropertySlot/slot.getValue— both covered by the existingCLEAR_IF_EXCEPTION, mirroring the slow path. objectToUse != objectprototype-walk case now routes through the same accessor-preserving fallback instead ofgetIfPropertyExists— strictly safer than before.- Test matrix covers fast/slow/sorted parity for all six mutation shapes; the inherited-getter row throws if invoked.
Extended reasoning...
Overview
Follow-up to #37169 in src/jsc/bindings/bindings.cpp (JSC__JSValue__forEachPropertyImpl, the fast structure walk backing Bun.inspect / console.log). #37169 fixed a use-after-free by snapshotting property entries before running the formatter callback, but froze both keys and values, so a hook mutating a not-yet-formatted sibling printed stale data. This PR keeps the key snapshot (still needed for the UAF fix) but re-resolves each value live via getDirect(vm, name, attributes), falling back to a PropertySlot lookup with the slow path's accessor handling when the direct lookup misses. The parallel MarkedArgumentBuffer is removed. Tests in test/js/bun/util/inspect.test.js add six mutation cases to the existing ASAN fixture plus a new non-ASAN cross-path matrix test asserting fast/slow/sorted agree.
Security risks
None. This is output formatting for Bun.inspect; no auth, crypto, or trust boundaries. The relevant safety property is that inspect must not invoke user getters — which this PR now enforces on the newly-reachable deleted-shadowing-key fallback (my prior review flagged the earlier revision for invoking inherited getters via getIfPropertyExists; bdd3758 fixed it and added a throwing-getter test row).
Level of scrutiny
High. This is C++ in the JSC bindings on a code path that just had a use-after-free (#37169), and it changes how values are held across callbacks that run arbitrary user JS. I checked: (1) dropping MarkedArgumentBuffer is safe because the snapshot now holds only Identifiers (refcounted UniquedStringImpl, not GC-managed) and each JSValue is fetched immediately before its callback under EnsureStillAliveScope; (2) getDirect(vm, PropertyName, attributes&) is a non-throwing structure lookup, and the fallback's getPropertySlot + slot.getValue are covered by CLEAR_IF_EXCEPTION matching the slow path at lines 5577–5627; (3) the isAccessor() → isCacheableGetter() ? getPureResult() : getterSetter() branch is byte-identical to the slow path's handling.
Other factors
- I previously reviewed at 0b6d0ed and flagged the inherited-accessor invocation; that was fixed in bdd3758 with test coverage (the getter throws if called), and CodeRabbit independently confirmed the fix. This is my first look at the fixed revision.
- The comment-cop bot fired repeatedly on comment length; the author trimmed to a one-sentence invariant note plus a two-line fallback comment, which seems appropriate for a UAF-adjacent invariant.
- Test coverage is thorough: ASAN fixture extended with 6 new cases, plus a build-agnostic 6×3 matrix (fast/slow/sorted) that would catch any future divergence between paths.
- Not approving because this is non-trivial JSC binding logic on a memory-safety-critical path; a maintainer familiar with
forEachProperty/PropertySlotsemantics should confirm the accessor-resolution shape.
|
Updated 9:06 PM PT - Aug 7th, 2026
✅ @robobun, your commit bdd3758de51a831c2eb0b8e1e898c12462e0e6e5 passed in 🧪 To try this PR locally: bunx bun-pr 37178That installs a local version of the PR into your bun-37178 --bun |
1 participant
Follow-up to #37169.
Problem
#37169 fixed the use-after-free in the
Bun.inspect/console.logfast property walk by snapshotting entries before invoking the formatter callback. It captured both keys and values, which changed observable behavior: when a hook (for exampleinspect.customon a nested value) mutates a sibling that has not been formatted yet, the frozen value was printed instead of the current one, and a deleted property was printed anyway.p.zp.z = 777z: 777z: 1(stale)z: 777z: 777delete p.zz: 1(resurrected)delete p.z; p.z = 999z: 999z: 1(stale)z: 999z: 999z: [Getter]z: 1(stale)z: [Getter]z: [Getter]delete p.zshadowing inheritedzz: "inherited"z: 1(stale)z: "inherited"The slow path in the same function (taken when the object has getters or is a Proxy) reads values live, so the two paths also disagreed with each other.
Fix
The use-after-free fix only requires that no user code runs while
Structure::forEachPropertyiterates thePropertyTable. So snapshot just the key list during the walk, and resolve each value by name in the post-walk loop:getDirect(vm, propertyName, attributes)against the live structure, falling back to aPropertySlotlookup with the slow path's accessor handling when the direct lookup misses. A by-name lookup holds no iteration state, so mutation between callbacks is fine: overwritten keys report the current value, a property redefined as an accessor shows[Getter]because the attributes are read live too, and a deleted key re-resolves through the prototype chain, printing the inherited value it was shadowing (an inherited accessor stays[Getter], not invoked) or omitted when gone, exactly like the slow path.This restores the pre-#37169 value semantics (which matched Node) while keeping its key-list snapshot semantics (which also match Node: properties added mid-format are not shown). The parallel
MarkedArgumentBuffervalue buffer is no longer needed and is removed; each value is now fetched right before its callback and protected by the existingEnsureStillAliveScope.Verification
test/js/bun/util/inspect.test.jswith overwrite, delete, delete+re-add, redefine-as-getter, delete-of-a-shadowing-key, and delete-of-a-key-shadowing-an-inherited-getter of a later-ordered sibling. The new assertions fail on current main (stalez: 1) and pass with this change; the use-after-free cases from Fix use-after-free in Bun.inspect when user code mutates the object mid-format #37169 still pass under ASAN withMalloc=1.test/js/bun/util/inspect.test.js: 80 pass.test/js/bun/console/: 85 pass, 1 skip.test/js/node/util/custom-inspect.test.js+bun-inspect.test.ts: 56 pass.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/inspect.test.js