-
-
Notifications
You must be signed in to change notification settings - Fork 132
fix: route Proxy receivers through traps; fix Function.toString static read (#5135) #5184
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 all commits
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 |
|---|---|---|
|
|
@@ -137,9 +137,47 @@ pub extern "C" fn js_array_grow(arr: *mut ArrayHeader, min_capacity: u32) -> *mu | |
| } | ||
|
|
||
| /// Push an element to the end of an array, growing if needed | ||
| /// #5135: read `Get(proxy, "length")` and ToLength-coerce it. Used by the | ||
| /// proxy-array push path so immer drafts (Proxies typed as arrays) mutate | ||
| /// through their traps instead of a native ArrayHeader deref. | ||
| unsafe fn proxy_array_length(proxy: f64) -> u64 { | ||
| let key = crate::string::js_string_from_bytes(b"length".as_ptr(), 6); | ||
| let key_f64 = crate::value::js_nanbox_string(key as i64); | ||
| let n = crate::builtins::js_number_coerce(crate::proxy::js_proxy_get(proxy, key_f64)); | ||
| if n.is_finite() && n >= 0.0 { | ||
| n as u64 | ||
| } else { | ||
| 0 | ||
| } | ||
| } | ||
|
|
||
| /// #5135: `Set(proxy, <string key>, value)` through the proxy's `set` trap. The | ||
| /// key string is allocated fresh per call so an intervening GC can't leave a | ||
| /// stale interior pointer. | ||
| 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); | ||
| } | ||
|
|
||
| /// Returns a pointer to the (possibly reallocated) array | ||
| #[no_mangle] | ||
| pub extern "C" fn js_array_push_f64(arr: *mut ArrayHeader, value: f64) -> *mut ArrayHeader { | ||
| // #5135: a Proxy whose static type is an array (immer drafts) reaches here | ||
| // with the masked proxy id. Perform the spec `Array.prototype.push` for a | ||
| // single element directly through the proxy's `get`/`set` traps: | ||
| // len = ToLength(Get(P, "length")); Set(P, len, value); Set(P, "length", len+1) | ||
| // Routing through the native push (`js_native_call_method`) would recurse | ||
| // back here with the same proxy. Return `arr` unchanged so the codegen's | ||
| // realloc write-back is a no-op (the proxy mutates its target in place). | ||
| 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); | ||
| } | ||
|
Comment on lines
+173
to
+178
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. Root
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 |
||
| return arr; | ||
| } | ||
| let arr = clean_arr_ptr_mut(arr); | ||
| if arr.is_null() { | ||
| return js_array_alloc(0); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,122 @@ | ||
| //! Regression tests for #5135: importing `immer` and calling | ||
| //! `produce(base, draft => { draft.count++; draft.list.push(3) })` crashed with | ||
| //! SIGSEGV. immer's drafts are `Proxy` objects that are statically typed as the | ||
| //! plain base type, which exposed three independent Perry bugs. These tests | ||
| //! reproduce each with a plain `Proxy` (no immer dependency needed): | ||
| //! | ||
| //! 1. A compound-assignment write through a `Proxy` (`p.count++`) lowered to | ||
| //! `js_object_set_field_by_name` with the proxy's NaN-box tag masked off; | ||
| //! the runtime had no proxy branch there and dereferenced the masked id as | ||
| //! an `ObjectHeader` → SIGSEGV. (The read side already routed proxies.) | ||
| //! 2. The statically-typed `Function.toString` static-member read collapsed to | ||
| //! `globalThis.toString` and folded to a number, so | ||
| //! `Function.toString.call(Ctor)` (immer's `isPlainObject`) threw | ||
| //! "Function.prototype.call was called on a value that is not a function". | ||
| //! 3. A native array method / `length` read on a value that is a `Proxy` at | ||
| //! runtime (`draft.list.push(x)`) dereferenced the masked proxy id as an | ||
| //! `ArrayHeader` → SIGSEGV. The array helpers now route a proxy receiver | ||
| //! through its traps. | ||
|
|
||
| use std::path::PathBuf; | ||
| use std::process::Command; | ||
|
|
||
| fn perry_bin() -> PathBuf { | ||
| PathBuf::from(env!("CARGO_BIN_EXE_perry")) | ||
| } | ||
|
|
||
| fn compile_and_run(source: &str) -> String { | ||
| let dir = tempfile::tempdir().expect("tempdir"); | ||
| let root = dir.path(); | ||
| let entry = root.join("main.ts"); | ||
| let output = root.join("main_bin"); | ||
| std::fs::write(&entry, source).expect("write entry"); | ||
|
|
||
| let compile = Command::new(perry_bin()) | ||
| .current_dir(root) | ||
| .arg("compile") | ||
| .arg(&entry) | ||
| .arg("-o") | ||
| .arg(&output) | ||
| .arg("--no-cache") | ||
| .output() | ||
| .expect("run perry compile"); | ||
| assert!( | ||
| compile.status.success(), | ||
| "perry compile failed\nstdout:\n{}\nstderr:\n{}", | ||
| String::from_utf8_lossy(&compile.stdout), | ||
| String::from_utf8_lossy(&compile.stderr) | ||
| ); | ||
|
|
||
| let run = Command::new(&output).output().expect("run compiled binary"); | ||
| assert!( | ||
| run.status.success(), | ||
| "compiled binary failed (signal/exit) — likely a SIGSEGV regression\nstatus: {:?}\nstdout:\n{}\nstderr:\n{}", | ||
| run.status, | ||
| String::from_utf8_lossy(&run.stdout), | ||
| String::from_utf8_lossy(&run.stderr) | ||
| ); | ||
| String::from_utf8_lossy(&run.stdout).to_string() | ||
| } | ||
|
|
||
| /// Fix #1: `proxy.count++` writes through the `set` trap instead of crashing. | ||
| #[test] | ||
| fn proxy_compound_assignment_routes_through_set_trap() { | ||
| let out = compile_and_run( | ||
| r#" | ||
| const target: any = { count: 0 }; | ||
| const p: any = new Proxy(target, { | ||
| get(t: any, k: any) { return t[k]; }, | ||
| set(t: any, k: any, v: any) { t[k] = v; return true; }, | ||
| }); | ||
| p.count++; | ||
| console.log(p.count, target.count); | ||
| "#, | ||
| ); | ||
| assert_eq!( | ||
| out, "1 1\n", | ||
| "p.count++ must write through the proxy set trap" | ||
| ); | ||
| } | ||
|
|
||
| /// Fix #2: `Function.toString` (and `Array.toString`) read as a value are real | ||
| /// functions, not numbers. | ||
| #[test] | ||
| fn function_tostring_static_member_is_callable() { | ||
| let out = compile_and_run( | ||
| r#" | ||
| console.log(typeof Function.toString); | ||
| console.log(typeof Array.toString); | ||
| // immer's isPlainObject reaches `Function.toString.call(Ctor)`: | ||
| console.log(typeof Function.toString.call(Array)); | ||
| "#, | ||
| ); | ||
| assert_eq!( | ||
| out, "function\nfunction\nstring\n", | ||
| "Function.toString / Array.toString must resolve to callable functions" | ||
| ); | ||
| } | ||
|
|
||
| /// Fix #3: a native array method (`push`) on a value that is a Proxy at runtime | ||
| /// dispatches through the proxy's traps instead of dereferencing the masked | ||
| /// proxy id as an ArrayHeader. This mirrors immer's `draft.list.push(x)`, where | ||
| /// the receiver is a member access (`obj.list`) that returns a proxy array — the | ||
| /// `js_array_push_f64` runtime helper path the issue actually exercised. | ||
| #[test] | ||
| fn proxy_array_push_via_member_routes_through_traps() { | ||
| let out = compile_and_run( | ||
| r#" | ||
| const target: any = [1, 2]; | ||
| const inner: any = new Proxy(target, { | ||
| get(t: any, k: any) { return t[k]; }, | ||
| set(t: any, k: any, v: any) { t[k] = v; return true; }, | ||
| }); | ||
| const holder: any = { list: inner }; | ||
| holder.list.push(3); | ||
| console.log(target.join(","), holder.list.length); | ||
| "#, | ||
| ); | ||
| assert_eq!( | ||
| out, "1,2,3 3\n", | ||
| "obj.list.push must mutate the proxied array through its set trap" | ||
| ); | ||
| } |
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.
Do not discard failed proxy
setresults. The new proxy write paths calljs_proxy_setbut ignore its boolean result, so asettrap returningfalsesilently succeeds instead of failing the strict/Throw=truewrite.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