fix(runtime): route Proxy values through [[Call]] in the ad-hoc closure probes (#6320)#6346
Merged
Merged
Conversation
…re probes (#6320) Follow-up to #5976/#6321. Ad-hoc CLOSURE_MAGIC / GcHeader probes still floored the candidate address at 0x1000 (or 0x10000) — an order of magnitude below HANDLE_BAND_MAX (0x100000). A Proxy is a NaN-boxed registry id (POINTER_TAG | PROXY_ID_BAND_START + id), so storing one where a closure is expected let the probe read *(0xF000D + 12) — unmapped low memory — and SIGSEGV (EXC_BAD_ACCESS at 0x000f000d). Confirmed against a pristine origin/main build: both fixtures below exit 139 there and 0 here. Unlike #5976, these sites have no proxy-aware fallback below them, so a band guard alone would stop the crash without dispatching the trap. Each site now routes a proxy through the Proxy [[Call]] path, matching node: * symbol/iterator.rs (js_to_primitive): `obj[Symbol.toPrimitive] = new Proxy(fn, {})` calls the proxy with `this` bound to the object and the hint string as its single argument — apply trap honored, revoked proxy throws TypeError. New proxy::call_proxy_value_with_this builds the spec argArray with every operand rooted across the allocations. * jsx.rs (function component): `<P />` where `P = new Proxy(Component, {})` dispatches through js_native_call_value's proxy branch; a proxy over a non-callable target is not a component (undefined). * symbol/properties.rs (js_object_set_symbol_method / js_object_set_method_by_name): the reserved this-slot patch is now one bind_reserved_this_slot helper that no-ops on anything that is not a real heap closure, and stores the value verbatim. Two further sites the sweep found, both on the same repro: * symbol/properties.rs register_closure_name_if_absent — a `<= 0x10000` floor in front of a `*(addr - 8)` GcHeader read. `{ [Symbol.toPrimitive]: proxyFn }` routes the proxy VALUE through it for fn.name inference. Now uses addr_class::try_read_gc_header. * closure/dynamic_props.rs js_closure_unbind_this — a `< 0x10000` floor; faulted on `const g = obj.m` where obj.m is a Proxy of a function. Guarding those turned the SIGSEGV into a spurious `TypeError: value is not a function`, because js_closure_callN receives a RAW pointer (codegen already stripped the tag) and get_valid_func_ptr correctly refuses to deref a proxy id. The compiler only emits ProxyApply when it can statically prove the callee is a proxy, so a proxy read out of any dynamically-typed slot — `const g = obj.m`, `arr[0]`, `map.get(k)` — could not be called at all. The 17 js_closure_callN entry points now fall back to dispatch_proxy_callee_or_throw, which re-boxes the id, asks the proxy registry, and calls [[Call]]; a non-proxy still throws exactly as before. This runs only on the branch that used to throw unconditionally, so there is no hot-path cost. The addr-class lint gate could not see any of the closure probes: HANDLE_FLOOR_RE only matches a floor written as a comparison, and these were written as `(0x1000..HEAP_MAX).contains(&addr)`. Added HANDLE_FLOOR_RANGE_RE for the range form — cleared when a band predicate guards the next few code lines, i.e. the #6321 fix shape — plus self-tests. The regenerated ratchet baseline drops the six fixed sites and records ten previously-invisible range-form sites as debt. Verification: test_gap_6320_proxy_closure_probes is byte-identical to node, including under PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1 over 120 diagnosed GC cycles with live forwarding stubs. Full 297-test gap suite A/B against a pristine origin/main build: zero regressions.
📝 WalkthroughWalkthroughThe runtime now routes proxy-valued closure slots through proxy call semantics, centralizes closure-pointer validation, updates JSX and symbol handling, extends handle-floor detection, and adds regression tests for proxy-based methods and components. ChangesProxy closure dispatch
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant JSXRuntime
participant is_valid_closure
participant js_proxy_is_proxy
participant js_native_call_value
JSXRuntime->>is_valid_closure: validate component pointer
JSXRuntime->>js_proxy_is_proxy: classify proxy component
js_proxy_is_proxy-->>JSXRuntime: callable proxy result
JSXRuntime->>js_native_call_value: invoke callable proxy with props
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 `@crates/perry-runtime/src/symbol/properties.rs`:
- Around line 590-606: Update bind_reserved_this_slot so the reserved capture
write goes through the existing barriered closure-capture setter, preserving the
same real_count - 1 slot and obj_f64 value. Reuse the path involving
note_closure_capture_slot/runtime_write_barrier_gc_slot instead of writing
directly through captures_ptr.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ba61f2ad-5bf4-4468-ad58-6b6ec9b4da3c
📒 Files selected for processing (13)
crates/perry-runtime/src/closure/dispatch.rscrates/perry-runtime/src/closure/dispatch/calln.rscrates/perry-runtime/src/closure/dispatch/validate.rscrates/perry-runtime/src/closure/dynamic_props.rscrates/perry-runtime/src/jsx.rscrates/perry-runtime/src/proxy.rscrates/perry-runtime/src/proxy/apply_construct.rscrates/perry-runtime/src/symbol/iterator.rscrates/perry-runtime/src/symbol/properties.rsscripts/addr_class_inventory.pyscripts/addr_class_ratchet_baseline.txttest-files/test_gap_6320_proxy_closure_probes.tstest-files/test_issue_6320_jsx_proxy_component.tsx
Comment on lines
+590
to
606
| unsafe fn bind_reserved_this_slot(closure_f64: f64, obj_f64: f64) { | ||
| let c_bits = closure_f64.to_bits(); | ||
| let c_tag = c_bits & 0xFFFF_0000_0000_0000; | ||
| if c_tag == POINTER_TAG { | ||
| let c_ptr = (c_bits & POINTER_MASK) as *mut crate::closure::ClosureHeader; | ||
| if !c_ptr.is_null() && (c_ptr as usize) >= 0x1000 { | ||
| // Read the type_tag at offset 12 (layout: func_ptr u64, capture_count u32, type_tag u32). | ||
| let type_tag = std::ptr::read_volatile( | ||
| (c_ptr as *const u8).add(crate::closure::CLOSURE_TYPE_TAG_OFFSET) as *const u32, | ||
| ); | ||
| if type_tag == crate::closure::CLOSURE_MAGIC { | ||
| let raw_count = (*c_ptr).capture_count; | ||
| let real_count = crate::closure::real_capture_count(raw_count); | ||
| if real_count >= 1 { | ||
| let captures_ptr = (c_ptr as *mut u8) | ||
| .add(std::mem::size_of::<crate::closure::ClosureHeader>()) | ||
| as *mut f64; | ||
| *captures_ptr.add((real_count - 1) as usize) = obj_f64; | ||
| } | ||
| } | ||
| } | ||
| if c_bits & 0xFFFF_0000_0000_0000 != POINTER_TAG { | ||
| return; | ||
| } | ||
| let c_addr = (c_bits & POINTER_MASK) as usize; | ||
| if !crate::closure::is_closure_ptr(c_addr) { | ||
| return; | ||
| } | ||
| let c_ptr = c_addr as *mut crate::closure::ClosureHeader; | ||
| let real_count = crate::closure::real_capture_count((*c_ptr).capture_count); | ||
| if real_count >= 1 { | ||
| let captures_ptr = (c_ptr as *mut u8) | ||
| .add(std::mem::size_of::<crate::closure::ClosureHeader>()) | ||
| as *mut f64; | ||
| *captures_ptr.add((real_count - 1) as usize) = obj_f64; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how rebuild_closure_layout_and_barriers is implemented and whether an
# equivalent barrier is applied elsewhere for single-slot capture mutations.
rg -n "rebuild_closure_layout_and_barriers|GC_STORE_AUDIT|write_barrier" crates/perry-runtime/src -A3 -B3Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first.
ast-grep outline crates/perry-runtime/src/symbol/properties.rs --view expanded
ast-grep outline crates/perry-runtime/src/closure/dynamic_props.rs --view expanded
ast-grep outline crates/perry-runtime/src/gc/copying.rs --view expanded
# Read the exact function bodies and nearby comments.
sed -n '560,650p' crates/perry-runtime/src/symbol/properties.rs
sed -n '1,260p' crates/perry-runtime/src/closure/dynamic_props.rs
sed -n '1,260p' crates/perry-runtime/src/gc/copying.rs
# Find all closure capture-slot writes and any barrier helpers used with them.
rg -n "capture_count|capture slot|rebuild_closure_layout_and_barriers|runtime_write_barrier|GC_STORE_AUDIT\\(" crates/perry-runtime/src/closure crates/perry-runtime/src/gc crates/perry-runtime/src/symbol -A3 -B3Repository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the allocation/call sites for the two helpers.
rg -n "js_object_set_symbol_method|js_object_set_method_by_name|bind_reserved_this_slot|captures_this=true|lower_object_literal|CAPTURES_THIS_FLAG" crates/perry-runtime crates/perry-codegen -A4 -B4
# Inspect the closure allocation and slot-barrier helpers.
rg -n "closure_capture_slots_mut|rebuild_closure_layout_and_barriers|js_closure_alloc|js_closure_alloc_with_captures|note_closure_capture_slot|runtime_write_barrier_slot|runtime_write_barrier_external_slot" crates/perry-runtime/src/closure crates/perry-runtime/src/gc -A4 -B4
# Inspect the copying GC remembered-set logic around old->young slot tracking.
sed -n '330,390p' crates/perry-runtime/src/gc/copying.rs
sed -n '560,720p' crates/perry-runtime/src/gc/copying.rs
# Read the caller-side codegen path if it exists in the repository.
fd -a 'expr.rs' crates/perry-codegen crates/perry-runtime | sed -n '1,20p'Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the object-literal lowering path that feeds js_object_set_*_method.
sed -n '306,520p' crates/perry-codegen/src/expr/object_literal.rs
# Inspect closure allocation and any post-write scanning/layout hooks.
sed -n '1,240p' crates/perry-runtime/src/closure/alloc.rs
sed -n '1,220p' crates/perry-runtime/src/closure/mod.rs
# Inspect how closure capture slots are scanned or remembered by the GC.
rg -n "closure_capture_slots_mut|scan_singleton_closure_roots_mut|scan_closure|rebuild_closure_layout_and_barriers|runtime_write_barrier_slot|StickyRememberedSet|remember_slot" crates/perry-runtime/src -A4 -B4
# If the codegen lowers methods through a helper, show that helper call site too.
rg -n "js_object_set_symbol_method|js_object_set_method_by_name|captures_this=true" crates/perry-codegen/src/expr crates/perry-runtime/src -A4 -B4Repository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the closure capture setter and related helpers.
rg -n "js_closure_set_capture_bits|js_closure_set_capture_f64|js_closure_set_capture_ptr|note_closure_capture_slot" crates/perry-runtime/src/closure/alloc.rs -A8 -B8
# Check whether closure capture slots are scanned as ordinary heap slots or only via special paths.
rg -n "gc_capture_slot_range|scan_singleton_closure_roots_mut|scan_closure_dynamic_props_roots_mut|visit_closure_dynamic_prop_value_slots_mut|rebuild_closure_layout_and_barriers" crates/perry-runtime/src/closure crates/perry-runtime/src/gc -A6 -B6
# Inspect the runtime method/property call path for these helpers.
sed -n '160,230p' crates/perry-runtime/src/object/property_key.rs
sed -n '1,120p' crates/perry-runtime/src/object/native_call_method.rsRepository: PerryTS/perry
Length of output: 36939
Add a GC barrier for the reserved this slot write
bind_reserved_this_slot writes a heap pointer directly into a live closure capture slot, but that slot is part of the GC-tracked capture range and the usual setter path records the store via note_closure_capture_slot/runtime_write_barrier_gc_slot. Reuse the barriered setter here, or this can drop an old→young edge and let the receiver be collected while the closure still points at it.
🤖 Prompt for 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.
In `@crates/perry-runtime/src/symbol/properties.rs` around lines 590 - 606, Update
bind_reserved_this_slot so the reserved capture write goes through the existing
barriered closure-capture setter, preserving the same real_count - 1 slot and
obj_f64 value. Reuse the path involving
note_closure_capture_slot/runtime_write_barrier_gc_slot instead of writing
directly through captures_ptr.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fixes #6320.
The bug
Follow-up to #5976/#6321. Ad-hoc CLOSURE_MAGIC / GcHeader probes still floored the candidate address at
0x1000(or0x10000) — an order of magnitude belowHANDLE_BAND_MAX(0x100000). A Proxy is a NaN-boxed registry id (POINTER_TAG | (PROXY_ID_BAND_START + id)), so storing one where a closure is expected let the probe read*(0xF000D + 12)— unmapped low memory — and SIGSEGV (EXC_BAD_ACCESS at 0x000f000d).Verified non-vacuously against a pristine
origin/mainbuild (735e894, own worktree + own target): both fixtures below exit 139 there and 0 here.Why a band guard alone wasn't enough
As @-noted on #6321, these sites have no proxy-aware fallback below them: a guard stops the crash but yields "not a closure" instead of dispatching the trap. So each site decides what node does and routes there.
symbol/iterator.rsjs_to_primitiveobj[Symbol.toPrimitive] = new Proxy(fn, {})Call(method, obj, «hint»)[[Call]]withthis= obj and the hint string as the single arg. Apply trap honored; revoked proxy throwsTypeError; a proxy over a non-callable target falls through to the existing "not a method" return (Perry does not throw for a non-callable@@toPrimitivein general — unchanged, and out of scope here)jsx.rsfunction component<P />whereP = new Proxy(Component, {})[[Call]]js_native_call_value's proxy branch (components take nothis). A proxy over a non-callable target is not a component →undefinedsymbol/properties.rsjs_object_set_symbol_method/js_object_set_method_by_namebind_reserved_this_slothelper that no-ops on anything that is not a real heap closure, and stores the value verbatim; the call site then dispatches it through[[Call]]New
proxy::call_proxy_value_with_thisbuilds the specargArrayfor these callers, keeping every operand rooted across the allocations.Extra sites found by the sweep
Both fire on the same repro, so the fixture would still have crashed without them:
symbol/properties.rsregister_closure_name_if_absent— a<= 0x10000floor in front of a*(addr - 8)GcHeader read.{ [Symbol.toPrimitive]: proxyFn }routes the proxy VALUE through it forfn.nameinference. Now usesaddr_class::try_read_gc_header.closure/dynamic_props.rsjs_closure_unbind_this— a< 0x10000floor; faulted onconst g = obj.mwhereobj.mis aProxy(fn).All the probes now delegate to
closure::is_closure_ptr, which owns the band + heap-range + alignment + magic checks.The TypeError that guarding revealed
Guarding those two turned the SIGSEGV into a spurious
TypeError: value is not a function.js_closure_callNreceives a RAW*const ClosureHeader— codegen already stripped the tag injs_closure_unbox_callee_checked— andget_valid_func_ptrcorrectly refuses to dereference a proxy id (#5976/#6321). The compiler only emits aProxyApplynode when it can statically prove the callee is a proxy, so a proxy read out of any dynamically-typed slot could not be called at all:The 17
js_closure_callNentry points now fall back todispatch_proxy_callee_or_throw, which re-boxes the id, asks the proxy registry, and calls[[Call]]; a non-proxy throws exactly as before. It runs only on the branch that previously threw unconditionally, so there is no hot-path cost. All four shapes above now match node.Lint gate (
scripts/addr_class_inventory.py)The addr-class ratchet could not see any of the closure probes:
HANDLE_FLOOR_REonly matches a floor written as a comparison, but these were written as(0x1000..HEAP_MAX).contains(&addr)— the literal is followed by.., and the address operand appears afterwards inside.contains(&…). That blind spot is exactly why they survived the #6279 sweep.Added
HANDLE_FLOOR_RANGE_REfor the range form, reported under the existinghandle-floorrule so it shares that per-file ratchet. A band predicate guarding the next few code lines (comments skipped) clears it — that is the #6321 remediation shape, so the rule accepts a fix instead of blocking it. Self-tests cover the flag, the clear, and the correct-boundary non-finding.The regenerated baseline drops the six sites fixed here and records ten previously-invisible range-form sites (
native_abi.rs,buffer/query.rs,timer.rs,node_vm.rs, …) as debt. Those are raw-pointer classifiers followed by registry lookups, not derefs — surfaced, not introduced.Tests
test-files/test_gap_6320_proxy_closure_probes.ts— node-compared gap test covering all five runtime sites (toPrimitive proxy + apply trap + hints + nested + revoked, computed-symbol methods, spread method-by-name, proxy under a computed key before/after a spread, detached proxy method). Byte-identical to node.test-files/test_issue_6320_jsx_proxy_component.tsx— end-to-end JSX repro (run_parity_tests.shonly globs*.ts, and node can't run Perry's server-JSX runtime, so this is a checked-in repro, not a suite entry).jsx.rsunit tests are the CI gate for the JSX site:proxy_wrapped_function_component_dispatches_through_call,proxy_over_non_callable_target_is_not_a_component,closure_function_component_renders(control),is_valid_closure_rejects_the_handle_band(every band id rejected without a deref).Verification
origin/main, exit 0 and byte-identical to node here.PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1 PERRY_GC_DIAG=1on an allocation-heavy variant: 120 diagnosed GC cycles with live forwarding stubs, no verifier panic, output still matches node. (The plain fixture is too small to trigger a collection — a forced-evacuate run over it is vacuous.)cargo test -p perry-runtime: 1288 passed / 1 failed —array::tests::test_array_exotic_descriptors_and_global_prototype_identity, the known parallel-suite flake (passes 5/5 standalone, unrelated to this change).origin/mainbuild: zero regressions. The only deltas aretest_gap_6320_proxy_closure_probes(CRASH 139 → PASS) andtest_gap_node_fs, the known parallel-harness flake — 5/5 serially on both sides.test_gap_http_overloads_3226plusaborts (134) on both sides (pre-existing).cargo fmt --all -- --check,scripts/check_file_size.sh,scripts/addr_class_inventory.py --self-testand the audit itself all clean.Summary by CodeRabbit
Bug Fixes
Proxyvalues, including proxied JSX components.[Symbol.toPrimitive]methods and non-callable proxy targets.Tests