From 54f69b174c2fb6f300b2e881e154ef3350c84604 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 5 Sep 2026 09:17:27 +0200 Subject: [PATCH] fix(regex): never memoize a never-match program beside a missing fancy fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by the regex lane (#9796 item 2) against the construction cache added earlier on this branch. `REGEX_CACHE` holds a never-match placeholder for a lookbehind / backreference pattern whose real program lives in `FANCY_CACHE`, and the two maps have independent clear-on-overflow caps. `FANCY_CACHE` can therefore drop a pattern `REGEX_CACHE` still answers for, and `build_and_install_programs` then reads the pair (never-match, no fancy) — which matches nothing. Left to the maps alone that state heals: the next `REGEX_CACHE` clear makes the pattern recompile and repopulate both. A site-cache entry never heals — a construction that hits it is born built and never consults the maps again — so memoizing the incoherent pair makes a lookbehind literal PERMANENTLY non-matching, silently, for the life of the thread. That permanence is this branch's regression, and this is its fix: the triple is only remembered against the text when it is coherent. The placeholder becomes a process-wide singleton so the question is an `Arc::ptr_eq` rather than a pattern-text compare. This is the narrow guard, not the whole repair: #9796 item 2 also REBUILDS the missing fallback so the header itself stops mis-matching, which fixes the underlying transient defect (present on main) that this only declines to make permanent. Keep both; this one becomes a defensive invariant once that lands. The same hole exists for `REPEAT_MATCHER_CACHE` — a cleared repeat matcher is memoized as `repeat: None` beside a real std program, changing capture semantics — and is reported to that lane rather than guessed at here. Test: `an_incoherent_program_pair_is_never_memoized_by_the_site_cache` reads `Some(true)` and then `-1` without the guard. --- .../9764-regex-site-cache-coherence.md | 8 +++ crates/perry-runtime/src/regex.rs | 51 +++++++++++++++-- crates/perry-runtime/src/regex/lazy.rs | 36 +++++++++--- crates/perry-runtime/src/regex/tests.rs | 56 +++++++++++++++++++ 4 files changed, 137 insertions(+), 14 deletions(-) create mode 100644 changelog.d/9764-regex-site-cache-coherence.md diff --git a/changelog.d/9764-regex-site-cache-coherence.md b/changelog.d/9764-regex-site-cache-coherence.md new file mode 100644 index 0000000000..db86e4a849 --- /dev/null +++ b/changelog.d/9764-regex-site-cache-coherence.md @@ -0,0 +1,8 @@ +Fixed the RegExp construction cache remembering an incoherent program pair. The +compiled-program caches are capped and cleared independently, so `FANCY_CACHE` +can drop a lookbehind/backreference pattern while `REGEX_CACHE` still answers +for it with the never-match placeholder. The maps heal on their next clear; a +memoized entry does not, because a construction that hits it is born built and +never consults the maps again — so a lookbehind literal could be born +permanently non-matching. The triple is now only remembered against the text +when it is coherent. diff --git a/crates/perry-runtime/src/regex.rs b/crates/perry-runtime/src/regex.rs index 714a4f81c6..5a0608c559 100644 --- a/crates/perry-runtime/src/regex.rs +++ b/crates/perry-runtime/src/regex.rs @@ -242,6 +242,24 @@ pub(crate) unsafe fn regex_header_finalize_for_gc(re: *mut RegExpHeader) { regex_header_clear_dead_for_gc(re as usize); } +/// Model one `evict_regex_cache_if_full` overflow clear of `FANCY_CACHE`. +/// +/// The production guard's entire action on overflow is `cache.clear()` (see +/// [`evict_regex_cache_if_full`]) — this seam re-implements no predicate, only +/// spares a test 512 fancy compiles to reach the cap. What matters for the test +/// is that the two program caches have INDEPENDENT caps, so one can drop a +/// pattern the other still holds. +#[cfg(all(test, feature = "regex-engine"))] +pub(crate) fn test_clear_fancy_cache() { + FANCY_CACHE.with(|fc| fc.borrow_mut().clear()); +} + +/// The same, for `REGEX_CACHE` — the clear that lets an incoherent pair heal. +#[cfg(all(test, feature = "regex-engine"))] +pub(crate) fn test_clear_std_cache() { + REGEX_CACHE.with(|c| c.borrow_mut().clear()); +} + #[cfg(test)] pub(crate) fn test_regex_pointer_entry_exists(addr: usize) -> bool { REGEX_POINTERS.with(|table| table.borrow().contains(&addr)) @@ -481,6 +499,29 @@ fn evict_regex_cache_if_full(cache: &mut HashMap<(String, String), V>) { } } +/// The never-match program `REGEX_CACHE` holds for a pattern only `fancy-regex` +/// can match (lookbehind, backreferences), and for one both engines rejected. +/// +/// It is a process-wide singleton so that "is this entry a placeholder?" is an +/// `Arc::ptr_eq`, not a pattern-text compare. That question has to be asked +/// before anything MEMOIZES a `(std, fancy, repeat)` triple: a placeholder with +/// no fancy twin is an incoherent triple, and memoizing it makes the pattern +/// permanently non-matching (see `lazy::build_and_install_programs`). +#[cfg(feature = "regex-engine")] +fn never_match_program() -> Arc { + static NEVER: std::sync::OnceLock> = std::sync::OnceLock::new(); + NEVER + .get_or_init(|| Arc::new(Regex::new(r"[^\s\S]").unwrap())) + .clone() +} + +/// Is `program` the shared never-match placeholder rather than a real compiled +/// program? See [`never_match_program`]. +#[cfg(feature = "regex-engine")] +pub(crate) fn is_never_match_program(program: &Arc) -> bool { + Arc::ptr_eq(program, &never_match_program()) +} + /// Compile `(pattern, flags)` into the caches if absent, reporting whether /// SOME engine accepted the flag-prefixed pattern. One NFA build total. /// @@ -526,8 +567,8 @@ fn compile_and_cache_regex_checked(pattern: &str, flags: &str) -> bool { // prefix the flags imply. Shared with `lazy::std_engine_syntax_ok` so the // eager syntax check and this build can never inspect different strings. let regex_pattern = lazy::flag_prefixed_pattern(pattern, flags); - let regex = match build_std_regex(®ex_pattern) { - Ok(re) => re, + let regex: Arc = match build_std_regex(®ex_pattern) { + Ok(re) => Arc::new(re), Err(_) => { // Pattern has features regex crate doesn't support // (lookbehind, lookahead). Try fancy-regex which supports @@ -554,7 +595,7 @@ fn compile_and_cache_regex_checked(pattern: &str, flags: &str) -> bool { if !fancy_ok { return false; } - Regex::new(r"[^\s\S]").unwrap() + never_match_program() } }; if crate::hot_diag::regex_on() { @@ -563,7 +604,7 @@ fn compile_and_cache_regex_checked(pattern: &str, flags: &str) -> bool { REGEX_CACHE.with(|cache| { let mut cache = cache.borrow_mut(); evict_regex_cache_if_full(&mut cache); - cache.insert((pattern.to_string(), flags.to_string()), Arc::new(regex)); + cache.insert((pattern.to_string(), flags.to_string()), regex); }); true } @@ -587,7 +628,7 @@ fn get_or_compile_regex(pattern: &str, flags: &str) -> Arc { } // Both engines rejected it (validation normally throws before this // point) — keep the historical behavior: cache + return never-match. - let arc = Arc::new(Regex::new(r"[^\s\S]").unwrap()); + let arc = never_match_program(); evict_regex_cache_if_full(&mut cache); cache.insert((pattern.to_string(), flags.to_string()), arc.clone()); arc diff --git a/crates/perry-runtime/src/regex/lazy.rs b/crates/perry-runtime/src/regex/lazy.rs index 438d8eca4b..b18396afe0 100644 --- a/crates/perry-runtime/src/regex/lazy.rs +++ b/crates/perry-runtime/src/regex/lazy.rs @@ -257,15 +257,33 @@ fn build_and_install_programs(re: *const RegExpHeader) { }); // Remember the built programs against the pattern text, so the next // construction of the same literal is born built (`js_regexp_new`). - super::site_cache::install_programs( - &pattern, - &flags, - super::site_cache::Programs { - std: std_arc.clone(), - fancy: fancy_arc.clone(), - repeat: repeat_arc.clone(), - }, - ); + // + // ONLY a coherent triple may be memoized. `get_or_compile_regex` answers + // from `REGEX_CACHE`, and for a lookbehind / backreference pattern the + // entry it finds there is the never-match PLACEHOLDER whose real program + // lives in `FANCY_CACHE`. Those two maps have independent + // clear-on-overflow caps (`evict_regex_cache_if_full`), so `FANCY_CACHE` + // can have dropped the pattern while `REGEX_CACHE` still holds its + // placeholder — and then the pair read here is (never-match, None), which + // matches nothing. + // + // Left alone, the maps heal: the next `REGEX_CACHE` clear makes the + // pattern recompile and repopulate both. A site-cache entry never heals — + // a construction that hits it is born built and never consults the maps + // again — so memoizing the incoherent pair would make a lookbehind literal + // PERMANENTLY non-matching, silently, for the life of the thread. Declining + // to memoize costs one recompile and keeps the self-healing behaviour. + if !(super::is_never_match_program(&std_arc) && fancy_arc.is_none()) { + super::site_cache::install_programs( + &pattern, + &flags, + super::site_cache::Programs { + std: std_arc.clone(), + fancy: fancy_arc.clone(), + repeat: repeat_arc.clone(), + }, + ); + } let regex_ptr = Arc::into_raw(std_arc) as *mut Regex; let fancy_ptr: *const () = fancy_arc.map_or(std::ptr::null(), |arc| Arc::into_raw(arc) as *const ()); diff --git a/crates/perry-runtime/src/regex/tests.rs b/crates/perry-runtime/src/regex/tests.rs index e41c2abc61..511a5043ea 100644 --- a/crates/perry-runtime/src/regex/tests.rs +++ b/crates/perry-runtime/src/regex/tests.rs @@ -1649,6 +1649,62 @@ fn global_replace_substitutes_at_every_empty_match() { assert_eq!(string_as_str(out), "[a][]"); } +/// #9764 + the regex lane's #9796 finding: the construction cache must never +/// memoize an INCOHERENT program triple. +/// +/// `REGEX_CACHE` holds a never-match placeholder for a lookbehind / +/// backreference pattern whose real program lives in `FANCY_CACHE`, and the two +/// maps have independent clear-on-overflow caps. So `FANCY_CACHE` can drop a +/// pattern while `REGEX_CACHE` still answers for it, and a build then reads +/// (never-match, no fancy) — a pair that matches nothing. +/// +/// The maps heal on their next clear. A site-cache entry does not: a +/// construction that hits it is born built and never consults the maps again. +/// Memoizing the incoherent pair therefore makes the literal PERMANENTLY +/// non-matching. Without the guard in `build_and_install_programs` the first +/// assertion below reads `Some(true)` and the last one reads `-1`. +#[test] +fn an_incoherent_program_pair_is_never_memoized_by_the_site_cache() { + let _lock = crate::gc::global_side_table_test_lock(); + // A fixed-width lookbehind: the linear engine rejects it, so the standard + // cache gets the placeholder and the real program goes to `FANCY_CACHE`. + let pat = r"(?<=Q9zq)zz"; + site_cache::test_reset(); + test_clear_std_cache(); + test_clear_fancy_cache(); + + let re1 = js_regexp_new(make_string(pat), make_string("")); + assert_eq!( + js_string_search_regex(make_string("aQ9zqzz"), re1), + 5, + "the fancy fallback matches on a coherent runtime" + ); + + // `FANCY_CACHE` overflows and clears; `REGEX_CACHE` keeps the placeholder. + // Drop the site entry too, so the next construction takes the build path. + site_cache::test_reset(); + test_clear_fancy_cache(); + + let re2 = js_regexp_new(make_string(pat), make_string("")); + let _ = js_string_search_regex(make_string("aQ9zqzz"), re2); + assert_eq!( + site_cache::test_has_programs(pat, ""), + Some(false), + "a (never-match, no-fancy) pair must not be remembered against the text" + ); + + // The maps heal — and because nothing incoherent was memoized, so does the + // pattern. With the pair memoized this construction is born built from it + // and returns -1 forever. + test_clear_std_cache(); + let re3 = js_regexp_new(make_string(pat), make_string("")); + assert_eq!( + js_string_search_regex(make_string("aQ9zqzz"), re3), + 5, + "the literal must recover once the program caches heal" + ); +} + /// The construction cache (`regex::site_cache`): once a header built from /// some `(pattern, flags)` has been executed, the next construction of the /// same text is born built — it shares the executed header's program and