diff --git a/changelog.d/8446-regexp-lastindex-rooting.md b/changelog.d/8446-regexp-lastindex-rooting.md new file mode 100644 index 0000000000..8171863cec --- /dev/null +++ b/changelog.d/8446-regexp-lastindex-rooting.md @@ -0,0 +1,10 @@ +Fixed a moving-GC hazard in `RegExp.prototype.exec` and `String.prototype.matchAll`. +RegExpBuiltinExec coerces `lastIndex` with `ToLength`, which runs user +`valueOf`/`toString` when `lastIndex` holds an object — arbitrary JS that can +run a copying minor. Both entry points captured the subject string (and, in +`exec`, borrowed its inline payload) *before* that coercion, so a collection +inside the callback left the match running over relocated-away bytes: the match +silently evaporated or returned text from unrelated heap memory. The coercion +now runs first with the regex header and subject rooted, and every later use +reads the refreshed addresses — including the match array's `.input` property +and the `lastIndex` write-back on the lookbehind/backreference path. diff --git a/crates/perry-runtime/src/gc/tests/runtime_roots.rs b/crates/perry-runtime/src/gc/tests/runtime_roots.rs index 97cd2442d4..a93ba9d7c9 100644 --- a/crates/perry-runtime/src/gc/tests/runtime_roots.rs +++ b/crates/perry-runtime/src/gc/tests/runtime_roots.rs @@ -13,6 +13,7 @@ mod json_shape_template; mod native_module_name; mod old_defrag_contract; mod prototype_addr_cache; +mod regexp_last_index; mod side_table_scanners; mod string_slice; mod symbol_description; diff --git a/crates/perry-runtime/src/gc/tests/runtime_roots/regexp_last_index.rs b/crates/perry-runtime/src/gc/tests/runtime_roots/regexp_last_index.rs new file mode 100644 index 0000000000..74726e6adf --- /dev/null +++ b/crates/perry-runtime/src/gc/tests/runtime_roots/regexp_last_index.rs @@ -0,0 +1,128 @@ +//! Moving-GC regression for `RegExp.prototype.exec` (#8428). +//! +//! RegExpBuiltinExec step 4 is `ToLength(Get(R, "lastIndex"))`. When +//! `lastIndex` holds an object the ToNumber half runs OrdinaryToPrimitive — +//! user `valueOf`/`toString`, i.e. arbitrary JS, which reaches safepoints and +//! can run a copying minor. `js_regexp_exec` borrowed the subject's inline +//! WTF-8 payload (`string_as_str`) BEFORE that coercion, and rooting rewrites +//! slots, never an already materialized `&str` — so the whole match ran over +//! from-space bytes. +//! +//! The planted `valueOf` runs a copying minor and then refills the just +//! retired Eden with a distinctive pattern, so a pre-fix run matches against +//! the stomped bytes instead of the relocated subject and the match evaporates. +//! Liveness is asserted both ways (a copying minor ran; the subject actually +//! moved), per CLAUDE.md's "a gate must assert its subject was live". + +use super::super::super::*; +use super::super::support::*; + +use crate::array::ArrayHeader; +use crate::object::ObjectHeader; +use crate::regex::RegExpHeader; +use crate::string::StringHeader; + +/// Stand-in for `{ valueOf() { …allocating user JS…; return 0 } }`. +extern "C" fn collect_then_zero(_closure: *const crate::closure::ClosureHeader) -> f64 { + crate::gc::gc_collect_minor(); + // Recycle the just-retired from-space blocks back out as fresh strings, so + // a stale borrow reads THESE bytes rather than the subject's old ones. + let filler = [b'#'; 64]; + for _ in 0..64 { + let _ = crate::string::js_string_from_bytes(filler.as_ptr(), filler.len() as u32); + } + 0.0 +} + +fn heap_string(bytes: &[u8]) -> *mut StringHeader { + crate::string::js_string_from_bytes(bytes.as_ptr(), bytes.len() as u32) +} + +fn capture_text(element: f64, index: usize) -> String { + let mut scratch = [0u8; crate::value::SHORT_STRING_MAX_LEN]; + let (data, len) = crate::string::str_bytes_from_jsvalue(element, &mut scratch) + .unwrap_or_else(|| panic!("capture {index} must be a string")); + let bytes = unsafe { std::slice::from_raw_parts(data, len as usize) }; + String::from_utf8_lossy(bytes).into_owned() +} + +#[test] +fn regexp_exec_survives_a_moving_minor_inside_the_lastindex_coercion() { + let _guard = CopyingNurseryTestGuard::new(0); + let _scan = ConservativeScanDisabledGuard::new(); + let _triggers = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + let _force = ForcedEvacuationTestGuard::on(); + register_runtime_handle_root_scanner_for_tests(); + + let scope = crate::gc::RuntimeHandleScope::new(); + + // The subject must live in the movable nursery — a large string goes + // straight to the non-moving old generation and the window never opens. + let subject = heap_string(b"prefix-young-42-suffix"); + assert!(crate::arena::pointer_in_nursery(subject as usize)); + let s_handle = scope.root_string_ptr(subject); + let subject_before = subject as usize; + + let re = crate::regex::js_regexp_new(heap_string(br"(young)-(\d+)"), heap_string(b"g")); + let re_handle = scope.root_raw_mut_ptr(re); + + // `re.lastIndex = { valueOf() { … } }`. + let coercer = crate::object::js_object_alloc(0, 1); + let coercer_handle = scope.root_raw_mut_ptr(coercer); + let fp = collect_then_zero as *const u8; + crate::closure::js_register_closure_arity(fp, 0); + let value_of = crate::closure::js_closure_alloc_singleton(fp); + let value_of_value = crate::value::js_nanbox_pointer(value_of as i64); + let value_of_key = heap_string(b"valueOf"); + coercer_handle.with_mut_ptr::(|obj| { + crate::object::js_object_set_field_by_name(obj, value_of_key, value_of_value) + }); + let coercer_value = coercer_handle + .with_mut_ptr::(|obj| crate::value::js_nanbox_pointer(obj as i64)); + re_handle.with_mut_ptr::(|hdr| unsafe { + (*hdr).last_index = coercer_value.to_bits(); + }); + + let cycles_before = copying_minor_cycles(); + let matched = s_handle.with_const_ptr::(|subject_now| { + re_handle.with_mut_ptr::(|re_now| { + crate::regex::js_regexp_exec(re_now, subject_now) + }) + }); + let cycles_after = copying_minor_cycles(); + + assert!( + cycles_after > cycles_before, + "subject not live: the lastIndex coercion must run a copying minor \ + (before={cycles_before}, after={cycles_after})" + ); + let subject_after = s_handle.with_const_ptr::(|p| p as usize); + assert_ne!( + subject_before, subject_after, + "subject not live: the collection must actually move the subject string" + ); + + // The discriminating check: pre-fix the match ran over the retired + // from-space copy of the subject, so `exec` returned null. + assert!( + !matched.is_null(), + r"/(young)-(\d+)/g must match the relocated subject" + ); + let matched_handle = scope.root_raw_mut_ptr(matched); + for (index, expected) in ["young-42", "young", "42"].into_iter().enumerate() { + let element = matched_handle.with_mut_ptr::(|arr| { + crate::array::js_array_get_f64(arr, index as u32) + }); + assert_eq!( + capture_text(element, index), + expected, + "capture {index} read from-space bytes" + ); + } + let last_index = + re_handle.with_const_ptr::(crate::regex::regex_last_index_offset); + assert_eq!( + last_index, 15, + "lastIndex must advance past the relocated match" + ); +} diff --git a/crates/perry-runtime/src/regex/exec.rs b/crates/perry-runtime/src/regex/exec.rs index c18c701949..8a117c05f9 100644 --- a/crates/perry-runtime/src/regex/exec.rs +++ b/crates/perry-runtime/src/regex/exec.rs @@ -33,22 +33,47 @@ pub extern "C" fn js_regexp_exec( return ptr::null_mut(); } + // Spec RegExpBuiltinExec step 4 is `ToLength(Get(R, "lastIndex"))`, and it + // runs before anything else. The ToNumber half of that coercion executes + // USER JS whenever `lastIndex` is coercible (`re.lastIndex = { valueOf() { + // … } }` — test262 prototype/exec/{success,failure}-lastindex-access covers + // exactly this), and user JS reaches back-edge safepoint polls, so a moving + // minor can relocate BOTH arguments inside the window. + // + // Do the coercion FIRST, with `re` and `s` rooted, and take the subject's + // payload borrow only afterwards. `string_as_str` hands out a `&str` into + // the inline WTF-8 bytes; rooting rewrites *slots*, never an already + // materialized borrow, so a borrow taken ahead of this call reads from-space + // for the entire match (#8428, the `HeapKeyBytes` doc states the rule). + // + // Audited in the same pass: `set_last_index_throwing` does NOT reopen the + // window. It reads the property attributes and stores a number; its only + // allocation is on the non-writable arm, which throws and therefore never + // returns to the borrow. + let scope = crate::gc::RuntimeHandleScope::new(); + let re_handle = scope.root_raw_mut_ptr(re); + let s_handle = scope.root_string_ptr(s); + let ((last_index_read, re), s) = s_handle.across_const::(|| { + re_handle + .across_mut::(|| re_handle.with_const_ptr(regex_last_index_offset)) + }); let str_data = string_as_str(s); unsafe { let regex = &*(*re).regex_ptr; let global = (*re).global; let sticky = (*re).sticky; + let has_indices = (*re).has_indices; // Per spec RegExpBuiltinExec, `lastIndex` drives the search start for // BOTH global and sticky regexes (and lastIndex is reset/updated for // either). A sticky match must additionally *anchor* at lastIndex. let use_last_index = global || sticky; - // Spec RegExpBuiltinExec step 4 reads `lastIndex` (Get → ToLength) once, - // up front and *before* the global/sticky branch (step 8). So the read - // — and any `valueOf`/`toString` side effect of a coercible lastIndex — - // is observed exactly once even for a non-global/non-sticky regex - // (test262 prototype/exec/{success,failure}-lastindex-access). - let last_index_read = regex_last_index_offset(re); + // `last_index_read` was taken at the top of the function, before the + // borrow: spec step 4 reads `lastIndex` (Get → ToLength) once, up front + // and *before* the global/sticky branch (step 8), so the read — and any + // `valueOf`/`toString` side effect of a coercible lastIndex — is observed + // exactly once even for a non-global/non-sticky regex (test262 + // prototype/exec/{success,failure}-lastindex-access). // Step 8: a non-global/non-sticky search always starts at 0; the value // read above only drives the search start for a stateful regex. let last_index = if use_last_index { last_index_read } else { 0 }; @@ -92,6 +117,16 @@ pub extern "C" fn js_regexp_exec( let match_byte_offset = full.start() + search_start_byte; let match_char_offset = super::exec_array::byte_index_to_utf16_index(str_data, match_byte_offset); + // Spec order (step 15 precedes the ArrayCreate of step 16), + // and it keeps `re` out of the window opened by the capture + // allocations below — the standard arm already does this. + if use_last_index { + let match_end_byte = full.end() + search_start_byte; + set_last_index_throwing( + re, + super::exec_array::byte_index_to_utf16_index(str_data, match_end_byte), + ); + } let arr = crate::array::js_array_alloc(caps.len() as u32); let scope = crate::gc::RuntimeHandleScope::new(); let arr_handle = scope.root_raw_mut_ptr(arr); @@ -109,13 +144,6 @@ pub extern "C" fn js_regexp_exec( crate::array::store_array_slot(arr, i, undefined.to_bits()); } } - if use_last_index { - let match_end_byte = full.end() + search_start_byte; - set_last_index_throwing( - re, - super::exec_array::byte_index_to_utf16_index(str_data, match_end_byte), - ); - } set_exec_array_metadata( arr_handle.get_raw_mut_ptr::(), str_data, @@ -129,7 +157,7 @@ pub extern "C" fn js_regexp_exec( LAST_EXEC_GROUPS.with(|g| *g.borrow_mut() = groups_obj); set_exec_array_groups(arr_handle.get_raw_mut_ptr::(), groups_obj); // Build indices array if `d` flag (hasIndices) is set - if (*re).has_indices { + if has_indices { set_exec_array_indices_fancy( arr_handle.get_raw_mut_ptr::(), str_data, @@ -243,24 +271,34 @@ pub extern "C" fn js_regexp_exec( *g.borrow_mut() = groups_handle.get_raw_mut_ptr::() }); - super::exec_array::set_exec_array_metadata_groups_fresh( - arr_handle.get_raw_mut_ptr::(), - s, - match_char_offset as f64, - groups_handle.get_raw_mut_ptr::(), - ); + // `.input` re-boxes the subject, so it must be the CURRENT + // address: the capture/groups allocations above can have + // moved it since `s` was refreshed. The helper itself + // performs no GC allocation, which is what makes the scoped + // `with_const_ptr` argument shape sound here. + s_handle.with_const_ptr::(|s_now| { + super::exec_array::set_exec_array_metadata_groups_fresh( + arr_handle.get_raw_mut_ptr::(), + s_now, + match_char_offset as f64, + groups_handle.get_raw_mut_ptr::(), + ) + }); } else { LAST_EXEC_GROUPS.with(|g| *g.borrow_mut() = ptr::null_mut()); - super::exec_array::set_exec_array_metadata_groups_fresh( - arr_handle.get_raw_mut_ptr::(), - s, - match_char_offset as f64, - ptr::null_mut(), - ); + // Current subject address — see the named-groups arm above. + s_handle.with_const_ptr::(|s_now| { + super::exec_array::set_exec_array_metadata_groups_fresh( + arr_handle.get_raw_mut_ptr::(), + s_now, + match_char_offset as f64, + ptr::null_mut(), + ) + }); } // Build indices array if `d` flag (hasIndices) is set - if (*re).has_indices { + if has_indices { set_exec_array_indices( arr_handle.get_raw_mut_ptr::(), str_data, diff --git a/crates/perry-runtime/src/regex/match_all.rs b/crates/perry-runtime/src/regex/match_all.rs index e1b38ae576..a4a91d99f5 100644 --- a/crates/perry-runtime/src/regex/match_all.rs +++ b/crates/perry-runtime/src/regex/match_all.rs @@ -236,18 +236,24 @@ pub extern "C" fn js_string_match_all_value( } else { 0 }; - let (re, start_index) = if raw != 0 && is_valid_regex_ptr(raw as *const RegExpHeader) { + let (start_index, re) = if raw != 0 && is_valid_regex_ptr(raw as *const RegExpHeader) { let re = raw as *const RegExpHeader; unsafe { if !(*re).global { throw_match_all_non_global_regex(); } - (re, crate::regex::regex_last_index_offset(re)) + // The ToLength coercion inside `regex_last_index_offset` runs user + // `valueOf`/`toString`, so it can move the regex header too — not + // just the subject `s_handle` already covers (#8428). + let re_handle = scope.root_raw_const_ptr(re); + re_handle.across_const::(|| { + re_handle.with_const_ptr(crate::regex::regex_last_index_offset) + }) } } else { ( - match_all_pattern_to_regex(pattern_value) as *const RegExpHeader, 0, + match_all_pattern_to_regex(pattern_value) as *const RegExpHeader, ) }; @@ -275,8 +281,21 @@ pub extern "C" fn js_string_match_all( if !(*re).global { throw_match_all_non_global_regex(); } - let matches = - materialize_match_all_results(s, re, crate::regex::regex_last_index_offset(re)); + // `regex_last_index_offset` ToLength-coerces `lastIndex`, which runs + // user `valueOf`/`toString` and can therefore move both arguments + // (#8428). Rust evaluates call arguments left to right, so the previous + // shape passed the PRE-coercion `s` — a from-space subject that + // `materialize_match_all_results` then rooted and snapshotted. Root + // first, coerce, then hand over the refreshed addresses. + let scope = crate::gc::RuntimeHandleScope::new(); + let s_handle = scope.root_string_ptr(s); + let re_handle = scope.root_raw_const_ptr(re); + let ((start_index, re), s) = s_handle.across_const::(|| { + re_handle.across_const::(|| { + re_handle.with_const_ptr(crate::regex::regex_last_index_offset) + }) + }); + let matches = materialize_match_all_results(s, re, start_index); alloc_regexp_string_iterator(matches) } } diff --git a/test-files/test_issue_8428_exec_lastindex_reentrant.ts b/test-files/test_issue_8428_exec_lastindex_reentrant.ts new file mode 100644 index 0000000000..94a8dd97e9 --- /dev/null +++ b/test-files/test_issue_8428_exec_lastindex_reentrant.ts @@ -0,0 +1,101 @@ +// parity-env: PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_GC_SCHEDULE_SEED=8428 PERRY_GC_SCHEDULE_RATE=1 PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1 +// +// #8428: RegExpBuiltinExec step 4 is `ToLength(Get(R, "lastIndex"))`, and the +// ToNumber half of it runs user JS whenever `lastIndex` is a coercible object. +// `js_regexp_exec` used to borrow the subject's inline WTF-8 payload BEFORE +// that coercion, so a moving minor inside the user callback left the borrow +// pointing at from-space for the whole match. The callbacks below churn enough +// young strings to reach several back-edge safepoint polls. + +function makeSubject(): string { + // Built at runtime so the subject is a young heap string rather than a + // constant baked into the binary. + let subject = "prefix"; + subject = subject + "-young"; + subject = subject + "-42-suffix"; + return subject; +} + +function movingLastIndex(label: string, result: number): object { + return { + valueOf(): number { + let junk = label; + for (let i = 0; i < 512; i++) { + junk = junk + "x"; + } + // Keep the churn observable so nothing above can be folded away. + if (junk.length !== label.length + 512) { + throw new Error("string churn was optimized away"); + } + return result; + }, + }; +} + +// (a) std-regex arm: the `regex` crate compiles this pattern. +const standardSubject = makeSubject(); +const standard = /(young)-(\d+)/g; +standard.lastIndex = movingLastIndex("standard", 0) as any; +const standardMatch = standard.exec(standardSubject)!; +console.log( + "standard", + standardMatch[0], + standardMatch[1], + standardMatch[2], + standardMatch.index, + standardMatch.input, + standard.lastIndex, +); + +// (b) fancy-regex arm: the lookbehind forces the fancy fallback, which reads +// the same borrow through a different matcher. +const fancySubject = makeSubject(); +const fancy = /(?<=prefix-)(young)-(\d+)/g; +fancy.lastIndex = movingLastIndex("fancy", 0) as any; +const fancyMatch = fancy.exec(fancySubject)!; +console.log( + "fancy", + fancyMatch[0], + fancyMatch[1], + fancyMatch[2], + fancyMatch.index, + fancyMatch.input, + fancy.lastIndex, +); + +// (c) a NON-zero coerced lastIndex additionally walks the borrow in +// `utf16_index_to_byte` to find the search start. +const offsetSubject = makeSubject(); +const offset = /(\d+)/g; +offset.lastIndex = movingLastIndex("offset", 7) as any; +const offsetMatch = offset.exec(offsetSubject)!; +console.log("offset", offsetMatch[0], offsetMatch.index, offset.lastIndex); + +// (d) named captures + `d` (hasIndices) exercise the groups/indices decoration +// built after the same borrow. +const namedSubject = makeSubject(); +const named = /(?young)-(?\d+)/dg; +named.lastIndex = movingLastIndex("named", 0) as any; +const namedMatch = named.exec(namedSubject)!; +console.log( + "named", + namedMatch[0], + namedMatch.groups!.word, + namedMatch.groups!.num, + JSON.stringify(namedMatch.indices), + named.lastIndex, +); + +// (e) `String.prototype.matchAll` reads `lastIndex` through the same coercion +// before it snapshots the subject. +const allSubject = makeSubject(); +const all = /(\w+)-(\d+)/g; +all.lastIndex = movingLastIndex("all", 0) as any; +const allResults = Array.from(allSubject.matchAll(all)).map((m) => m[0]); +console.log("matchAll", allResults.join("|")); + +// (f) `RegExp.prototype.test` routes global regexes through exec. +const testSubject = makeSubject(); +const tester = /young/g; +tester.lastIndex = movingLastIndex("test", 0) as any; +console.log("test", tester.test(testSubject), tester.lastIndex);