fix(runtime): root the normalize subject across form coercion

changelog.d/8451-normalize-form-coercion-rooting.md

@@ -0,0 +1,13 @@
+### Fixed
+
+- `String.prototype.normalize` no longer holds a borrowed heap-string payload
+  across its `form` argument's `ToString` coercion. The coercion is a
+  collection point twice over — an object form runs user `toString` (whose
+  loop back-edge polls run a moving minor), and an inline short-string form
+  materializes onto the heap — so a young subject could be evacuated while
+  `js_string_normalize` held a `&str` into its pre-move address, after which
+  the normalization pass read retired from-space. The form is now coerced
+  first, the subject is rooted across the coercion, and the payload is
+  borrowed only from the post-collection address. The observable orderings are
+  unchanged: `ToString` still runs before the form is validated, so a Symbol
+  form throws `TypeError` rather than the invalid-form `RangeError`. (#8426)

crates/perry-runtime/src/gc/tests/runtime_roots.rs

@@ -15,6 +15,7 @@ mod old_defrag_contract;
 mod prototype_addr_cache;
 mod regexp_last_index;
 mod side_table_scanners;
+mod string_normalize_form;
 mod string_slice;
 mod symbol_description;
 mod transient_handles;

crates/perry-runtime/src/gc/tests/runtime_roots/string_normalize_form.rs

@@ -0,0 +1,132 @@
+use super::*;
+use crate::arena::FromSpaceProtection;
+
+/// #8426: `String.prototype.normalize` borrowed the subject string's inline
+/// WTF-8 payload BEFORE coercing its `form` argument, then read that borrow
+/// after the coercion had returned.
+///
+/// The coercion is a collection point twice over: an inline short-string form
+/// materializes onto the heap (so even `s.normalize("NFC")` allocates there),
+/// and an object form runs user `toString`, whose loop back-edge polls run a
+/// moving minor. Either can evacuate a young subject, and a `&str` taken
+/// beforehand is a copy the collector cannot rewrite — rooting rewrites slots,
+/// never already-materialized borrows. The normalization pass then read
+/// retired from-space.
+///
+/// The test drives the object-form window, which is the one reachable from
+/// user code today: `toString` forces a real copying minor. Three assertions
+/// together, because any one alone can pass vacuously — (1) the subject was
+/// young, (2) the collection actually MOVED it, so the window was live, and
+/// (3) the normalized bytes are the subject's, not the retired page's.
+///
+/// `PoisonOnly` makes failure certain rather than lucky. Without it, whether a
+/// stale borrow is *detected* depends on what the allocator happened to
+/// recycle into the retired page; with it, those bytes are guaranteed poison.
+/// The cost is the failure mode: a regression faults inside the normalization
+/// pass (poison is not valid WTF-8) rather than reaching the byte assertions
+/// below, so a reintroduced #8426 shows up as a SIGSEGV naming this test.
+/// That is deliberate — a gate that can pass vacuously is not a gate.
+#[test]
+fn normalize_form_coercion_must_not_strand_the_subject_payload() {
+    let _guard = CopyingNurseryTestGuard::new(0);
+    let _trigger = GcTriggerThresholdTestGuard::suppress_automatic_triggers();
+    let _mode = crate::arena::ProtectionModeGuard::set(FromSpaceProtection::PoisonOnly);
+    register_runtime_handle_root_scanner_for_tests();
+
+    // Decomposed "café-normalize": NFC must compose e+U+0301 into U+00E9, so a
+    // pass-through cannot be mistaken for a correct normalization.
+    const SUBJECT: &[u8] = "cafe\u{301}-normalize".as_bytes();
+    const EXPECTED: &[u8] = "caf\u{e9}-normalize".as_bytes();
+
+    let scope = RuntimeHandleScope::new();
+    let subject = crate::string::js_string_from_bytes(SUBJECT.as_ptr(), SUBJECT.len() as u32);
+    // (1) premise: a heap string in the MOVABLE nursery. A `+=` accumulator
+    // buffer or a large string lives outside it and would never relocate,
+    // which would make every assertion below vacuous.
+    assert!(
+        crate::arena::pointer_in_nursery(subject as usize),
+        "test premise: the subject must be nursery-resident, or nothing moves"
+    );
+    let subject_handle = scope.root_string_ptr(subject);
+    let before_addr = subject as usize;
+
+    // `{ toString() { <forces a copying minor>; return "NFC" } }`
+    let form = crate::object::js_object_alloc(0, 1);
+    let form_handle = scope.root_raw_mut_ptr(form);
+    let to_string = crate::closure::js_closure_alloc(normalize_form_force_minor_gc as *const u8, 0);
+    let to_string_handle = scope.root_raw_mut_ptr(to_string);
+    let key = crate::string::js_string_from_bytes(b"toString".as_ptr(), 8);
+    let key_handle = scope.root_string_ptr(key);
+    form_handle.with_mut_ptr::<crate::object::ObjectHeader, _>(|form_ptr| {
+        key_handle.with_const_ptr::<crate::StringHeader, _>(|key_ptr| {
+            crate::object::js_object_set_field_by_name(
+                form_ptr,
+                key_ptr,
+                to_string_handle.with_mut_ptr::<crate::closure::ClosureHeader, _>(
+                    |to_string_ptr| crate::value::js_nanbox_pointer(to_string_ptr as i64),
+                ),
+            );
+        });
+    });
+
+    NORMALIZE_FORM_COERCIONS.with(|c| c.set(0));
+    let before_collections = gc_collection_count();
+    let form_value = form_handle.with_mut_ptr::<crate::object::ObjectHeader, _>(|form_ptr| {
+        crate::value::js_nanbox_pointer(form_ptr as i64)
+    });
+    // Two combinators, no bare read (#7341): `with_const_ptr` hands the
+    // subject to `js_string_normalize`, which since the fix roots it itself
+    // (a self-rooting entry point), and `across_const` hands back its
+    // POST-collection address — the coercion inside moves it.
+    let (result, after_ptr) = subject_handle.across_const::<crate::StringHeader, _>(|| {
+        subject_handle.with_const_ptr::<crate::StringHeader, _>(|s| {
+            crate::string::js_string_normalize(s, form_value)
+        })
+    });
+
+    assert_eq!(
+        NORMALIZE_FORM_COERCIONS.with(|c| c.get()),
+        1,
+        "the form's toString must have run exactly once"
+    );
+    assert!(
+        gc_collection_count() > before_collections,
+        "test premise: the coercion must have collected"
+    );
+    // (2) the subject really was evacuated inside the window — otherwise a
+    // stale borrow would still point at live bytes and this test could not
+    // distinguish the fix from the bug.
+    let after_addr = after_ptr as usize;
+    assert_ne!(
+        after_addr, before_addr,
+        "test premise: the copying minor must have MOVED the subject"
+    );
+
+    // (3) the normalization read the subject's live bytes, not the retired page.
+    unsafe {
+        assert_eq!(
+            (*result).byte_len as usize,
+            EXPECTED.len(),
+            "normalized length must come from the live subject"
+        );
+        let data = (result as *const u8).add(std::mem::size_of::<crate::StringHeader>());
+        let bytes = std::slice::from_raw_parts(data, EXPECTED.len());
+        assert_eq!(
+            bytes, EXPECTED,
+            "normalized bytes must be the subject's, not retired from-space"
+        );
+    }
+}
+
+thread_local! {
+    static NORMALIZE_FORM_COERCIONS: Cell<u32> = const { Cell::new(0) };
+}
+
+/// The form object's `toString`: forces a real copying minor — the moving
+/// collection a user `toString`'s loop back-edge polls would run — then
+/// returns the form name.
+extern "C" fn normalize_form_force_minor_gc(_closure: *const crate::closure::ClosureHeader) -> f64 {
+    NORMALIZE_FORM_COERCIONS.with(|c| c.set(c.get() + 1));
+    let _ = crate::gc::gc_collect_minor();
+    test_string_value(b"NFC")
+}

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

@@ -517,25 +517,41 @@ pub extern "C" fn js_string_normalize(
     if !is_valid_string_ptr(s) {
         return js_string_from_bytes(std::ptr::null(), 0);
     }
-    let str_data = string_as_str(s);
 
     // `undefined` (omitted argument) → default NFC. Note: explicit `null`
     // is NOT undefined — it stringifies to "null" and falls through to the
     // invalid-form error path below.
     let form_jsval = crate::value::JSValue::from_bits(form_value.to_bits());
-    let form_owned: String = if form_jsval.is_undefined() {
-        "NFC".to_string()
-    } else {
-        // ToString(form) runs before the form-validity check, so a Symbol form
-        // throws a TypeError (§7.1.17) — not the RangeError of an invalid form.
-        crate::builtins::reject_symbol_to_string(form_value);
-        let form_ptr = crate::value::js_jsvalue_to_string(form_value);
-        if is_valid_string_ptr(form_ptr) {
-            string_as_str(form_ptr).to_string()
+
+    // Coerce the form BEFORE borrowing the subject's payload. `ToString(form)`
+    // is a collection point twice over: an inline short-string form
+    // materializes to the heap (so even `s.normalize("NFC")` allocates here),
+    // and an object form runs user `toString`, whose loop back-edge polls can
+    // run a moving minor. Either can evacuate `s`, and a `&str` taken
+    // beforehand is a copy the collector cannot rewrite — rooting rewrites
+    // slots, never already-materialized borrows
+    // (`docs/src/internals/gc-rooting-invariant.md`). Root the subject across
+    // the coercion and borrow only from the address handed back. (#8426)
+    let scope = crate::gc::RuntimeHandleScope::new();
+    let s_handle = scope.root_string_ptr(s);
+    let (form_owned, s) = s_handle.across_const::<StringHeader, _>(|| -> String {
+        if form_jsval.is_undefined() {
+            "NFC".to_string()
         } else {
-            String::new()
+            // ToString(form) runs before the form-validity check, so a Symbol
+            // form throws a TypeError (§7.1.17) — not the RangeError of an
+            // invalid form. The reorder preserves that ordering: coercion
+            // still precedes validation.
+            crate::builtins::reject_symbol_to_string(form_value);
+            let form_ptr = crate::value::js_jsvalue_to_string(form_value);
+            if is_valid_string_ptr(form_ptr) {
+                string_as_str(form_ptr).to_string()
+            } else {
+                String::new()
+            }
         }
-    };
+    });
+    let str_data = string_as_str(s);
 
     #[cfg(feature = "string-normalize")]
     let normalized: String = {

test-files/test_issue_8426_normalize_reentrant.ts

@@ -0,0 +1,105 @@
+// #8426: `String.prototype.normalize` must not hold a borrowed heap-string
+// payload across the form argument's ToString coercion.
+//
+// The coercion is a collection point twice over: an inline short-string form
+// materializes onto the heap, and an object form runs user `toString`, whose
+// loop back-edge polls can run a moving minor. Either can evacuate the
+// subject; a `&str` taken beforehand then points into from-space.
+//
+// The fix reorders the coercion ahead of the borrow, so this file also pins
+// the two observable orderings that reorder must NOT change: ToString runs
+// before the form is validated, and a Symbol form throws TypeError (§7.1.17)
+// rather than the invalid-form RangeError (#2782).
+
+// Build the subject at runtime so it is a young *nursery* heap string (>5
+// bytes, so not SSO) rather than a folded constant: "cafe" + U+0301 combining
+// acute. `join` matters: a `+=` accumulator chain leaves its buffer outside the
+// movable nursery, so a subject built that way never relocates and this test
+// would pass whether or not the bug is present.
+function buildSubject(tag: string): string {
+  const acute = String.fromCharCode(0x0301);
+  return ["caf", "e", acute, "-", tag].join("");
+}
+
+// Churn hard enough to cross several loop back-edge safepoint polls: volume
+// alone is not enough, the collector needs garbage to actually move.
+function churn(): void {
+  let junk = "";
+  const scraps: string[] = [];
+  for (let i = 0; i < 5000; i++) {
+    junk = junk + "x";
+    if (i % 50 === 0) {
+      scraps.push(junk.slice(0, 8) + i);
+    }
+  }
+  if (junk.length !== 5000 || scraps.length !== 100) {
+    throw new Error("string churn was optimized away");
+  }
+}
+
+// ---- 1. the bug: subject must survive a moving collection in the window ----
+let coercions = 0;
+const subject = buildSubject("runtime");
+const reentrantForm = {
+  toString(): string {
+    coercions++;
+    churn();
+    return "NFC";
+  },
+};
+console.log("reentrant NFC =>", JSON.stringify(subject.normalize(reentrantForm as any)));
+console.log("reentrant coercions =>", coercions);
+
+// Decomposing form, same window — a different normalization pass over the
+// same borrowed payload.
+const subjectD = buildSubject("decompose");
+const reentrantFormD = {
+  toString(): string {
+    churn();
+    return "NFD";
+  },
+};
+const decomposed = subjectD.normalize(reentrantFormD as any);
+console.log("reentrant NFD length =>", decomposed.length);
+console.log("reentrant NFD roundtrip =>", JSON.stringify(decomposed.normalize("NFC")));
+
+// Repeat under sustained pressure: each call opens the window again.
+let repeated = "";
+for (let i = 0; i < 20; i++) {
+  const s = buildSubject("iter" + i);
+  repeated = s.normalize({
+    toString(): string {
+      churn();
+      return "NFC";
+    },
+  } as any);
+}
+console.log("repeated last =>", JSON.stringify(repeated));
+
+// A plain string form is the *common* case and still allocates (an SSO form
+// materializes onto the heap inside the coercion).
+console.log("sso form =>", JSON.stringify(buildSubject("sso").normalize("NFC")));
+
+// ---- 2. ToString still runs BEFORE the form is validated ----
+let badCoercions = 0;
+try {
+  buildSubject("bad").normalize({
+    toString(): string {
+      badCoercions++;
+      churn();
+      return "BAD";
+    },
+  } as any);
+  console.log("bad form => no throw");
+} catch (e: any) {
+  console.log("bad form =>", e.name);
+}
+console.log("bad form coercions =>", badCoercions);
+
+// ---- 3. a Symbol form throws TypeError, not RangeError (#2782) ----
+try {
+  buildSubject("sym").normalize(Symbol("nope") as any);
+  console.log("symbol form => no throw");
+} catch (e: any) {
+  console.log("symbol form =>", e.name);
+}