fix: route Proxy receivers through traps; fix Function.toString static read (#5135)#5184
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughFixes SIGSEGV in immer ChangesProxy Dispatch and toString HIR Fix (issue
Sequence Diagram(s)sequenceDiagram
participant TS as TypeScript (immer draft)
participant JSArrayPush as js_array_push_f64
participant ArrayPtrAsProxy as array_ptr_as_proxy
participant ProxyTraps as Proxy get/set traps
participant JSObjectSet as js_object_set_field_by_name
participant JSProxySet as js_proxy_set
TS->>JSArrayPush: holder.list.push(3) → arr, value=3
JSArrayPush->>ArrayPtrAsProxy: arr
ArrayPtrAsProxy-->>JSArrayPush: Some(boxed proxy)
JSArrayPush->>ProxyTraps: get proxy["length"] → len
JSArrayPush->>ProxyTraps: set proxy[len] = 3
JSArrayPush->>ProxyTraps: set proxy["length"] = len+1
JSArrayPush-->>TS: arr (no native realloc)
TS->>JSObjectSet: p.count++ → receiver=proxy_id, key="count"
JSObjectSet->>JSObjectSet: address-band check → proxy detected
JSObjectSet->>JSProxySet: NaN-boxed receiver, boxed key, value
JSProxySet-->>TS: write routed through set trap
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
… + array ops; fix Function.toString static read (#5135) Importing `immer` and calling `produce(base, d => { d.count++; d.list.push(3) })` crashed with SIGSEGV. immer drafts are `Proxy` objects statically typed as the plain base type, which exposed three independent bugs: 1. A compound-assignment write through a Proxy (`d.count++`) lowers to `js_object_set_field_by_name` with the proxy's NaN-box tag masked off. The write path had no proxy branch (only the read path did), so it dereferenced the masked proxy id as an `ObjectHeader` → SIGSEGV. Mirror the read-side dispatch: a registered proxy receiver routes to `js_proxy_set`. 2. The statically-typed `Function.toString` static-member read collapsed to `globalThis.toString` (folded to a number), so `Function.toString.call(Ctor)` in immer's `isPlainObject` threw "Function.prototype.call was called on a value that is not a function". `toString` is a universal inherited method; add it to the reroute-undo keep-list alongside `valueOf`/`hasOwnProperty`/… so the reified constructor receiver is preserved. 3. A native array method (`push`) / `length` read on a value that is a Proxy at runtime (`d.list.push(x)`) dereferenced the masked proxy id as an `ArrayHeader` → SIGSEGV. `js_array_push_f64` now performs the spec push for a single element through the proxy's `get`/`set` traps, and `js_array_length` reads `length` through the `get` trap, when the receiver is a registered proxy. (Recursing through the native method dispatcher would re-enter the same helper; the spec ops avoid that.) The exact issue repro now prints `0 1 1,2,3 2`, matching Node. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
proggeramlug
force-pushed
the
fix/immer-5135-proxy-compound-assign
branch
from
d55fb41 to
6569620
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/array/push_pop.rs`:
- Around line 157-160: The `js_proxy_set` function returns a boolean indicating
whether the proxy set operation succeeded, but the return values are being
ignored, causing failed proxy writes to silently succeed instead of throwing
errors in strict mode. In crates/perry-runtime/src/array/push_pop.rs#L157-L160,
modify the `proxy_set_str_key` function to return the boolean result from
`js_proxy_set` instead of discarding it. In
crates/perry-runtime/src/array/push_pop.rs#L176-L178, check the return value
from both the indexed-element write (the call to `proxy_set_str_key`) and the
"length" write (the `js_proxy_set` call), and throw a `TypeError` if either
returns false. In
crates/perry-runtime/src/object/field_set_by_name.rs#L198-L200, check the return
value from the proxy assignment (`js_proxy_set`) and throw a `TypeError` on
false for the strict property-write path.
- Around line 173-178: The `value` parameter must be rooted before any proxy
operations that could trigger allocation or garbage collection. In the unsafe
block where `proxy_array_length` and `proxy_set_str_key` are called, root
`value` at the beginning of the function or immediately after the `if let
Some(proxy)` check (before calling `proxy_array_length`) to prevent a moving GC
from invalidating the NaN-boxed heap pointer during the subsequent proxy
operations in this code block.
In `@crates/perry-runtime/src/object/field_set_by_name.rs`:
- Around line 193-197: The proxy-band check in the function is being performed
on a masked address value before it's been properly unmasked. Move the unmasking
operation (creating the boxed value with POINTER_TAG) before the
is_proxy_id_band check in the if statement, so that the proxy ID band detection
works on the actual unmasked receiver value rather than the masked
representation. This ensures that NaN-boxed pointer receivers cannot bypass
proxy dispatch by remaining in their masked form.
🪄 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: 25d6cddc-293f-477c-be53-bb69ca8c8d9a
📒 Files selected for processing (7)
crates/perry-hir/src/lower/expr_member.rscrates/perry-runtime/src/array/header.rscrates/perry-runtime/src/array/indexing.rscrates/perry-runtime/src/array/mod.rscrates/perry-runtime/src/array/push_pop.rscrates/perry-runtime/src/object/field_set_by_name.rscrates/perry/tests/issue_5135_proxy_compound_and_function_tostring.rs
Comment on lines
+157
to
+160
| unsafe fn proxy_set_str_key(proxy: f64, key_bytes: &[u8], value: f64) { | ||
| let key = crate::string::js_string_from_bytes(key_bytes.as_ptr(), key_bytes.len() as u32); | ||
| let key_f64 = crate::value::js_nanbox_string(key as i64); | ||
| crate::proxy::js_proxy_set(proxy, key_f64, value); |
There was a problem hiding this comment.
Do not discard failed proxy set results. The new proxy write paths call js_proxy_set but ignore its boolean result, so a set trap returning false silently succeeds instead of failing the strict/Throw=true write.
crates/perry-runtime/src/array/push_pop.rs#L157-L160: makeproxy_set_str_keyreturn whetherjs_proxy_setwas truthy.crates/perry-runtime/src/array/push_pop.rs#L176-L178: check both the indexed-element write and the"length"write, and throwTypeErrorif either returns false.crates/perry-runtime/src/object/field_set_by_name.rs#L198-L200: check the proxy assignment result and throw on false for the strict property-write path.
📍 Affects 2 files
crates/perry-runtime/src/array/push_pop.rs#L157-L160(this comment)crates/perry-runtime/src/array/push_pop.rs#L176-L178crates/perry-runtime/src/object/field_set_by_name.rs#L198-L200
🤖 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/array/push_pop.rs` around lines 157 - 160, The
`js_proxy_set` function returns a boolean indicating whether the proxy set
operation succeeded, but the return values are being ignored, causing failed
proxy writes to silently succeed instead of throwing errors in strict mode. In
crates/perry-runtime/src/array/push_pop.rs#L157-L160, modify the
`proxy_set_str_key` function to return the boolean result from `js_proxy_set`
instead of discarding it. In
crates/perry-runtime/src/array/push_pop.rs#L176-L178, check the return value
from both the indexed-element write (the call to `proxy_set_str_key`) and the
"length" write (the `js_proxy_set` call), and throw a `TypeError` if either
returns false. In
crates/perry-runtime/src/object/field_set_by_name.rs#L198-L200, check the return
value from the proxy assignment (`js_proxy_set`) and throw a `TypeError` on
false for the strict property-write path.
Comment on lines
+173
to
+178
| if let Some(proxy) = array_ptr_as_proxy(arr) { | ||
| let len = unsafe { proxy_array_length(proxy) }; | ||
| unsafe { | ||
| proxy_set_str_key(proxy, len.to_string().as_bytes(), value); | ||
| proxy_set_str_key(proxy, b"length", (len as f64) + 1.0); | ||
| } |
There was a problem hiding this comment.
Root value before proxy length/get/set work.
proxy_array_length and proxy_set_str_key can allocate and run proxy traps before value is rooted; if value is a NaN-boxed heap pointer, a moving GC can leave the later indexed write with stale bits.
Suggested fix
if let Some(proxy) = array_ptr_as_proxy(arr) {
+ let scope = crate::gc::RuntimeHandleScope::new();
+ let value_handle = scope.root_nanbox_f64(value);
let len = unsafe { proxy_array_length(proxy) };
unsafe {
- proxy_set_str_key(proxy, len.to_string().as_bytes(), value);
+ proxy_set_str_key(proxy, len.to_string().as_bytes(), value_handle.get_nanbox_f64());
proxy_set_str_key(proxy, b"length", (len as f64) + 1.0);
}
return arr;🤖 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/array/push_pop.rs` around lines 173 - 178, The
`value` parameter must be rooted before any proxy operations that could trigger
allocation or garbage collection. In the unsafe block where `proxy_array_length`
and `proxy_set_str_key` are called, root `value` at the beginning of the
function or immediately after the `if let Some(proxy)` check (before calling
`proxy_array_length`) to prevent a moving GC from invalidating the NaN-boxed
heap pointer during the subsequent proxy operations in this code block.
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.
Summary
Fixes #5135. Importing
immerand callingproduce(base, d => { d.count++; d.list.push(3) })crashed with SIGSEGV (exit 139, no output).immer's drafts are
Proxyobjects that are statically typed as the plain base type. That combination exposed three independent bugs in Perry. The exact issue repro now prints0 1 1,2,3 2, matching Node.Root causes & fixes
1. Compound assignment through a Proxy (
d.count++) → SIGSEGVPropertyUpdatecodegen writes viajs_object_set_field_by_namewith the proxy's NaN-box tag masked off. The write path had no proxy branch (only the read path,js_object_get_field_by_name, did), so it dereferenced the masked proxy id as anObjectHeader. Mirror the read-side dispatch: a registered proxy receiver routes tojs_proxy_set.→
crates/perry-runtime/src/object/field_set_by_name.rs2.
Function.toStringstatic read folded to a numberThe statically-typed
Function.toStringcollapsed toglobalThis.toString(folded to0.0), soFunction.toString.call(Ctor)in immer'sisPlainObjectthrew "Function.prototype.call was called on a value that is not a function".toStringis a universal inherited method; added it to the reroute-undo keep-list next tovalueOf/hasOwnProperty/… so the reified constructor receiver is preserved.→
crates/perry-hir/src/lower/expr_member.rs3. Native array op on a runtime Proxy (
d.list.push(3)) → SIGSEGVjs_array_push_f64/js_array_lengthdereferenced the masked proxy id as anArrayHeader. They now detect a registered proxy receiver and operate through its traps —pushperforms the spec single-element push (Get(P,"length"),Set(P,len,v),Set(P,"length",len+1)), andlengthreads through thegettrap. (Delegating to the native method dispatcher would re-enter the same helper; the spec ops avoid the recursion.)→
crates/perry-runtime/src/array/{push_pop,indexing,header,mod}.rsVerification
0 1 1,2,3 2(matches Node), onorigin/main.crates/perry/tests/issue_5135_proxy_compound_and_function_tostring.rs(3 tests, plainProxy, no immer dep) — pass.cargo testgreen forperry-runtime(1035),perry-hir,perry-codegen,perry-transform.Known follow-ups (out of scope, same architectural class)
Some immer-shaped patterns still hit Perry's array fast paths that trust static types over a runtime Proxy:
produce([1,2], d => d.push(3))) lowers to the inlineArrayPushLLVM path, which derefs the receiver directly;d.items[0].n = 9) routes index-get through a native array path.These are deeper (hot inline codegen) and tracked separately.
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
toStringpreserves the correct receiver context.lengthhandling and Proxy-basedpushbehavior.Tests
Function.toString/Array.toString, plus Proxy array mutation viapush.