Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions changelog.d/7374-regexp-flags-stale-store.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
### Fixed

- **A `RegExp`'s `flags` string could be stored already-stale, permanently.**
`js_regexp_new` allocates the canonical flags string, then calls `gc_malloc`
for the header — a collection point — and then writes the *pre-collection*
flags pointer into `flags_ptr`. An evacuating minor inside that `gc_malloc`
moves the string, so a live `RegExpHeader` ends up holding a retired
from-space address for the rest of its life.

The pattern string was already rooted and re-read across exactly this
allocation, with a comment explaining why; the flags string was not. Now both
are.

This is the root cause of the `lookup_fancy_regex` cluster in #7341 — 5
catches reaching one read (`string_as_str((*re).flags_ptr)`) from four
different callers. Worth recording what it was *not*: a no-move window inside
`lookup_fancy_regex` closes 0/5, and rooting the search-value operand in
codegen closes 0/5. The helper and the call site were both innocent.

4 of the 5 cluster tests are now clean and a 6-line reproducer goes 6/6 → 0/6.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Correct the reproducer result direction.

Line 20 reports 6/6 → 0/6, but the PR objective states that the reproducer improved from 0/6 to 6/6. The current text reports a regression. Change the arrow to 0/6 → 6/6.

Proposed fix
-  4 of the 5 cluster tests are now clean and a 6-line reproducer goes 6/6 → 0/6.
+  4 of the 5 cluster tests are now clean and a 6-line reproducer goes 0/6 → 6/6.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
4 of the 5 cluster tests are now clean and a 6-line reproducer goes 6/6 → 0/6.
4 of the 5 cluster tests are now clean and a 6-line reproducer goes 0/6 → 6/6.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@changelog.d/7374-regexp-flags-stale-store.md` at line 20, Correct the
reproducer result in the changelog text by changing the reported direction from
“6/6 → 0/6” to “0/6 → 6/6”, preserving the surrounding wording.

15 changes: 15 additions & 0 deletions crates/perry-runtime/src/regex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -774,6 +774,19 @@ pub extern "C" fn js_regexp_new(
// GC-survivable source table all agree on the canonical form, and the
// header never holds the caller's possibly-temporary input flags.
let canonical_flags_ptr = js_string_from_str(flags_str);
// ★ #7341: root the canonical flags string too. The `gc_malloc` below is an
// allocation and therefore a collection point, exactly as the comment above
// `pattern_root` says — but only the PATTERN was rooted and re-read. The
// flags string is created here and stored into the header AFTER that
// allocation, so an evacuating minor in `gc_malloc` moved it and the header
// kept the pre-collection address. `flags_ptr` is then permanently stale in
// a live header: `lookup_fancy_regex` reads it through `string_as_str` and
// faults on retired from-space, which is 5 of the 31 catches in #7341
// (four different callers, all reaching that one read).
//
// The write barrier below already treated this as a real GC edge; what was
// missing is that the value written had to survive the allocation first.
let flags_root = scope.root_string_ptr(canonical_flags_ptr);
unsafe {
let raw = crate::gc::gc_malloc(header_size, crate::gc::GC_TYPE_OBJECT);
if raw.is_null() {
Expand All @@ -789,6 +802,8 @@ pub extern "C" fn js_regexp_new(
// so the incoming argument may name from-space; the handle is a mutable
// root the collector rewrote.
let pattern = pattern_root.get_raw_const_ptr::<StringHeader>();
// #7341: same re-read for the flags, for the same reason.
let canonical_flags_ptr = flags_root.get_raw_const_ptr::<StringHeader>();

(*ptr).regex_ptr = regex_ptr;
(*ptr).pattern_ptr = pattern;
Expand Down
Loading