fix(runtime): root operands across GC-capable coercions in dynamic arith / relational / concat (#6655)

changelog.d/6934-dynamic-arith-operand-rooting.md

@@ -0,0 +1,15 @@
+**Root operands across GC-capable coercions in the dynamic operator helpers (#6934, closes #6655)**
+
+`to_numeric` on an object operand runs a user `Symbol.toPrimitive` / `valueOf` / `toString`, which can allocate, trigger a GC and **evacuate** live objects. The dynamic operator helpers held the *other* operand as a raw NaN-boxed `f64` in a Rust local across exactly that call — neither a GC root nor a codegen shadow slot — so after the first coercion it could name a forwarded address. The mirror hazard hits the *coerced* `a` when it resolves to a BigInt pointer and the **second** operand is the allocating one, so both operand orders are affected. The correct discipline already existed in-tree (`dynamic_bigint_binary_op`; `js_dynamic_ushr` / `throw_mix_bigint` in #6650); this applies it file-wide and to the siblings sharing the shape.
+
+- **`value/dynamic_arith.rs`** — the `to_numeric(a); to_numeric(b)` prelude in `js_dynamic_{mul,sub,div,mod,pow,shr,shl,bitand,bitor,bitxor}`, factored into one `to_numeric_pair` helper that returns handles and feeds the existing `dynamic_bigint_binary_op_from_handles` without re-rooting. `js_dynamic_ushr` folded onto the same helper so the file has a single mechanism. Also `js_numeric_step` (`++`/`--`), which allocates `1n` via `js_bigint_from_i64` while the incoming BigInt operand sits raw in a local — the old comment there reasoned only about the *new* `one_ptr` surviving and missed that the pre-existing operand is the one at risk. Unaffected and unchanged: `js_dynamic_add`, `js_dynamic_neg`, `js_dynamic_bitnot`, `js_to_numeric`, `js_dynamic_string_or_number_add` (already rooted).
+- **`builtins/arithmetic.rs`** — `abstract_relational` (behind `js_rel_{lt,gt,le,ge}`) had the identical prelude and additionally held `px` (frequently a freshly allocated heap string from the `DefaultString` arm) across the second coercion; its `vx`/`vy` snapshots also had `as_bigint_ptr()` payloads dereferenced *after* allocating `string_to_bigint` / `js_number_coerce` steps, so those pointers are now re-derived from handles at the point of use. `js_loose_eq` — not named in the issue, found while reading the file — coerces the object side via `rel_to_primitive` and then recurses with the other, raw operand.
+- **`string/concat.rs`** — `js_string_concat_value` / `js_value_concat_string` hold the raw `*const StringHeader` operand across **two** GC-capable operations: `string_storage_alloc` (→ `arena_alloc_gc`) on the fast path and `js_jsvalue_to_string`'s user `toString` on the slow path. `js_string_concat` already roots its arguments, but that is one frame too late.
+
+Because the fix puts a `RuntimeHandleScope` on every dynamic binary operator, each also gains the plain-double fast path that skips the scope — the same `0x7FF9` tag-band predicate `js_number_coerce` already short-circuits on and that #5525 added to `js_dynamic_string_or_number_add`. For two plain IEEE-754 doubles `ToNumeric` is the identity and there are no pointers to root; the issue explicitly anticipates this escape. A unit test feeds every operator the same numbers as plain doubles and as int32-tagged values — forcing the fast and rooted paths respectively — and requires them to agree, plus a non-finite case pinning NaN/±Inf and `-1 % -1 == -0`.
+
+**The pre-fix state did not reproduce, and that is recorded rather than papered over.** Reverting only the three runtime files to the merge base (keeping the identical tests) still passed, as did a stronger variant that re-fills the nursery *after* the collection. `PERRY_GC_DIAG=1` confirms the stress arm is not inert (`retained_forwarded_stub_objects=6` — evacuation fires) and shows why it stays latent: `gc/oldgen.rs` deliberately retains a forwarding stub at the old address because "a minor sweep cannot prove a stub unreferenced", so a stale read lands on the stub and silently gets the right answer. Stubs are reclaimed only once outside the recent-block safety window. `PERRY_GC_VERIFY_EVACUATION=1` also cannot catch this class structurally — it checks mutable live *slots*, and a raw operand in a Rust local is not a tracked slot. This lands as soundness hardening, not as a fix for an observed miscompile.
+
+New `crates/perry/tests/gc_dynamic_arith_operand_rooting_6655.rs`: 4 tests covering every affected operator in **both** operand orders under `PERRY_GC_FORCE_EVACUATE=1` + `PERRY_GC_VERIFY_EVACUATION=1`, with operands kept reachable from a root so they are genuinely evacuated (moved + rewritten) rather than merely swept, and `valueOf` reading an instance *field* so a stale receiver yields a wrong value rather than a coincidentally-correct constant. 4/4 pass; the standalone probe is also clean under default, `GEN_GC=0`, `WRITE_BARRIERS=0` and `FORCE_EVACUATE+VERIFY+GEN_GC=0`. `cargo test -p perry-runtime --lib` is 1478/0 with `--test-threads=1` (the 5 failures in the default parallel run — `gc::tests::teardown::*`, `global_this_webassembly`, `native_module_stream` — pass in isolation and are a pre-existing shared-global parallelism artifact). Gap coverage was **scoped, not full**: 29 gap tests over the touched surfaces are byte-exact vs pinned Node v26.5.0 (pass=29 fail=0 skip=0), but the full 430-file sweep was skipped because the box sat at 13–16 GB free against the 25 GB gate with three GC agents building concurrently.
+
+The new harness sets `PERRY_EXTRA_LINK_ARGS="-framework CoreFoundation"`: on this host the runtime-only macOS link path omits CoreFoundation while `perry-runtime` pulls `iana_time_zone` (`_CFRelease` & co.), and the pre-existing `gc_side_table_roots_evacuation` test fails identically on an untouched checkout in both `perry-dev` and `release` — likely fallout from #6923, worth its own issue. The larger sibling family (receiver + stored value held across `ToPropertyKey`) is deliberately out of scope and filed separately.

crates/perry-runtime/src/builtins/arithmetic.rs

@@ -124,13 +124,30 @@ pub extern "C" fn js_loose_eq(a: JSValue, b: JSValue) -> JSValue {
     // steps 10-11). Object-vs-object was settled above; symbols are primitives
     // (`eq_is_object` excludes them) and correctly fall through to not-equal.
     // Done before the BigInt block so `0n == { valueOf() { return 0n } }` works.
+    // #6655: `rel_to_primitive` runs a user `valueOf`/`toString`, so it can
+    // allocate, collect and evacuate. The *other* operand is a raw NaN-boxed
+    // local here — not a GC root — so it must be rooted across the coercion and
+    // re-read through its handle before the recursive call, or `==` compares
+    // against a forwarded address.
     if eq_is_object(a) {
+        let scope = crate::gc::RuntimeHandleScope::new();
+        let b_handle = scope.root_nanbox_f64(f64::from_bits(b.bits()));
         let pa = unsafe { rel_to_primitive(f64::from_bits(a.bits())) };
-        return js_loose_eq(JSValue::from_bits(pa.to_bits()), b);
+        let pa_handle = scope.root_nanbox_f64(pa);
+        return js_loose_eq(
+            JSValue::from_bits(pa_handle.get_nanbox_u64()),
+            JSValue::from_bits(b_handle.get_nanbox_u64()),
+        );
     }
     if eq_is_object(b) {
+        let scope = crate::gc::RuntimeHandleScope::new();
+        let a_handle = scope.root_nanbox_f64(f64::from_bits(a.bits()));
         let pb = unsafe { rel_to_primitive(f64::from_bits(b.bits())) };
-        return js_loose_eq(a, JSValue::from_bits(pb.to_bits()));
+        let pb_handle = scope.root_nanbox_f64(pb);
+        return js_loose_eq(
+            JSValue::from_bits(a_handle.get_nanbox_u64()),
+            JSValue::from_bits(pb_handle.get_nanbox_u64()),
+        );
     }
     // BigInt abstract equality (ES2024 §7.2.15). Neither side is
     // null/undefined here and boxed wrappers (incl. `Object(0n)`) have already
@@ -273,22 +290,40 @@ unsafe fn rel_string_compare(a: f64, b: f64) -> i32 {
 /// runs on the two operands (observable when a `valueOf`/`toString` has side
 /// effects). Returns [`REL_TRUE`], [`REL_FALSE`], or [`REL_UNDEFINED`].
 unsafe fn abstract_relational(x: f64, y: f64, x_first: bool) -> i32 {
-    let (px, py) = if x_first {
-        let px = rel_to_primitive(x);
-        let py = rel_to_primitive(y);
+    // #6655: `rel_to_primitive` runs a user `Symbol.toPrimitive` / `valueOf` /
+    // `toString`, which can allocate, trigger a GC and *evacuate* live objects.
+    // Every raw NaN-boxed `f64` in a local here is invisible to the collector,
+    // so pre-fix the second operand was held unrooted across the first
+    // coercion, and `px` — frequently a *freshly allocated* heap string from
+    // the `DefaultString` arm — was held unrooted across the second. Root both
+    // inputs before the first coercion and both primitives as they are
+    // produced, then read every value back through its handle. Same discipline
+    // as `dynamic_bigint_binary_op` / `js_dynamic_ushr` in `value/dynamic_arith.rs`.
+    let scope = crate::gc::RuntimeHandleScope::new();
+    let x_in = scope.root_nanbox_f64(x);
+    let y_in = scope.root_nanbox_f64(y);
+    let (px_handle, py_handle) = if x_first {
+        let px = scope.root_nanbox_f64(rel_to_primitive(x_in.get_nanbox_f64()));
+        let py = scope.root_nanbox_f64(rel_to_primitive(y_in.get_nanbox_f64()));
         (px, py)
     } else {
-        let py = rel_to_primitive(y);
-        let px = rel_to_primitive(x);
+        let py = scope.root_nanbox_f64(rel_to_primitive(y_in.get_nanbox_f64()));
+        let px = scope.root_nanbox_f64(rel_to_primitive(x_in.get_nanbox_f64()));
         (px, py)
     };
+    let px = px_handle.get_nanbox_f64();
+    let py = py_handle.get_nanbox_f64();
 
+    // NOTE: `vx` / `vy` are *snapshots*. Tag predicates (`is_any_string`,
+    // `is_bigint`, …) stay valid across a GC because evacuation preserves the
+    // tag, but any pointer payload read out of them (`as_bigint_ptr`) must be
+    // re-derived from the handle at the point of use — see the BigInt arms below.
     let vx = JSValue::from_bits(px.to_bits());
     let vy = JSValue::from_bits(py.to_bits());
 
     // Both String → code-unit (byte) compare; never `undefined`.
     if vx.is_any_string() && vy.is_any_string() {
-        return if rel_string_compare(px, py) < 0 {
+        return if rel_string_compare(px_handle.get_nanbox_f64(), py_handle.get_nanbox_f64()) < 0 {
             REL_TRUE
         } else {
             REL_FALSE
@@ -301,11 +336,16 @@ unsafe fn abstract_relational(x: f64, y: f64, x_first: bool) -> i32 {
     // BigInt vs String / String vs BigInt: parse the string as a BigInt
     // (StringToBigInt); a non-numeric string makes the comparison `undefined`.
     if x_big && vy.is_any_string() {
-        let s = string_content_for_bigint(py);
+        let s = string_content_for_bigint(py_handle.get_nanbox_f64());
+        // `string_to_bigint` allocates the parsed BigInt, so re-derive the `x`
+        // pointer from its handle *after* that call — the snapshot in `vx` may
+        // name a forwarded address by now (#6655).
         return match crate::bigint::string_to_bigint(&s) {
             None => REL_UNDEFINED,
             Some(ny) => {
-                if crate::bigint::js_bigint_cmp(vx.as_bigint_ptr(), ny) < 0 {
+                let px_ptr =
+                    JSValue::from_bits(px_handle.get_nanbox_f64().to_bits()).as_bigint_ptr();
+                if crate::bigint::js_bigint_cmp(px_ptr, ny) < 0 {
                     REL_TRUE
                 } else {
                     REL_FALSE
@@ -314,11 +354,13 @@ unsafe fn abstract_relational(x: f64, y: f64, x_first: bool) -> i32 {
         };
     }
     if vx.is_any_string() && y_big {
-        let s = string_content_for_bigint(px);
+        let s = string_content_for_bigint(px_handle.get_nanbox_f64());
         return match crate::bigint::string_to_bigint(&s) {
             None => REL_UNDEFINED,
             Some(nx) => {
-                if crate::bigint::js_bigint_cmp(nx, vy.as_bigint_ptr()) < 0 {
+                let py_ptr =
+                    JSValue::from_bits(py_handle.get_nanbox_f64().to_bits()).as_bigint_ptr();
+                if crate::bigint::js_bigint_cmp(nx, py_ptr) < 0 {
                     REL_TRUE
                 } else {
                     REL_FALSE
@@ -327,9 +369,13 @@ unsafe fn abstract_relational(x: f64, y: f64, x_first: bool) -> i32 {
         };
     }
 
-    // Both BigInt → exact integer compare.
+    // Both BigInt → exact integer compare. `js_bigint_cmp` does not allocate,
+    // but re-read both pointers through the handles anyway so this arm stays
+    // correct if it ever grows an allocating step.
     if x_big && y_big {
-        return if crate::bigint::js_bigint_cmp(vx.as_bigint_ptr(), vy.as_bigint_ptr()) < 0 {
+        let px_ptr = JSValue::from_bits(px_handle.get_nanbox_f64().to_bits()).as_bigint_ptr();
+        let py_ptr = JSValue::from_bits(py_handle.get_nanbox_f64().to_bits()).as_bigint_ptr();
+        return if crate::bigint::js_bigint_cmp(px_ptr, py_ptr) < 0 {
             REL_TRUE
         } else {
             REL_FALSE
@@ -339,26 +385,30 @@ unsafe fn abstract_relational(x: f64, y: f64, x_first: bool) -> i32 {
     // BigInt vs Number (mixed): exact mathematical compare. `js_number_coerce`
     // is `ToNumber` and throws on a Symbol operand, as the spec requires.
     if x_big {
-        let yn = js_number_coerce(py);
-        return match crate::bigint::bigint_cmp_f64(vx.as_bigint_ptr(), yn) {
+        // `js_number_coerce` on a string primitive can allocate; re-derive the
+        // BigInt pointer from its handle after the coercion (#6655).
+        let yn = js_number_coerce(py_handle.get_nanbox_f64());
+        let px_ptr = JSValue::from_bits(px_handle.get_nanbox_f64().to_bits()).as_bigint_ptr();
+        return match crate::bigint::bigint_cmp_f64(px_ptr, yn) {
             2 => REL_UNDEFINED,
             c if c < 0 => REL_TRUE,
             _ => REL_FALSE,
         };
     }
     if y_big {
-        let xn = js_number_coerce(px);
+        let xn = js_number_coerce(px_handle.get_nanbox_f64());
+        let py_ptr = JSValue::from_bits(py_handle.get_nanbox_f64().to_bits()).as_bigint_ptr();
         // `bigint_cmp_f64(y, xn)` is the sign of (y − x); x < y ⇔ that is positive.
-        return match crate::bigint::bigint_cmp_f64(vy.as_bigint_ptr(), xn) {
+        return match crate::bigint::bigint_cmp_f64(py_ptr, xn) {
             2 => REL_UNDEFINED,
             c if c > 0 => REL_TRUE,
             _ => REL_FALSE,
         };
     }
 
     // Both Number (after ToNumber). NaN on either side → undefined.
-    let xn = js_number_coerce(px);
-    let yn = js_number_coerce(py);
+    let xn = js_number_coerce(px_handle.get_nanbox_f64());
+    let yn = js_number_coerce(py_handle.get_nanbox_f64());
     if xn.is_nan() || yn.is_nan() {
         return REL_UNDEFINED;
     }

crates/perry-runtime/src/string/concat.rs

@@ -310,6 +310,17 @@ pub extern "C" fn js_string_concat_value(
     prefix: *const StringHeader,
     value: f64,
 ) -> *mut StringHeader {
+    // #6655: `prefix` is a raw movable heap pointer held across two different
+    // GC-capable operations — `string_storage_alloc` on the fast path below,
+    // and `js_jsvalue_to_string(value)` (an arbitrary user `toString`) on the
+    // slow path. Neither is a GC root, so an evacuating collection during
+    // either would leave the subsequent `(*prefix)` reads and `string_data`
+    // copy pointing at a forwarded address. Root it for the whole body and
+    // re-read it through the handle after anything that can allocate.
+    // (`js_string_concat` already roots its own arguments — that is one frame
+    // too late for this one.)
+    let scope = crate::gc::RuntimeHandleScope::new();
+    let prefix_handle = scope.root_string_ptr(prefix);
     let prefix_blen = if is_valid_string_ptr(prefix) {
         unsafe { (*prefix).byte_len }
     } else {
@@ -370,6 +381,10 @@ pub extern "C" fn js_string_concat_value(
         // Single allocation for prefix + number string
         let total_blen = prefix_blen as usize + num_len;
         let (ptr, data_ptr) = string_storage_alloc(total_blen as u32);
+        // `string_storage_alloc` → `arena_alloc_gc` can collect and evacuate, so
+        // the incoming `prefix` may have moved. Re-read it from its handle
+        // before touching the header or copying the payload (#6655).
+        let prefix = prefix_handle.get_raw_const_ptr::<StringHeader>();
 
         unsafe {
             // Both prefix and number digits are ASCII, so utf16_len == byte_len for the number part
@@ -400,9 +415,11 @@ pub extern "C" fn js_string_concat_value(
         return ptr;
     }
 
-    // Slow path: non-number value — fall back to js_jsvalue_to_string + js_string_concat
+    // Slow path: non-number value — fall back to js_jsvalue_to_string + js_string_concat.
+    // `js_jsvalue_to_string` can run a user `toString` and collect, so reload
+    // `prefix` from its handle afterwards (#6655).
     let value_str = crate::value::js_jsvalue_to_string(value);
-    js_string_concat(prefix, value_str)
+    js_string_concat(prefix_handle.get_raw_const_ptr::<StringHeader>(), value_str)
 }
 
 /// N-way string concatenation (v0.5.771).
@@ -629,6 +646,11 @@ pub extern "C" fn js_value_concat_string(
     value: f64,
     suffix: *const StringHeader,
 ) -> *mut StringHeader {
+    // #6655: mirror of `js_string_concat_value` — `suffix` is a raw movable
+    // heap pointer held across `string_storage_alloc` (fast path) and across
+    // `js_jsvalue_to_string(value)`'s user `toString` (slow path).
+    let scope = crate::gc::RuntimeHandleScope::new();
+    let suffix_handle = scope.root_string_ptr(suffix);
     let suffix_blen = if is_valid_string_ptr(suffix) {
         unsafe { (*suffix).byte_len }
     } else {
@@ -683,6 +705,8 @@ pub extern "C" fn js_value_concat_string(
 
         let total_blen = num_len + suffix_blen as usize;
         let (ptr, data_ptr) = string_storage_alloc(total_blen as u32);
+        // Re-read after the allocation: it can collect and evacuate (#6655).
+        let suffix = suffix_handle.get_raw_const_ptr::<StringHeader>();
 
         unsafe {
             let flags = if is_valid_string_ptr(suffix) {
@@ -712,8 +736,9 @@ pub extern "C" fn js_value_concat_string(
         return ptr;
     }
 
+    // Reload `suffix` after the user `toString` (#6655).
     let value_str = crate::value::js_jsvalue_to_string(value);
-    js_string_concat(value_str, suffix)
+    js_string_concat(value_str, suffix_handle.get_raw_const_ptr::<StringHeader>())
 }
 
 /// Fast integer-to-ASCII formatting into a provided buffer.