fix(gc): a RegExp's flags string was stored into the header already stale - #7374
Conversation
…tale 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 relocates the string, so a live RegExpHeader holds a retired from-space address permanently, and every later string_as_str((*re).flags_ptr) reads it. The pattern string was already rooted and re-read across exactly this allocation, with a star-marked comment explaining the hazard. The flags string, created after that comment and stored beside it, was not. Root cause of the lookup_fancy_regex cluster in #7341: 5 catches, four different callers, all reaching that one read. Two wrong fixes are recorded in the changelog because both looked obvious and both measured zero: a no-move window inside lookup_fancy_regex (0/5), and rooting the search-value operand in codegen's replace lowering (0/5, even after moving the re-read to the point of use). The helper and the call site were both innocent; the header had been carrying a dead pointer since construction. 4/5 cluster tests clean, 6-line reproducer 6/6 -> 0/6.
📝 WalkthroughWalkthrough
ChangesRegExp flags GC fix
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@changelog.d/7374-regexp-flags-stale-store.md`:
- 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.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 90f9a4fc-5a7e-41f8-b92c-dc1724c51d05
📒 Files selected for processing (2)
changelog.d/7374-regexp-flags-stale-store.mdcrates/perry-runtime/src/regex.rs
| `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. |
There was a problem hiding this comment.
📐 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.
| 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.
…ale (#7376) * fix(gc): a Symbol's description was stored into the header already stale alloc_symbol calls gc_malloc -- a collection point -- and then writes the description pointer its caller computed BEFORE that call. An evacuating minor moves the description string, so a live SymbolHeader holds a retired from-space address and js_symbol_to_string faults reading it through str_from_header. Same shape and same fix as RegExpHeader::flags_ptr (#7374): root across the allocation, re-read after. 3/3 cluster tests clean, byte-identical to Node. LEFT OPEN DELIBERATELY: the header is allocated GC_TYPE_STRING, whose payload the collector treats as opaque, so a fresh symbol's description is never marked or rewritten after construction. The comment at the site already says this -- 'kept alive through the SYMBOL_REGISTRY (for registered symbols) or not at all (for fresh symbols ... which is fine for test workloads)'. This makes the STORED value correct; keeping it alive for the symbol's lifetime is a separate fix, noted in the changelog and tracked in #7341. * docs: the symbol-description fix closes 6 catches, not 3 js_object_literal_infer_computed_function_name was triaged as a separate cluster because its backtrace names a different frame. It reaches the same stale (*sym_ptr).description through infer_symbol_function_name, so the #7376 fix closes it too -- verified 3/3 clean, byte-identical to Node. Two distinct faulting frames, one root cause. Worth recording: grouping catches by frame #0 is the right first cut, but it over-counts clusters whenever one bad field has several readers. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
Closes the
lookup_fancy_regexcluster in #7341 — 5 of 31 catches, four different callers reaching one read.The bug
js_regexp_newallocates the canonical flags string, then callsgc_mallocfor the header — a collection point — and then writes the pre-collection pointer:A live
RegExpHeaderthen holds a retired from-space address permanently, and every laterstring_as_str((*re).flags_ptr)reads it.The pattern string was already rooted and re-read across that exact allocation, with a ★-marked comment explaining the hazard. The flags string, created three lines later and stored beside it, was not.
Two wrong fixes, recorded so nobody repeats them
Both looked obvious and both measured exactly zero:
lookup_fancy_regex— the faulting helper. 0/5.replacelowering. 0/5, even after confirming in the IR that the root landed and moving the re-read to the point of use.The helper was innocent. The call site was innocent. The header had been carrying a dead pointer since construction.
Verification
4/5 cluster tests clean, 5/5 byte-identical to Node, 43/43 regex unit tests. A 6-line reproducer goes 6/6 → 0/6.
Summary by CodeRabbit
Release Notes