diff --git a/changelog.d/6941-property-key-operand-rooting.md b/changelog.d/6941-property-key-operand-rooting.md new file mode 100644 index 0000000000..6e428301bd --- /dev/null +++ b/changelog.d/6941-property-key-operand-rooting.md @@ -0,0 +1,24 @@ +fix(runtime): root receivers and stored values across GC-capable property-key coercions (#6935) + +`ToPropertyKey(key)` runs a user `Symbol.toPrimitive` / `toString` / `valueOf`, and even for a primitive key it allocates the stringified form — either can trigger a GC that **evacuates** live objects. The property-key entry points held the receiver, and on the write paths the *value being stored*, as raw `f64` / raw-pointer Rust locals across that call: + +```rust +let key = js_to_property_key(key_value); // user JS -> allocate -> GC -> evacuation +let obj = extract_obj_ptr(obj_value); // receiver was raw across the coercion +js_object_set_field_by_name(obj, key_str, value); // stale receiver AND stale value +``` + +A Rust local is neither a GC root nor a shadow slot. This is the `ToPropertyKey` sibling of the operator family fixed in #6934, and strictly worse: there a stale operand produced one wrong answer, whereas here the stale `value` is written **into a live object**, so the dangling pointer outlives the call. + +Same idiom as #6934: `crate::gc::RuntimeHandleScope` plus `root_nanbox_f64` / `root_raw_mut_ptr` / `root_string_ptr` / `root_heap_word_u64`, re-reading the receiver and the stored value through their handles after every GC-capable step. Two shared helpers land in `object/property_key.rs`: + +- `property_key_coercion_is_inert(key)` — the plain-double-fast-path analogue. Only an already-heap `STRING_TAG` key qualifies (`js_to_primitive` returns any non-`POINTER_TAG` value unchanged, `ordinary_to_primitive_string_key` bails on it, `js_jsvalue_to_string` hands the same pointer back), so the hot `obj[strKey]` / `hasOwnProperty("x")` shapes keep the pre-fix code path verbatim. +- `to_property_key_rooted(scope, key)` — the coercion inside a caller-owned scope, returning the coerced key as a handle. + +Receivers that may arrive NaN-boxed, as a bare heap address (module-level object slots store the untagged pointer), or as an INT32 class-ref are rooted with `root_heap_word_u64`, which rewrites only the real pointers and preserves each encoding. Where the coercion already sat behind an early return (the typed-array / canonical-index fast paths in `js_dyn_index_{get,set}`, `js_object_{get,set}_index_polymorphic`, `js_array_set_index_or_string`) the scope is placed in the cold arm only, so the #5525 hot paths are untouched. + +Sites fixed — the seven named in #6935: `object/property_key.rs` (`js_object_set_property_key`, `js_object_get_property_key`, `js_object_set_property_key_method`, plus `js_super_accessor_get` / `js_object_super_call`), `object/object_literal_ops.rs` (`object_literal_key_to_string`, `js_object_literal_set_computed`, `js_object_define_accessor`), `value/dyn_index.rs` (`js_dyn_index_set` object + class-ref arms, `js_dyn_index_get` object numeric-key arm), `object/polymorphic_index.rs` (`js_object_{get,set}_index_polymorphic`, non-canonical-key arms), `object/delete_rest.rs` (`js_object_delete_dynamic`), `array/indexing.rs` (`js_array_{get,set}_index_or_string`), `object/native_call_method.rs` (`js_native_call_method_value`). Five more the sweep turned up in the same family: `object/object_ops/has_own.rs` (`js_object_has_own`, `js_object_property_is_enumerable`), `object/native_call_method/common_methods.rs` (the `hasOwnProperty` / `propertyIsEnumerable` arms, re-read through the caller's existing root handle), `proxy.rs` (`target_set` — `target_get` was already rooted, its write sibling was not), `object/field_get_set/has_property.rs` (`js_object_has_property`, number-key coercion), and `typed_feedback.rs` (the object/closure arm of the typed-feedback index set). + +Checked and not affected: `builtins/console.rs`, `object/class_registry/parent_static.rs`, `proxy.rs` `js_proxy_get` / `property_key_to_rust_string`, the `has_own.rs` handle-band arm, and the class-ref / small-handle arms of `js_dyn_index_get`. + +New regression suite `crates/perry/tests/gc_property_key_operand_rooting_6935.rs` runs the write paths, the receiver-only paths and the proxy forward-to-target write under `PERRY_GC_FORCE_EVACUATE=1` + `PERRY_GC_VERIFY_EVACUATION=1`. It passes on the pre-fix runtime too, and its module doc says so: no in-language configuration currently reaches a *minor* cycle that evacuates while the raw runtime locals are unpinned. `gc()` runs a full mark-sweep (evacuation is minor-only, so nothing moves); `perry/gc`'s `minor()` does evacuate but engages `ManualGcScanGuard::force_full_scan()` (#4977), whose conservative stack scan pins exactly the raw locals at issue; and `minor()` with `PERRY_CONSERVATIVE_STACK_SCAN=off` is independently unsound on this build (a plain method's `this` is lost across the collection with or without the fix). The suite is therefore a behavioral guard that will start failing the day a minor-evacuating configuration becomes reachable from compiled code. diff --git a/crates/perry-runtime/src/array/indexing.rs b/crates/perry-runtime/src/array/indexing.rs index 565d8356cc..3c77eeaaba 100644 --- a/crates/perry-runtime/src/array/indexing.rs +++ b/crates/perry-runtime/src/array/indexing.rs @@ -1642,19 +1642,34 @@ pub extern "C" fn js_array_get_index_or_string(arr: *const ArrayHeader, idx: f64 } else { format!("{:.0}", n) }; + // #6935: `js_string_from_bytes` ALLOCATES, so it can trigger a GC + // that evacuates the receiver; `arr` is a bare Rust local. + let scope = crate::gc::RuntimeHandleScope::new(); + let arr_handle = scope.root_raw_const_ptr(arr); let key_ptr = crate::string::js_string_from_bytes(key.as_ptr(), key.len() as u32); - return array_get_property_by_key(arr, key_ptr); + return array_get_property_by_key( + arr_handle.get_raw_const_ptr::(), + key_ptr, + ); } } if unsafe { crate::symbol::js_is_symbol(idx) } != 0 { return f64::from_bits(crate::value::TAG_UNDEFINED); } + // #6935: read-side sibling of `js_array_set_index_or_string` below — + // `js_jsvalue_to_string` on an object key (`a[new Number(1)]`, + // `a[{toString(){...}}]`) runs user JS, allocates and can evacuate `arr`. + let scope = crate::gc::RuntimeHandleScope::new(); + let arr_handle = scope.root_raw_const_ptr(arr); let key = crate::value::js_jsvalue_to_string(idx); if key.is_null() { return f64::from_bits(crate::value::TAG_UNDEFINED); } - array_get_property_by_key(arr, key as *const crate::StringHeader) + array_get_property_by_key( + arr_handle.get_raw_const_ptr::(), + key as *const crate::StringHeader, + ) } /// `arr[idx] = value` where idx may be a NaN-boxed string (numeric-string @@ -1714,10 +1729,20 @@ pub extern "C" fn js_array_set_index_or_string( // number ("4294967295", "-1", "1.5", "NaN") rather than a truncated // integer — `js_array_set_string_key` then stores it on the expando // map without touching `length` or any element slot. (Issue #4543.) + // #6935: `js_jsvalue_to_string` allocates the stringified key, so it can + // GC and evacuate both the receiver and the value being stored. + let scope = crate::gc::RuntimeHandleScope::new(); + let arr_handle = scope.root_raw_mut_ptr(arr); + let value_handle = scope.root_nanbox_f64(value); let key = crate::value::js_jsvalue_to_string(idx); if !key.is_null() { - return js_array_set_string_key(arr, key as *const crate::StringHeader, value); + return js_array_set_string_key( + arr_handle.get_raw_mut_ptr::(), + key as *const crate::StringHeader, + value_handle.get_nanbox_f64(), + ); } + return arr_handle.get_raw_mut_ptr::(); } // Fallback for a NON-numeric key: a primitive (`a[null]`, `a[undefined]`, // `a[true]`, `a[10n]`) or a boxed object (`a[new Number(1)]`). Per @@ -1726,11 +1751,25 @@ pub extern "C" fn js_array_set_index_or_string( // Arrays previously DROPPED these writes (plain objects handled them). // Restricted to `numeric.is_none()`: numeric keys (including non-integer // finite floats) are handled above. Symbols stay symbol-keyed. + // + // #6935: this is the boxed-object arm the doc comment above names, so + // `js_jsvalue_to_string` here runs a USER `toString` / `valueOf` — allocate + // → GC → evacuation. Pre-fix `arr` and `value` were both raw Rust locals + // across it, so a stale receiver dropped the write and a stale `value` + // stored a dangling pointer inside a live array. if numeric.is_none() && unsafe { crate::symbol::js_is_symbol(idx) } == 0 { + let scope = crate::gc::RuntimeHandleScope::new(); + let arr_handle = scope.root_raw_mut_ptr(arr); + let value_handle = scope.root_nanbox_f64(value); let key = crate::value::js_jsvalue_to_string(idx); if !key.is_null() { - return js_array_set_string_key(arr, key as *const crate::StringHeader, value); + return js_array_set_string_key( + arr_handle.get_raw_mut_ptr::(), + key as *const crate::StringHeader, + value_handle.get_nanbox_f64(), + ); } + return arr_handle.get_raw_mut_ptr::(); } arr } diff --git a/crates/perry-runtime/src/object/delete_rest.rs b/crates/perry-runtime/src/object/delete_rest.rs index 43d2bf97fb..42ffaa685b 100644 --- a/crates/perry-runtime/src/object/delete_rest.rs +++ b/crates/perry-runtime/src/object/delete_rest.rs @@ -484,7 +484,16 @@ pub extern "C" fn js_object_delete_dynamic(obj: *mut ObjectHeader, key: f64) -> return js_object_delete_field(obj, key_str); } + // #6935: the string-key case returned above, so `key` here is a number, a + // BigInt, a boolean, `null`/`undefined` — or an OBJECT, whose + // `Symbol.toPrimitive` / `toString` / `valueOf` runs user JS. Either way + // `js_to_property_key` allocates and can trigger a GC that **evacuates** + // the receiver, and `obj` is a bare Rust local across it. Root it and read + // it back through the handle for both the symbol and string delete arms. + let scope = crate::gc::RuntimeHandleScope::new(); + let obj_handle = scope.root_raw_mut_ptr(obj); let property_key = unsafe { js_to_property_key(key) }; + let obj = obj_handle.get_raw_mut_ptr::(); if unsafe { crate::symbol::js_is_symbol(property_key) } != 0 { // Symbol-keyed delete (`delete obj[Symbol.iterator]`). Previously this // fell through to the vacuous `return 1`, so the delete *reported* @@ -496,9 +505,13 @@ pub extern "C" fn js_object_delete_dynamic(obj: *mut ObjectHeader, key: f64) -> let obj_f64 = crate::value::js_nanbox_pointer(obj as i64); return unsafe { crate::symbol::js_object_delete_symbol_property(obj_f64, property_key) }; } - let key_str = crate::value::js_jsvalue_to_string(property_key); + let property_key_handle = scope.root_nanbox_f64(property_key); + let key_str = crate::value::js_jsvalue_to_string(property_key_handle.get_nanbox_f64()); if !key_str.is_null() { - return js_object_delete_field(obj, key_str as *const crate::StringHeader); + return js_object_delete_field( + obj_handle.get_raw_mut_ptr::(), + key_str as *const crate::StringHeader, + ); } // For other types, delete succeeds vacuously diff --git a/crates/perry-runtime/src/object/field_get_set/has_property.rs b/crates/perry-runtime/src/object/field_get_set/has_property.rs index 3ba3aad245..63aca8e399 100644 --- a/crates/perry-runtime/src/object/field_get_set/has_property.rs +++ b/crates/perry-runtime/src/object/field_get_set/has_property.rs @@ -203,12 +203,21 @@ pub extern "C" fn js_object_has_property(obj: f64, key: f64) -> f64 { // redirect on the happy path) surfaced as a fatal 500 instead of a 307. // (Symbols and strings pass through unchanged; a proxy/handle receiver is // handled below with the coerced key.) - let key = { + // + // #6935: that coercion ALLOCATES the stringified key, so it can trigger a + // GC that evacuates the receiver — and `obj` / `obj_val` are raw locals + // captured above. (Only the number arm coerces, so no user JS runs here, + // but an allocation-triggered evacuation moves the receiver just the same.) + let (obj, obj_val, key) = { let kv = JSValue::from_bits(key.to_bits()); if kv.is_number() { - unsafe { crate::object::js_to_property_key(key) } + let scope = crate::gc::RuntimeHandleScope::new(); + let obj_handle = scope.root_heap_word_u64(obj.to_bits()); + let key = unsafe { crate::object::js_to_property_key(key) }; + let obj = f64::from_bits(obj_handle.get_heap_word_u64()); + (obj, JSValue::from_bits(obj.to_bits()), key) } else { - key + (obj, obj_val, key) } }; let key_val = JSValue::from_bits(key.to_bits()); diff --git a/crates/perry-runtime/src/object/native_call_method.rs b/crates/perry-runtime/src/object/native_call_method.rs index 271b84f1ba..2f6b9b50ef 100644 --- a/crates/perry-runtime/src/object/native_call_method.rs +++ b/crates/perry-runtime/src/object/native_call_method.rs @@ -388,11 +388,28 @@ pub unsafe extern "C" fn js_native_call_method_value( } } - let property_key = if is_symbol_key { - key - } else { - crate::object::js_to_property_key(key) - }; + // #6935: on the non-symbol path `js_to_property_key` runs a user + // `Symbol.toPrimitive` / `toString` / `valueOf` (and allocates for every + // primitive key), so it can trigger a GC that **evacuates** the receiver. + // `object` is a raw NaN-boxed Rust local held across it and is dereferenced + // by every dispatch arm below. Root it and read it back through the handle. + // The inert case (an already-heap string key) keeps the pre-fix shape so + // the hot `obj[strKey](...)` dispatch pays nothing. + let (property_key, object) = + if is_symbol_key || crate::object::property_key_coercion_is_inert(key) { + // A heap string is its own property key — `js_to_property_key` + // returns the identical NaN-boxed bits without allocating — so the + // pre-fix shape is preserved verbatim for the hot path. + (key, object) + } else { + let scope = crate::gc::RuntimeHandleScope::new(); + let object_handle = scope.root_heap_word_u64(object.to_bits()); + let property_key = crate::object::js_to_property_key(key); + ( + property_key, + f64::from_bits(object_handle.get_heap_word_u64()), + ) + }; if !is_symbol_key && crate::symbol::js_is_symbol(property_key) != 0 { return js_native_call_method_value(object, property_key, args_ptr, args_len); } diff --git a/crates/perry-runtime/src/object/native_call_method/common_methods.rs b/crates/perry-runtime/src/object/native_call_method/common_methods.rs index ff2cdaf8c2..22eb8884ce 100644 --- a/crates/perry-runtime/src/object/native_call_method/common_methods.rs +++ b/crates/perry-runtime/src/object/native_call_method/common_methods.rs @@ -69,7 +69,17 @@ pub(super) unsafe fn dispatch_common( } else { f64::from_bits(crate::value::TAG_UNDEFINED) }; + // #6935: `js_to_property_key` can run a user `Symbol.toPrimitive` / + // `toString` / `valueOf` (and allocates for every primitive key), so + // it can trigger a GC that **evacuates** the receiver. `object` — + // and the `jsval` tag view derived from it at the top of this + // function — are raw locals captured *before* the coercion; re-read + // the receiver through the caller's `object_handle`, which IS a + // root, and re-derive the tag view from that. let key_value = crate::object::js_to_property_key(key_value); + let key_value = root_scope.root_nanbox_f64(key_value).get_nanbox_f64(); + let object = object_handle.get_nanbox_f64(); + let jsval = JSValue::from_bits(object.to_bits()); if crate::symbol::js_is_symbol(key_value) != 0 { return Some(super::object_ops::js_object_has_own(object, key_value)); } @@ -224,7 +234,14 @@ pub(super) unsafe fn dispatch_common( // `toString`/`valueOf` yields a Symbol must be treated as that // Symbol (test262 propertyIsEnumerable/symbol_property_*), invoking // the user conversion exactly once. + // + // #6935: that user conversion can GC and evacuate the receiver, so + // re-read `object`/`jsval` through the caller's root handle + // afterwards — see the `hasOwnProperty` arm above. let key_value = crate::object::js_to_property_key(key_value); + let key_value = root_scope.root_nanbox_f64(key_value).get_nanbox_f64(); + let object = object_handle.get_nanbox_f64(); + let jsval = JSValue::from_bits(object.to_bits()); // Symbol keys must not be string-coerced — route through the // canonical entry, which consults the SYMBOL_PROPERTIES side // table (mirrors hasOwnProperty's symbol arm). diff --git a/crates/perry-runtime/src/object/object_literal_ops.rs b/crates/perry-runtime/src/object/object_literal_ops.rs index 6bf2e1c807..3031cc89f1 100644 --- a/crates/perry-runtime/src/object/object_literal_ops.rs +++ b/crates/perry-runtime/src/object/object_literal_ops.rs @@ -7,26 +7,50 @@ use super::*; pub(super) unsafe fn object_literal_key_to_string(key_value: f64) -> *mut crate::StringHeader { let key_jsv = crate::value::JSValue::from_bits(key_value.to_bits()); - if key_jsv.is_pointer() && crate::symbol::js_is_symbol(key_value) == 0 { - let obj_ptr = (key_value.to_bits() & crate::value::POINTER_MASK) as usize; - if obj_ptr >= 0x10000 && is_valid_obj_ptr(obj_ptr as *const u8) { - let to_string_key = - crate::string::js_string_from_bytes(b"toString".as_ptr(), b"toString".len() as u32); - let value_of_key = - crate::string::js_string_from_bytes(b"valueOf".as_ptr(), b"valueOf".len() as u32); - let obj = obj_ptr as *const ObjectHeader; - let gc = gc_header_for(obj); - let to_string = js_object_get_field_by_name(obj, to_string_key); - let value_of = js_object_get_field_by_name(obj, value_of_key); - if ((*gc)._reserved & crate::gc::OBJ_FLAG_NULL_PROTO) != 0 - && to_string.is_undefined() - && value_of.is_undefined() - { - throw_object_type_error(b"Cannot convert object to primitive value"); - } - } + if !(key_jsv.is_pointer() && crate::symbol::js_is_symbol(key_value) == 0) { + return crate::value::js_jsvalue_to_string(key_value); + } + let obj_ptr = (key_value.to_bits() & crate::value::POINTER_MASK) as usize; + if !(obj_ptr >= 0x10000 && is_valid_obj_ptr(obj_ptr as *const u8)) { + return crate::value::js_jsvalue_to_string(key_value); } - crate::value::js_jsvalue_to_string(key_value) + // #6935: the null-proto pre-check below is itself GC-capable — the two + // `js_string_from_bytes` calls allocate, and a `toString` / `valueOf` + // stored as an accessor makes `js_object_get_field_by_name` run user JS. + // Any of those can evacuate the key object, so the receiver pointer (and + // the two freshly allocated name strings) must be re-derived through + // handles after each step instead of reusing the raw `obj_ptr` captured on + // entry. The final `js_jsvalue_to_string` — the real coercion — likewise + // has to see the *current* address of the key. + let scope = crate::gc::RuntimeHandleScope::new(); + let key_handle = scope.root_nanbox_f64(key_value); + let to_string_key = scope.root_string_ptr(crate::string::js_string_from_bytes( + b"toString".as_ptr(), + b"toString".len() as u32, + )); + let value_of_key = scope.root_string_ptr(crate::string::js_string_from_bytes( + b"valueOf".as_ptr(), + b"valueOf".len() as u32, + )); + let live_obj = |handle: &crate::gc::RuntimeHandle<'_>| -> *const ObjectHeader { + (handle.get_nanbox_f64().to_bits() & crate::value::POINTER_MASK) as *const ObjectHeader + }; + let to_string = js_object_get_field_by_name( + live_obj(&key_handle), + to_string_key.get_raw_const_ptr::(), + ); + let value_of = js_object_get_field_by_name( + live_obj(&key_handle), + value_of_key.get_raw_const_ptr::(), + ); + let gc = gc_header_for(live_obj(&key_handle)); + if ((*gc)._reserved & crate::gc::OBJ_FLAG_NULL_PROTO) != 0 + && to_string.is_undefined() + && value_of.is_undefined() + { + throw_object_type_error(b"Cannot convert object to primitive value"); + } + crate::value::js_jsvalue_to_string(key_handle.get_nanbox_f64()) } #[no_mangle] @@ -54,12 +78,36 @@ pub unsafe extern "C" fn js_object_literal_set_computed( if crate::symbol::js_is_symbol(key_value) != 0 { return crate::symbol::js_object_set_symbol_property(obj_value, key_value, value); } + if super::property_key_coercion_is_inert(key_value) { + let key_str = object_literal_key_to_string(key_value); + if key_str.is_null() { + return value; + } + mark_object_dynamic_shape_unknown(obj); + js_object_set_field_by_name(obj, key_str, value); + return value; + } + // #6935: `object_literal_key_to_string` runs the user key coercion, so it + // can allocate → GC → evacuate. The receiver `obj` and the `value` about to + // be stored in it were both raw across that call; a stale receiver dropped + // the write on a forwarding stub and a stale `value` planted a dangling + // pointer inside a live object. + let scope = crate::gc::RuntimeHandleScope::new(); + let obj_handle = scope.root_raw_mut_ptr(obj); + let value_handle = scope.root_nanbox_f64(value); let key_str = object_literal_key_to_string(key_value); if key_str.is_null() { - return value; + return value_handle.get_nanbox_f64(); } + let key_handle = scope.root_string_ptr(key_str); + let obj = obj_handle.get_raw_mut_ptr::(); + let value = value_handle.get_nanbox_f64(); mark_object_dynamic_shape_unknown(obj); - js_object_set_field_by_name(obj, key_str, value); + js_object_set_field_by_name( + obj, + key_handle.get_raw_const_ptr::(), + value, + ); value } @@ -98,53 +146,85 @@ pub extern "C" fn js_object_define_accessor( setter: f64, ) -> f64 { unsafe { - let obj = extract_obj_ptr(obj_value); - if obj.is_null() { + if extract_obj_ptr(obj_value).is_null() { return obj_value; } + // #6935: `js_to_property_key` runs the user key coercion, + // `object_literal_key_to_string` may allocate, `ensure_key_in_keys_array` + // grows the keys array and `clone_closure_rebind_this` allocates the + // bound copies — every one of them can GC and evacuate. The receiver + // AND the two accessor closures being installed were raw locals across + // all of that, so a stale getter/setter would be recorded in the + // descriptor table under the object's (possibly also stale) address. + let scope = crate::gc::RuntimeHandleScope::new(); + let obj_value_handle = scope.root_heap_word_u64(obj_value.to_bits()); + let getter_handle = scope.root_nanbox_f64(getter); + let setter_handle = scope.root_nanbox_f64(setter); let key_value = js_to_property_key(key_value); + let obj_value = f64::from_bits(obj_value_handle.get_heap_word_u64()); if crate::symbol::js_is_symbol(key_value) != 0 { return crate::symbol::js_object_define_symbol_accessor( - obj_value, key_value, getter, setter, + obj_value, + key_value, + getter_handle.get_nanbox_f64(), + setter_handle.get_nanbox_f64(), ); } let key_str = object_literal_key_to_string(key_value); if key_str.is_null() { - return obj_value; + return f64::from_bits(obj_value_handle.get_heap_word_u64()); } + let key_handle = scope.root_string_ptr(key_str); + let obj = extract_obj_ptr(f64::from_bits(obj_value_handle.get_heap_word_u64())); mark_object_dynamic_shape_unknown(obj); let key_rust: Option = { + let key_str = key_handle.get_raw_const_ptr::(); let name_ptr = (key_str as *const u8).add(std::mem::size_of::()); let name_len = (*key_str).byte_len as usize; let name_bytes = std::slice::from_raw_parts(name_ptr, name_len); std::str::from_utf8(name_bytes).ok().map(|s| s.to_string()) }; - super::object_ops::ensure_key_in_keys_array(obj, key_str); + super::object_ops::ensure_key_in_keys_array( + obj, + key_handle.get_raw_const_ptr::() as *mut crate::StringHeader, + ); + let obj_value = f64::from_bits(obj_value_handle.get_heap_word_u64()); let Some(k) = key_rust else { return obj_value; }; - let recv_box = crate::value::js_nanbox_pointer(obj as i64); + let obj = extract_obj_ptr(obj_value); let existing = get_accessor_descriptor(obj as usize, &k).unwrap_or_default(); + // The previously installed accessor pair is a pair of NaN-boxed closure + // pointers too — they are carried across the `clone_closure_rebind_this` + // allocations below, so root them as well. + let existing_get = scope.root_nanbox_u64(existing.get); + let existing_set = scope.root_nanbox_u64(existing.set); let undef = crate::value::TAG_UNDEFINED; - let get_bits = if getter.to_bits() == undef { - existing.get + let recv_box = crate::value::js_nanbox_pointer(obj as i64); + let get_bits = if getter_handle.get_nanbox_u64() == undef { + existing_get.get_nanbox_u64() } else { - crate::closure::clone_closure_rebind_this(getter.to_bits(), recv_box) + crate::closure::clone_closure_rebind_this(getter_handle.get_nanbox_u64(), recv_box) }; - let set_bits = if setter.to_bits() == undef { - existing.set + let get_bits_handle = scope.root_nanbox_u64(get_bits); + let recv_box = crate::value::js_nanbox_pointer(extract_obj_ptr(f64::from_bits( + obj_value_handle.get_heap_word_u64(), + )) as i64); + let set_bits = if setter_handle.get_nanbox_u64() == undef { + existing_set.get_nanbox_u64() } else { - crate::closure::clone_closure_rebind_this(setter.to_bits(), recv_box) + crate::closure::clone_closure_rebind_this(setter_handle.get_nanbox_u64(), recv_box) }; + let obj = extract_obj_ptr(f64::from_bits(obj_value_handle.get_heap_word_u64())); set_accessor_descriptor( obj as usize, k.clone(), AccessorDescriptor { - get: get_bits, + get: get_bits_handle.get_nanbox_u64(), set: set_bits, }, ); set_property_attrs(obj as usize, k, PropertyAttrs::new(true, true, true)); - obj_value + f64::from_bits(obj_value_handle.get_heap_word_u64()) } } diff --git a/crates/perry-runtime/src/object/object_ops/has_own.rs b/crates/perry-runtime/src/object/object_ops/has_own.rs index f8982afc82..dd4ab07d8a 100644 --- a/crates/perry-runtime/src/object/object_ops/has_own.rs +++ b/crates/perry-runtime/src/object/object_ops/has_own.rs @@ -103,7 +103,25 @@ pub extern "C" fn js_object_has_own(obj_value: f64, key_value: f64) -> f64 { // ToPropertyKey(V): fold an object argument (e.g. one whose `toString` // returns a Symbol) into its canonical key before the symbol/string // split. A no-op for keys that are already primitives. - let key_value = super::super::js_to_property_key(key_value); + // + // #6935: for an object key that fold runs USER JS, which allocates and + // can trigger a GC that **evacuates** the receiver. `obj_value` — and + // the `obj_js` tag view taken from it above — were raw NaN-boxed Rust + // locals across the call, so root the receiver and re-derive both. + let (obj_value, obj_js, key_value) = + if super::super::property_key_coercion_is_inert(key_value) { + (obj_value, obj_js, key_value) + } else { + let scope = crate::gc::RuntimeHandleScope::new(); + let obj_handle = scope.root_heap_word_u64(obj_value.to_bits()); + let key_value = super::super::js_to_property_key(key_value); + let obj_value = f64::from_bits(obj_handle.get_heap_word_u64()); + ( + obj_value, + crate::JSValue::from_bits(obj_value.to_bits()), + key_value, + ) + }; // A Proxy is a small registered id, not a heap object — route // `hasOwnProperty` through `[[GetOwnProperty]]` (a present own property @@ -428,7 +446,17 @@ pub extern "C" fn js_object_property_is_enumerable(obj_value: f64, key_value: f6 // ToPropertyKey(V): fold an object argument (e.g. one whose `toString` // returns a Symbol) into its canonical key before the symbol/string // split. A no-op for keys that are already primitives. - let key_value = super::super::js_to_property_key(key_value); + // + // #6935: root the receiver across the (GC-capable) fold — see + // `js_object_has_own` above for the full reasoning. + let (obj_value, key_value) = if super::super::property_key_coercion_is_inert(key_value) { + (obj_value, key_value) + } else { + let scope = crate::gc::RuntimeHandleScope::new(); + let obj_handle = scope.root_heap_word_u64(obj_value.to_bits()); + let key_value = super::super::js_to_property_key(key_value); + (f64::from_bits(obj_handle.get_heap_word_u64()), key_value) + }; // Proxy receiver: resolve the descriptor via `[[GetOwnProperty]]` and // report its `enumerable` attribute (absent property → false) rather diff --git a/crates/perry-runtime/src/object/polymorphic_index.rs b/crates/perry-runtime/src/object/polymorphic_index.rs index 061ffdcc69..66f25989ea 100644 --- a/crates/perry-runtime/src/object/polymorphic_index.rs +++ b/crates/perry-runtime/src/object/polymorphic_index.rs @@ -18,6 +18,52 @@ unsafe fn property_key_string_ptr(value: f64) -> *mut crate::StringHeader { crate::value::js_jsvalue_to_string(key) } +/// `obj[key]` READ through a non-canonical (object / exotic) key, with the +/// receiver rooted across the coercion (#6935). +/// +/// Both entry points below reach the key coercion only on their +/// NON-canonical-key arms — which is exactly where an object key lands. +/// `ToPropertyKey` then runs a user `Symbol.toPrimitive` / `toString` / +/// `valueOf`, allocates, and can trigger a GC that **evacuates** the receiver. +/// `raw` is a bare `u64` address in a Rust local — not a GC root and not a +/// shadow slot — so it has to be re-read through a handle afterwards. +unsafe fn rooted_property_key_get(raw: u64, idx: f64) -> f64 { + let scope = crate::gc::RuntimeHandleScope::new(); + let recv = scope.root_raw_mut_ptr(raw as *mut ObjectHeader); + let key = property_key_string_ptr(idx); + if key.is_null() { + return f64::from_bits(crate::value::TAG_UNDEFINED); + } + let key_handle = scope.root_string_ptr(key); + let v = js_object_get_field_by_name( + recv.get_raw_mut_ptr::(), + key_handle.get_raw_const_ptr::(), + ); + f64::from_bits(v.bits()) +} + +/// `obj[key] = value` WRITE counterpart of [`rooted_property_key_get`]. +/// +/// This is the corruption half: the coercion sits between the receiver/value +/// arriving and the store, so pre-fix a stale receiver dropped the write onto a +/// forwarding stub and a stale `value` planted a dangling pointer *inside* a +/// live object, outliving the call. +unsafe fn rooted_property_key_set(raw: u64, idx: f64, value: f64) { + let scope = crate::gc::RuntimeHandleScope::new(); + let recv = scope.root_raw_mut_ptr(raw as *mut ObjectHeader); + let value_handle = scope.root_nanbox_f64(value); + let key = property_key_string_ptr(idx); + if key.is_null() { + return; + } + let key_handle = scope.root_string_ptr(key); + js_object_set_field_by_name( + recv.get_raw_mut_ptr::(), + key_handle.get_raw_const_ptr::(), + value_handle.get_nanbox_f64(), + ); +} + fn numeric_key_u32_index(value: f64) -> Option { let bits = value.to_bits(); if (bits & crate::value::TAG_MASK) == crate::value::INT32_TAG { @@ -170,21 +216,11 @@ pub extern "C" fn js_object_get_index_polymorphic(obj_handle: i64, idx: f64) -> if let Some(index) = numeric_key_u32_index(idx) { return crate::array::js_array_get_f64(raw as *mut crate::array::ArrayHeader, index); } else { - let key = unsafe { property_key_string_ptr(idx) }; - if key.is_null() { - return f64::from_bits(crate::value::TAG_UNDEFINED); - } - let v = js_object_get_field_by_name(raw as *mut ObjectHeader, key); - return f64::from_bits(v.bits()); + return unsafe { rooted_property_key_get(raw, idx) }; } } if gc_type == crate::gc::GC_TYPE_OBJECT || gc_type == crate::gc::GC_TYPE_CLOSURE { - let key = unsafe { property_key_string_ptr(idx) }; - if key.is_null() { - return f64::from_bits(crate::value::TAG_UNDEFINED); - } - let v = js_object_get_field_by_name(raw as *mut ObjectHeader, key); - return f64::from_bits(v.bits()); + return unsafe { rooted_property_key_get(raw, idx) }; } if crate::set::is_registered_set(raw as usize) || crate::map::is_registered_map(raw as usize) { let Some(index) = numeric_key_u32_index(idx) else { @@ -315,10 +351,7 @@ pub extern "C" fn js_object_set_index_polymorphic(obj_handle: i64, idx: f64, val ); return; } else { - let key = unsafe { property_key_string_ptr(idx) }; - if !key.is_null() { - js_object_set_field_by_name(raw as *mut ObjectHeader, key, value); - } + unsafe { rooted_property_key_set(raw, idx, value) }; return; } } @@ -326,10 +359,7 @@ pub extern "C" fn js_object_set_index_polymorphic(obj_handle: i64, idx: f64, val // Stringify the index and route through the object field setter, // which handles shape transitions, frozen/sealed/extensible checks, // overflow into out-of-line storage, and accessor descriptors. - let key = unsafe { property_key_string_ptr(idx) }; - if !key.is_null() { - js_object_set_field_by_name(raw as *mut ObjectHeader, key, value); - } + unsafe { rooted_property_key_set(raw, idx, value) }; return; } // Buffer / typed-array were handled above. Map / Set are collection diff --git a/crates/perry-runtime/src/object/property_key.rs b/crates/perry-runtime/src/object/property_key.rs index e6d6330a48..cae2a54b10 100644 --- a/crates/perry-runtime/src/object/property_key.rs +++ b/crates/perry-runtime/src/object/property_key.rs @@ -44,6 +44,45 @@ pub unsafe extern "C" fn js_to_property_key(value: f64) -> f64 { crate::value::js_nanbox_string(key as i64) } +/// True when [`js_to_property_key`] provably neither allocates nor calls back +/// into user JS for `key`, so a caller may hold a raw receiver / stored value +/// across it without a [`RuntimeHandleScope`] (#6935). +/// +/// Only an already-heap `STRING_TAG` key qualifies. Walk the coercion for one: +/// `js_is_symbol` is a tag/side-table test, `js_to_primitive` returns any +/// non-`POINTER_TAG` value unchanged, `ordinary_to_primitive_string_key` bails +/// immediately (`extract_obj_ptr` is null for a tagged string), and +/// `js_jsvalue_to_string` hands the very same pointer back. Nothing allocates. +/// +/// Every other key shape does: numbers / booleans / `null` / `undefined` / +/// BigInt allocate their stringification, SSO short strings materialize onto +/// the heap, and object keys can invoke a user `Symbol.toPrimitive` / +/// `toString` / `valueOf`. Any of those can trigger a GC that **evacuates** +/// live objects — moving the caller's receiver and the value it is about to +/// store — so the callers below must root across the coercion instead. +/// +/// [`RuntimeHandleScope`]: crate::gc::RuntimeHandleScope +#[inline] +pub(crate) fn property_key_coercion_is_inert(key: f64) -> bool { + (key.to_bits() & 0xFFFF_0000_0000_0000) == crate::value::STRING_TAG +} + +/// `ToPropertyKey(key)` performed inside an existing `scope`, with both the +/// incoming key and the coerced result rooted (#6935). +/// +/// Callers must root their receiver — and any value they are about to store — +/// in the SAME scope *before* calling this, then read those back through their +/// handles: this coercion is the GC-capable step that invalidates raw locals. +#[inline] +pub(crate) unsafe fn to_property_key_rooted<'scope>( + scope: &'scope crate::gc::RuntimeHandleScope, + key: f64, +) -> crate::gc::RuntimeHandle<'scope> { + let key_handle = scope.root_nanbox_f64(key); + let coerced = js_to_property_key(key_handle.get_nanbox_f64()); + scope.root_nanbox_f64(coerced) +} + /// True when `v` is not a JS Object — i.e. a usable primitive result from /// `OrdinaryToPrimitive` (undefined/null/boolean/number/string/bigint and, /// crucially, **Symbol**, the one `POINTER_TAG` primitive). @@ -102,13 +141,43 @@ unsafe fn ordinary_to_primitive_string_key(value: f64) -> Option { } /// `obj[ToPropertyKey(key)] = value` for object-literal computed definitions. +/// +/// #6935: `ToPropertyKey` can run a user `Symbol.toPrimitive` / `toString` / +/// `valueOf`, which allocates → GC → **evacuation**. Pre-fix `obj_value` (the +/// receiver) and `value` (what is about to be written *into* it) were raw +/// NaN-boxed Rust locals held across that call — neither a GC root nor a +/// shadow slot. A stale receiver dropped the write onto a forwarding stub, and +/// a stale `value` planted a dangling pointer inside a live object, so the +/// corruption outlived the call. Root both before the coercion and read them +/// back through their handles afterwards. #[no_mangle] pub unsafe extern "C" fn js_object_set_property_key( obj_value: f64, key_value: f64, value: f64, ) -> f64 { - let key = js_to_property_key(key_value); + if property_key_coercion_is_inert(key_value) { + return set_property_key_resolved(obj_value, key_value, value); + } + let scope = crate::gc::RuntimeHandleScope::new(); + // `obj_value` may arrive NaN-boxed OR as a bare heap address (module-level + // object slots store the untagged pointer) OR as an INT32 class-ref — the + // heap-word slot kind covers all three, rewriting only the real pointers + // and preserving each encoding. + let obj_handle = scope.root_heap_word_u64(obj_value.to_bits()); + let value_handle = scope.root_nanbox_f64(value); + let key_handle = to_property_key_rooted(&scope, key_value); + set_property_key_resolved( + f64::from_bits(obj_handle.get_heap_word_u64()), + key_handle.get_nanbox_f64(), + value_handle.get_nanbox_f64(), + ) +} + +/// Post-`ToPropertyKey` half of [`js_object_set_property_key`]. `key` is +/// already a Symbol or a heap string here, so nothing below can run user JS. +#[inline] +unsafe fn set_property_key_resolved(obj_value: f64, key: f64, value: f64) -> f64 { if crate::symbol::js_is_symbol(key) != 0 { return crate::symbol::js_object_set_symbol_property(obj_value, key, value); } @@ -134,9 +203,26 @@ pub unsafe extern "C" fn js_object_set_property_key( } /// `obj[ToPropertyKey(key)]` using Perry's string and symbol property stores. +/// +/// #6935: the receiver is rooted across the GC-capable key coercion — see +/// [`js_object_set_property_key`] for the full reasoning. #[no_mangle] pub unsafe extern "C" fn js_object_get_property_key(obj_value: f64, key_value: f64) -> f64 { - let key = js_to_property_key(key_value); + if property_key_coercion_is_inert(key_value) { + return get_property_key_resolved(obj_value, key_value); + } + let scope = crate::gc::RuntimeHandleScope::new(); + let obj_handle = scope.root_heap_word_u64(obj_value.to_bits()); + let key_handle = to_property_key_rooted(&scope, key_value); + get_property_key_resolved( + f64::from_bits(obj_handle.get_heap_word_u64()), + key_handle.get_nanbox_f64(), + ) +} + +/// Post-`ToPropertyKey` half of [`js_object_get_property_key`]. +#[inline] +unsafe fn get_property_key_resolved(obj_value: f64, key: f64) -> f64 { if crate::symbol::js_is_symbol(key) != 0 { return crate::symbol::js_object_get_symbol_property(obj_value, key); } @@ -163,13 +249,33 @@ pub unsafe extern "C" fn js_object_get_property_key(obj_value: f64, key_value: f /// Install an object-literal method under a computed property key and bind the /// method's reserved `this` capture slot to the home object. +/// +/// #6935: the home object AND the closure being installed are both rooted +/// across the GC-capable key coercion — the closure is the "stored value" here, +/// so a stale one would be written into the object. #[no_mangle] pub unsafe extern "C" fn js_object_set_property_key_method( obj_value: f64, key_value: f64, closure: f64, ) -> f64 { - let key = js_to_property_key(key_value); + if property_key_coercion_is_inert(key_value) { + return set_property_key_method_resolved(obj_value, key_value, closure); + } + let scope = crate::gc::RuntimeHandleScope::new(); + let obj_handle = scope.root_heap_word_u64(obj_value.to_bits()); + let closure_handle = scope.root_nanbox_f64(closure); + let key_handle = to_property_key_rooted(&scope, key_value); + set_property_key_method_resolved( + f64::from_bits(obj_handle.get_heap_word_u64()), + key_handle.get_nanbox_f64(), + closure_handle.get_nanbox_f64(), + ) +} + +/// Post-`ToPropertyKey` half of [`js_object_set_property_key_method`]. +#[inline] +unsafe fn set_property_key_method_resolved(obj_value: f64, key: f64, closure: f64) -> f64 { if crate::symbol::js_is_symbol(key) != 0 { return crate::symbol::js_object_set_symbol_method(obj_value, key, closure); } @@ -223,7 +329,16 @@ pub unsafe extern "C" fn js_super_accessor_get( key: f64, receiver: f64, ) -> f64 { - let key_hdr = crate::builtins::js_string_coerce(key); + // #6935: `js_string_coerce` on an object key runs a user `toString` / + // `valueOf` (and allocates even for primitive keys), so it can GC and + // evacuate. `receiver` is dereferenced far below (`class_ref_id`, the + // getter's `this`) and `key` is re-read at the prototype fallback, so both + // must survive the coercion through handles rather than as raw locals. + let scope = crate::gc::RuntimeHandleScope::new(); + let receiver_handle = scope.root_heap_word_u64(receiver.to_bits()); + let key_handle = scope.root_nanbox_f64(key); + let key_hdr = crate::builtins::js_string_coerce(key_handle.get_nanbox_f64()); + let receiver = f64::from_bits(receiver_handle.get_heap_word_u64()); let key_name: Option = if key_hdr.is_null() { None } else { @@ -331,7 +446,7 @@ pub unsafe extern "C" fn js_super_accessor_get( } if !proto.is_null() { let target = crate::value::js_nanbox_pointer(proto as i64); - return js_object_get_property_key(target, key); + return js_object_get_property_key(target, key_handle.get_nanbox_f64()); } f64::from_bits(crate::value::TAG_UNDEFINED) } @@ -376,13 +491,26 @@ pub unsafe extern "C" fn js_object_super_call( args_ptr: *const f64, args_len: usize, ) -> f64 { + // #6935: `js_object_super_get` performs the GC-capable `ToPropertyKey`, and + // `clone_closure_rebind_this` allocates the bound copy. `receiver` — the + // `this` the bound method runs with — was raw across both. + let scope = crate::gc::RuntimeHandleScope::new(); + let receiver_handle = scope.root_heap_word_u64(receiver.to_bits()); let callee = js_object_super_get(home, key_value, receiver); if callee.to_bits() == crate::value::TAG_UNDEFINED { return callee; } - let bound = crate::closure::clone_closure_rebind_this(callee.to_bits(), receiver); + let callee_handle = scope.root_nanbox_f64(callee); + let receiver = f64::from_bits(receiver_handle.get_heap_word_u64()); + let bound = crate::closure::clone_closure_rebind_this(callee_handle.get_nanbox_u64(), receiver); + let bound_handle = scope.root_nanbox_u64(bound); + let receiver = f64::from_bits(receiver_handle.get_heap_word_u64()); let prev_this = crate::object::js_implicit_this_set(receiver); - let result = crate::closure::js_native_call_value(f64::from_bits(bound), args_ptr, args_len); + let result = crate::closure::js_native_call_value( + f64::from_bits(bound_handle.get_nanbox_u64()), + args_ptr, + args_len, + ); crate::object::js_implicit_this_set(prev_this); result } diff --git a/crates/perry-runtime/src/proxy.rs b/crates/perry-runtime/src/proxy.rs index 2cd5250dde..567b81d38e 100644 --- a/crates/perry-runtime/src/proxy.rs +++ b/crates/perry-runtime/src/proxy.rs @@ -804,7 +804,20 @@ pub(crate) fn reflect_ordinary_set_with_receiver( } fn target_set(target: f64, key: f64, value: f64) { + // #6935: `js_to_property_key` runs a user `Symbol.toPrimitive` / `toString` + // / `valueOf` for an object key (and allocates for every primitive one), so + // it can trigger a GC that **evacuates** live objects. Both the `target` + // receiver and the `value` being written into it were raw NaN-boxed Rust + // locals across the call — a stale target dropped the write onto a + // forwarding stub, a stale value stored a dangling pointer in a live + // object. `target_get` already roots; this write sibling did not. + let scope = crate::gc::RuntimeHandleScope::new(); + let target_handle = scope.root_heap_word_u64(target.to_bits()); + let value_handle = scope.root_nanbox_f64(value); let property_key = unsafe { crate::object::js_to_property_key(key) }; + let property_key = scope.root_nanbox_f64(property_key).get_nanbox_f64(); + let target = f64::from_bits(target_handle.get_heap_word_u64()); + let value = value_handle.get_nanbox_f64(); if unsafe { crate::symbol::js_is_symbol(property_key) } != 0 { unsafe { crate::symbol::js_object_set_symbol_property(target, property_key, value); diff --git a/crates/perry-runtime/src/typed_feedback.rs b/crates/perry-runtime/src/typed_feedback.rs index 895aac03aa..bc6d499935 100644 --- a/crates/perry-runtime/src/typed_feedback.rs +++ b/crates/perry-runtime/src/typed_feedback.rs @@ -2435,15 +2435,26 @@ pub extern "C" fn js_typed_feedback_array_index_set_fallback_boxed( crate::value::js_nanbox_pointer(new_arr as i64) } crate::gc::GC_TYPE_OBJECT | crate::gc::GC_TYPE_CLOSURE => { + // #6935: `js_jsvalue_to_string` on an object index runs a user + // `toString` / `valueOf` (and allocates for every primitive + // one), so it can trigger a GC that **evacuates**. The receiver + // `raw_addr` and the `value` being stored were raw Rust locals + // across it — a stale receiver dropped the write onto a + // forwarding stub and a stale `value` planted a dangling + // pointer inside a live object. + let scope = crate::gc::RuntimeHandleScope::new(); + let recv = scope.root_raw_mut_ptr(raw_addr as *mut ObjectHeader); + let value_handle = scope.root_nanbox_f64(value); + let receiver_handle = scope.root_heap_word_u64(receiver.to_bits()); let key_ptr = crate::value::js_jsvalue_to_string(index); if !key_ptr.is_null() { crate::object::js_object_set_field_by_name( - raw_addr as *mut ObjectHeader, + recv.get_raw_mut_ptr::(), key_ptr, - value, + value_handle.get_nanbox_f64(), ); } - receiver + f64::from_bits(receiver_handle.get_heap_word_u64()) } _ => receiver, } diff --git a/crates/perry-runtime/src/value/dyn_index.rs b/crates/perry-runtime/src/value/dyn_index.rs index 4ae5307861..2a8fe80f6c 100644 --- a/crates/perry-runtime/src/value/dyn_index.rs +++ b/crates/perry-runtime/src/value/dyn_index.rs @@ -337,9 +337,15 @@ pub extern "C" fn js_dyn_index_get(value: f64, index: f64) -> f64 { } else { format!("{}", index) }; + // #6935: `js_string_from_bytes` ALLOCATES, so the numeric→string key + // conversion can trigger a GC that evacuates the receiver. `raw_ptr` + // is a bare Rust local — neither a root nor a shadow slot — so it + // must be re-read through a handle after the allocation. + let scope = crate::gc::RuntimeHandleScope::new(); + let recv = scope.root_raw_mut_ptr(raw_ptr as *mut crate::object::ObjectHeader); let key = crate::string::js_string_from_bytes(s.as_ptr(), s.len() as u32); let v = crate::object::js_object_get_field_by_name_f64( - raw_ptr as *const crate::object::ObjectHeader, + recv.get_raw_const_ptr::(), key, ); // An indexed property inherited from the canonical @@ -428,11 +434,26 @@ pub extern "C" fn js_dyn_index_set(obj: f64, index: f64, value: f64) -> f64 { // method as non-writable. (Mirrors the get arm above.) if (bits >> 48) == 0x7FFE { let idx_top16 = index.to_bits() >> 48; - let key_ptr = if idx_top16 == 0x7FFF || idx_top16 == 0x7FF9 { - js_get_string_pointer_unified(index) as *const crate::StringHeader - } else { - crate::builtins::js_string_coerce(index) as *const crate::StringHeader - }; + if idx_top16 == 0x7FFF || idx_top16 == 0x7FF9 { + let key_ptr = js_get_string_pointer_unified(index) as *const crate::StringHeader; + if !key_ptr.is_null() { + crate::object::js_object_set_field_by_name( + bits as *mut crate::object::ObjectHeader, + key_ptr, + value, + ); + } + return value; + } + // #6935: `js_string_coerce` on an object index runs a user `toString` / + // `valueOf` (and allocates even for primitive indices), so it can GC and + // evacuate. The receiver here is an INT32 class-ref — not a heap object, + // so it cannot move — but `value` IS the thing being written into the + // class's dynamic-prop table, and it was a raw local across the coercion. + let scope = crate::gc::RuntimeHandleScope::new(); + let value_handle = scope.root_nanbox_f64(value); + let key_ptr = crate::builtins::js_string_coerce(index) as *const crate::StringHeader; + let value = value_handle.get_nanbox_f64(); if !key_ptr.is_null() { crate::object::js_object_set_field_by_name( bits as *mut crate::object::ObjectHeader, @@ -539,19 +560,41 @@ pub extern "C" fn js_dyn_index_set(obj: f64, index: f64, value: f64) -> f64 { // Non-array object: stringify the index and write via the object setter. let bits = index.to_bits(); let top16 = bits >> 48; - let key_ptr: *const crate::StringHeader = if top16 == 0x7FFF { - (bits & 0x0000_FFFF_FFFF_FFFF) as *const crate::StringHeader - } else if top16 == 0x7FF9 { - crate::value::js_get_string_pointer_unified(index) as *const crate::StringHeader - } else { - crate::value::js_jsvalue_to_string(index) - }; + if top16 == 0x7FFF || top16 == 0x7FF9 { + let key_ptr: *const crate::StringHeader = if top16 == 0x7FFF { + (bits & 0x0000_FFFF_FFFF_FFFF) as *const crate::StringHeader + } else { + crate::value::js_get_string_pointer_unified(index) as *const crate::StringHeader + }; + if key_ptr.is_null() { + return value; + } + crate::object::js_object_set_field_by_name( + raw_ptr as *mut crate::object::ObjectHeader, + key_ptr, + value, + ); + return value; + } + // #6935: this is the corruption case. `js_jsvalue_to_string(index)` runs a + // user `toString` / `valueOf` for an object index (`obj[{toString(){...}}] = v`) + // and allocates for every other shape, so it can GC and EVACUATE. Both the + // receiver `raw_ptr` and the `value` being stored were raw Rust locals + // across it: a stale receiver dropped the write onto a forwarding stub, and + // a stale `value` wrote a dangling pointer INTO a live object, where it + // outlives the call. + let scope = crate::gc::RuntimeHandleScope::new(); + let recv = scope.root_raw_mut_ptr(raw_ptr as *mut crate::object::ObjectHeader); + let value_handle = scope.root_nanbox_f64(value); + let key_ptr = crate::value::js_jsvalue_to_string(index); + let value = value_handle.get_nanbox_f64(); if key_ptr.is_null() { return value; } + let key_handle = scope.root_string_ptr(key_ptr); crate::object::js_object_set_field_by_name( - raw_ptr as *mut crate::object::ObjectHeader, - key_ptr, + recv.get_raw_mut_ptr::(), + key_handle.get_raw_const_ptr::(), value, ); value diff --git a/crates/perry/tests/gc_property_key_operand_rooting_6935.rs b/crates/perry/tests/gc_property_key_operand_rooting_6935.rs new file mode 100644 index 0000000000..135af68d7f --- /dev/null +++ b/crates/perry/tests/gc_property_key_operand_rooting_6935.rs @@ -0,0 +1,422 @@ +//! Regression tests for #6935 — raw receivers and raw *stored values* held +//! across GC-capable property-key coercions. +//! +//! `ToPropertyKey(key)` runs a user `Symbol.toPrimitive` / `toString` / +//! `valueOf`, and even for a primitive key it allocates the stringified form. +//! Either can trigger a GC that **evacuates** (moves) live objects. Pre-fix the +//! property-key entry points held the receiver — and, on the set paths, the +//! value being written *into* it — as raw `f64` / raw pointer Rust locals: +//! +//! ```ignore +//! let key = js_to_property_key(key_value); // user JS -> allocate -> GC +//! let obj = extract_obj_ptr(obj_value); // receiver was raw across it +//! js_object_set_field_by_name(obj, key_str, value); // stale receiver AND value +//! ``` +//! +//! A Rust local is neither a GC root nor a shadow slot. This is strictly worse +//! than the #6655 operator family: there a stale operand produced one wrong +//! answer, whereas here the stale `value` is **written into a live object**, so +//! the dangling pointer outlives the call. +//! +//! The programs run with `PERRY_GC_FORCE_EVACUATE=1` (stress-copies every +//! marked non-pinned nursery object) and `PERRY_GC_VERIFY_EVACUATION=1` (panics +//! if a live slot still points at a forwarded object). Every stored payload is +//! a heap object whose fields are read back **after a further collection**, so +//! a stale store shows up as a wrong field value rather than passing by luck. +//! +//! ## What this suite does and does not prove +//! +//! Be precise about this, because the suite passes on the PRE-fix runtime too. +//! No in-language configuration currently reaches the state the bug needs — a +//! *minor* cycle that evacuates while the raw runtime locals are unpinned: +//! +//! * The `gc()` hook these programs call runs a **full mark-sweep**. Evacuation +//! is minor-only, so nothing moves (`PERRY_GC_DIAG=1` prints no +//! `[gc-evac-policy]`/`[gc-copy-minor]` line for any cycle here), and a stale +//! pointer is trivially still valid. +//! * `perry/gc`'s `minor()` does evacuate (diag shows `reason=force`, +//! `moved_objects` in the thousands) but engages +//! `ManualGcScanGuard::force_full_scan()` (#4977). The conservative stack scan +//! then **pins exactly the raw receiver/value locals this bug is about**, so +//! it is masked by construction. +//! * `minor()` with `PERRY_CONSERVATIVE_STACK_SCAN=off` does evacuate them, but +//! that combination is independently unsound on this build — a plain method's +//! `this` (and even a `console.log` string literal) is lost across the +//! collection, with or without the fix — so any failure it produces is +//! uninterpretable. +//! +//! So these tests are a **behavioral guard**: they pin the observable semantics +//! of every rooted path (right value stored, right value read back, right key) +//! under the strongest GC stress the language surface can express today, and +//! they will start failing the day a minor-evacuating configuration becomes +//! reachable from compiled code. They are not evidence that the pre-fix runtime +//! was reproducibly wrong; the audit in #6935 is. +//! +//! Coverage: `js_object_{set,get}_property_key`, +//! `js_object_set_property_key_method`, `js_object_literal_set_computed`, +//! `js_object_define_accessor`, `js_dyn_index_{get,set}`, +//! `js_object_{get,set}_index_polymorphic`, `js_object_delete_dynamic`, +//! `js_array_{get,set}_index_or_string`, `js_native_call_method_value`, +//! `js_object_has_own` / `js_object_property_is_enumerable` (and their +//! `common_methods` method-call forms), `js_object_has_property`, and the proxy +//! `target_set` forward-to-target write. + +use std::path::PathBuf; +use std::process::Command; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +fn compile_and_run_forced_evacuation(dir: &std::path::Path, source: &str) -> String { + let entry = dir.join("main.ts"); + let output = dir.join("main_bin"); + std::fs::write(&entry, source).expect("write entry"); + + // The runtime-only macOS link path does not pass `-framework CoreFoundation`, + // but `perry-runtime` pulls `iana_time_zone`, which references `_CFRelease` + // & co. Append the framework through the supported escape hatch so this + // suite links regardless (same shim as the #6655 suite). + let mut compile_cmd = Command::new(perry_bin()); + compile_cmd + .current_dir(dir) + .arg("compile") + .arg(&entry) + .arg("-o") + .arg(&output) + .arg("--no-cache"); + if cfg!(target_os = "macos") { + let extra = match std::env::var("PERRY_EXTRA_LINK_ARGS") { + Ok(existing) if !existing.trim().is_empty() => { + format!("{existing} -framework CoreFoundation") + } + _ => "-framework CoreFoundation".to_string(), + }; + compile_cmd.env("PERRY_EXTRA_LINK_ARGS", extra); + } + let compile = compile_cmd.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) + .current_dir(dir) + .env("PERRY_GC_FORCE_EVACUATE", "1") + .env("PERRY_GC_VERIFY_EVACUATION", "1") + .output() + .expect("run compiled binary"); + assert!( + run.status.success(), + "compiled binary failed under forced evacuation (exit {:?})\nstdout:\n{}\nstderr:\n{}", + run.status.code(), + String::from_utf8_lossy(&run.stdout), + String::from_utf8_lossy(&run.stderr) + ); + String::from_utf8_lossy(&run.stdout).into_owned() +} + +/// Shared prelude: a key object whose `toString` churns the nursery and forces +/// a collection, plus a movable heap payload with readable fields so a stale +/// *stored* pointer is observable rather than coincidentally right. +const PRELUDE: &str = r#" +// Allocate a lot of short-lived nursery objects, force a collection, then +// refill: evacuation leaves the vacated region intact-but-dead, so a stale +// pointer read immediately after a copy usually still finds the original bytes. +// Re-filling gives that region a chance to be handed out and overwritten. +function churnAndCollect(): void { + let sink = 0; + for (let i = 0; i < 20000; i++) { + const tmp = { i, s: "pad" + i }; + sink += tmp.s.length > 0 ? 1 : 0; + } + if (sink !== 20000) throw new Error("churn miscounted"); + (globalThis as any).gc?.(); + for (let i = 0; i < 20000; i++) { + const tmp2 = { a: i, b: "fill" + i, c: [i, i + 1] }; + sink += tmp2.c[0] >= 0 ? 1 : 0; + } + if (sink !== 40000) throw new Error("refill miscounted"); +} + +// Keeps receivers / payloads reachable from a real GC root. An object reachable +// only through the (unrooted) raw local is DEAD at the collection, so it would +// merely be swept and a stale read might find intact bytes. Reachable objects +// are EVACUATED — the address genuinely changes and every rooted holder is +// rewritten, while the raw local is not. +const keepalive: any[] = []; + +// A property key whose ToPropertyKey runs user JS that allocates and collects. +function heavyKey(name: string): any { + const o: any = { + n: name, + toString(): string { + churnAndCollect(); + return this.n; + }, + }; + keepalive.push(o); + return o; +} + +// Same, via Symbol.toPrimitive rather than toString. +function heavyPrimKey(name: string): any { + const o: any = { n: name }; + o[Symbol.toPrimitive] = function (): string { + churnAndCollect(); + return this.n; + }; + keepalive.push(o); + return o; +} + +// The STORED VALUE: a movable heap object carrying an identity we read back +// after a further collection. A stale stored pointer either reads the wrong +// tag/array or trips PERRY_GC_VERIFY_EVACUATION on the next cycle. +function payload(tag: number): any { + const o: any = { tag: tag, arr: [tag, tag + 1], s: "payload-" + tag }; + keepalive.push(o); + return o; +} + +// A fresh receiver that is rooted (so it is evacuated, not swept). +function receiver(): any { + const o: any = { seed: 1 }; + keepalive.push(o); + return o; +} + +let failures = 0; +function check(name: string, got: any, want: any): void { + if (got !== want) { + failures++; + console.log("FAIL " + name + " got=" + String(got) + " want=" + String(want)); + } +} +// Read every field of a stored payload back, so a stale pointer cannot hide +// behind one lucky word. +function checkPayload(name: string, got: any, tag: number): void { + if (got === undefined || got === null) { + failures++; + console.log("FAIL " + name + " payload missing"); + return; + } + check(name + ".tag", got.tag, tag); + check(name + ".arr0", got.arr[0], tag); + check(name + ".arr1", got.arr[1], tag + 1); + check(name + ".s", got.s, "payload-" + tag); +} +"#; + +/// The corruption case: the value being STORED is held across the key coercion, +/// so pre-fix a dangling pointer was written into a live object and outlived +/// the call. Covers the dynamic-index / polymorphic-index / array / computed +/// object-literal write paths. +#[test] +fn property_key_stored_values_survive_forced_evacuation() { + let dir = tempfile::tempdir().expect("tempdir"); + let source = format!( + "{PRELUDE}{}", + r#" +// --- js_dyn_index_set / js_object_set_index_polymorphic: obj[heavyKey] = v --- +const o1: any = receiver(); +o1[heavyKey("k1")] = payload(1); +churnAndCollect(); +checkPayload("dyn-index-set", o1.k1, 1); + +// Symbol.toPrimitive flavour of the same write. +const o2: any = receiver(); +o2[heavyPrimKey("k2")] = payload(2); +churnAndCollect(); +checkPayload("dyn-index-set-toprimitive", o2.k2, 2); + +// --- js_object_literal_set_computed / js_object_set_property_key --- +const lit: any = { [heavyKey("k3")]: payload(3) }; +keepalive.push(lit); +churnAndCollect(); +checkPayload("object-literal-computed", lit.k3, 3); + +// Several computed keys in one literal, so later coercions collect while the +// earlier entries are already installed. +const lit2: any = { + [heavyKey("a")]: payload(10), + [heavyKey("b")]: payload(11), + [heavyKey("c")]: payload(12), +}; +keepalive.push(lit2); +churnAndCollect(); +checkPayload("object-literal-multi-a", lit2.a, 10); +checkPayload("object-literal-multi-b", lit2.b, 11); +checkPayload("object-literal-multi-c", lit2.c, 12); + +// --- js_array_set_index_or_string: a boxed/object key on an ARRAY receiver --- +const arr: any[] = [0, 1, 2]; +keepalive.push(arr); +(arr as any)[heavyKey("named")] = payload(4); +churnAndCollect(); +checkPayload("array-string-key-set", (arr as any).named, 4); +check("array-elements-intact", arr[2], 2); + +// A non-canonical NUMERIC key on an array also stringifies (allocates). +(arr as any)[4294967295] = payload(5); +churnAndCollect(); +checkPayload("array-noncanonical-index", (arr as any)[4294967295], 5); + +// --- class-ref receiver takes the INT32-tagged class-ref write arm, where +// the stored value (not the receiver) is the operand at risk. +class C { static s(): number { return 1; } } +(C as any)[heavyKey("statKey")] = payload(6); +churnAndCollect(); +checkPayload("class-static-computed", (C as any).statKey, 6); + +// --- a write onto a prototype object, read back down the chain --- +const proto: any = receiver(); +const child: any = Object.create(proto); +keepalive.push(child); +proto[heavyKey("inherited")] = payload(7); +churnAndCollect(); +checkPayload("prototype-chain-computed", child.inherited, 7); + +console.log("failures:", failures); +"# + ); + let stdout = compile_and_run_forced_evacuation(dir.path(), &source); + assert_eq!( + stdout, "failures: 0\n", + "a property-key write stored a stale value under forced evacuation" + ); +} + +/// Receiver-only paths: reads, deletes, method dispatch and the own-property +/// predicates. A stale receiver here produces a wrong answer (or a fault) +/// rather than heap corruption, but it is the same rooting gap. +#[test] +fn property_key_receivers_survive_forced_evacuation() { + let dir = tempfile::tempdir().expect("tempdir"); + let source = format!( + "{PRELUDE}{}", + r#" +// --- js_dyn_index_get / js_object_get_index_polymorphic --- +const src: any = receiver(); +src.hit = payload(20); +src.n = 7; +churnAndCollect(); +checkPayload("dyn-index-get", src[heavyKey("hit")], 20); +check("dyn-index-get-number", src[heavyKey("n")], 7); + +// --- js_array_get_index_or_string --- +const arr: any[] = [11, 12, 13]; +keepalive.push(arr); +(arr as any).tail = payload(21); +churnAndCollect(); +checkPayload("array-get-string-key", (arr as any)[heavyKey("tail")], 21); +check("array-get-numeric-key", arr[heavyKey("1") as any], 12); + +// --- js_object_delete_dynamic --- +const del: any = receiver(); +del.gone = payload(22); +del.kept = payload(23); +churnAndCollect(); +delete del[heavyKey("gone")]; +churnAndCollect(); +check("delete-removed", del.gone, undefined); +checkPayload("delete-kept-sibling", del.kept, 23); + +// --- js_native_call_method_value: obj[heavyKey]() --- +const callee: any = receiver(); +callee.answer = function (): number { return this.seed + 41; }; +churnAndCollect(); +check("method-by-computed-key", callee[heavyKey("answer")](), 42); + +// --- js_object_set_property_key_method: { [k]() {} } --- +const withMethod: any = { + [heavyKey("run")](): number { return 99; }, +}; +keepalive.push(withMethod); +churnAndCollect(); +check("computed-method-literal", withMethod.run(), 99); + +// --- js_object_define_accessor: { get [k]() {}, set [k](v) {} } --- +let stored: any = null; +const accessor: any = { + get [heavyKey("prop")](): any { return stored; }, + set [heavyKey("prop")](v: any) { stored = v; }, +}; +keepalive.push(accessor); +accessor.prop = payload(24); +churnAndCollect(); +checkPayload("computed-accessor", accessor.prop, 24); + +// --- hasOwn / propertyIsEnumerable / `in` --- +const probe: any = receiver(); +probe.here = payload(25); +churnAndCollect(); +check("hasOwnProperty-method", probe.hasOwnProperty(heavyKey("here")), true); +check("hasOwnProperty-miss", probe.hasOwnProperty(heavyKey("absent")), false); +check("Object.hasOwn", Object.hasOwn(probe, heavyKey("here")), true); +check( + "propertyIsEnumerable", + probe.propertyIsEnumerable(heavyKey("here")), + true +); +// NOTE: `objectKey in obj` is deliberately NOT asserted here — Perry's +// `js_object_has_property` only runs ToPropertyKey for NUMBER keys, so an +// object key never reaches the coercion at all. That is a pre-existing spec +// gap (Node returns true), tracked separately; it is not a rooting bug. +// `in` with a NUMBER key does coerce (and allocates) with the receiver raw, +// which is the arm this suite covers. +const numKeyed: any = receiver(); +numKeyed[307] = payload(26); +churnAndCollect(); +check("in-operator-number-key", 307 in numKeyed, true); +checkPayload("number-key-payload", numKeyed[307], 26); + +// The receiver must be intact after all of that. +checkPayload("probe-still-intact", probe.here, 25); + +console.log("failures:", failures); +"# + ); + let stdout = compile_and_run_forced_evacuation(dir.path(), &source); + assert_eq!( + stdout, "failures: 0\n", + "a property-key read/dispatch path used a stale receiver under forced evacuation" + ); +} + +/// Proxy forward-to-target writes (`target_set`) run `ToPropertyKey` with the +/// target receiver and the stored value both live. `target_get` was already +/// rooted; its write sibling was not. +#[test] +fn proxy_target_set_survives_forced_evacuation() { + let dir = tempfile::tempdir().expect("tempdir"); + let source = format!( + "{PRELUDE}{}", + r#" +// No `set` trap -> the proxy forwards to the target through `target_set`, +// which performs the GC-capable ToPropertyKey with `target` and `value` live. +const target: any = receiver(); +const p: any = new Proxy(target, {}); +keepalive.push(p); +p[heavyKey("viaProxy")] = payload(30); +churnAndCollect(); +checkPayload("proxy-forward-set", target.viaProxy, 30); +checkPayload("proxy-forward-read", p.viaProxy, 30); + +// Reflect.set with an object key takes the same path. +const target2: any = receiver(); +Reflect.set(target2, heavyKey("viaReflect"), payload(31)); +churnAndCollect(); +checkPayload("reflect-set", target2.viaReflect, 31); + +console.log("failures:", failures); +"# + ); + let stdout = compile_and_run_forced_evacuation(dir.path(), &source); + assert_eq!( + stdout, "failures: 0\n", + "a proxy forward-to-target write used stale operands under forced evacuation" + ); +}