diff --git a/changelog.d/8451-normalize-form-coercion-rooting.md b/changelog.d/8451-normalize-form-coercion-rooting.md new file mode 100644 index 0000000000..ca923e2e66 --- /dev/null +++ b/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) diff --git a/crates/perry-runtime/src/gc/tests/runtime_roots.rs b/crates/perry-runtime/src/gc/tests/runtime_roots.rs index a93ba9d7c9..44869b2c89 100644 --- a/crates/perry-runtime/src/gc/tests/runtime_roots.rs +++ b/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; diff --git a/crates/perry-runtime/src/gc/tests/runtime_roots/string_normalize_form.rs b/crates/perry-runtime/src/gc/tests/runtime_roots/string_normalize_form.rs new file mode 100644 index 0000000000..3b471a048d --- /dev/null +++ b/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() { ; 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::(|form_ptr| { + key_handle.with_const_ptr::(|key_ptr| { + crate::object::js_object_set_field_by_name( + form_ptr, + key_ptr, + to_string_handle.with_mut_ptr::( + |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::(|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::(|| { + subject_handle.with_const_ptr::(|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 = crate::string::string_data(result); + 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 = 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") +} diff --git a/crates/perry-runtime/src/string/compare.rs b/crates/perry-runtime/src/string/compare.rs index d0d6865bde..164413be84 100644 --- a/crates/perry-runtime/src/string/compare.rs +++ b/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::(|| -> 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 = { diff --git a/test-files/test_issue_8426_normalize_reentrant.ts b/test-files/test_issue_8426_normalize_reentrant.ts new file mode 100644 index 0000000000..49fa7b0fbd --- /dev/null +++ b/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); +}