fix(runtime): root the normalize subject across form coercion - #8451
fix(runtime): root the normalize subject across form coercion#8451proggeramlug wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe change roots the subject string during ChangesString normalization GC safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change roots the subject string while form coercion can trigger garbage collection, preventing stale-string reads during normalization. No actionable merge-blocking risk remains after normal checks and review. Possibly related issues
Possibly related PRs
🚥 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 |
`js_string_normalize` borrowed the subject string's inline WTF-8 payload
before coercing its `form` argument, then read that borrow afterwards.
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.
Coerce first, root the subject across the coercion with a RuntimeHandleScope,
and borrow only from the address `across_const` hands back. Both cfg arms of
the normalization match read the re-derived borrow. 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 (#2782).
Fixes #8426
`raw_handle_debt.py`'s per-module rule locks any unlisted runtime module at
zero bare `get_raw_{mut,const}_ptr` reads. The new test had three: two in
argument position (the closure and form-object pointers) and one post-call
reload (the subject's address after the coercion).
Convert them to the sanctioned combinators — `with_mut_ptr` for the argument
positions, `across_const` for the reload, which hands back the
post-collection address directly so the pre-call one is never nameable.
Ratchet returns to baseline 978 with no ceiling raised.
Re-ran the sabotage check after converting: with the fix reverted the test
still SIGSEGVs, so the conversion did not defang the regression it guards.
3908ed7 to
bbbb9c3
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Superseded by #8483. The implementation in #8451 remains correct, but after #8481 landed it acquired the single |
|
Superseded by #8483 — your fix rebased onto current The blocker changed while this sat open: the original Fork branch, so the gate fix could not be pushed here. Full validation is on #8483. |
1 participant
Fixes #8426.
Problem
js_string_normalizeborrowed the subject string's inline WTF-8 payload, then coerced itsformargument, then read the borrow:The coercion is a collection point twice over:
toString, whose loop back-edge polls (default-on since fix(gc): make the moving-loop poll default ON in the code, not just the doc (#7690, #7682) #7721) run a moving minor — this is reachable from ordinary user code today;js_string_materialize_to_heap), so even a plains.normalize("NFC")allocates inside the window. That arm is latent — an alloc-point collection forces a conservative stack scan, which makes the copying minor ineligible — but it is the same window.Either can evacuate a young subject. Rooting rewrites slots, never an already-materialized
&str, so the normalization pass then reads retired from-space.Fix
Coerce the form first, root the subject across the coercion in a
RuntimeHandleScope, and take the borrow only from the addressacross_consthands back. Nothing in the coercion needsstr_data, and nothing after it allocates through the GC before the result is built from an owned RustString.Both
cfgarms (string-normalizeon/off) read the re-derived borrow. The observable orderings are unchanged — ToString still runs before the form is validated, so a Symbol form throwsTypeErrorrather than the invalid-formRangeError(#2782). The existingtest_gap_2786_2880_2782_2789_string_semantics.tsstill passes, and the new fixture pins both orderings explicitly.Validation
Fault demonstrated before the fix, with
test-files/test_issue_8426_normalize_reentrant.tsunder the issue's knobs (PERRY_GC_SCHEDULE_SEED=1 PERRY_GC_SCHEDULE_RATE=1 PERRY_GC_SCHEDULE_ALLOC_KB=0 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=64 PERRY_GC_FORCE_EVACUATE=1):obj_type=3isGC_TYPE_STRING— the subject. After the fix the same binary/knobs run clean (115,025 copying minors, 27,107 objects moved, exit 0) and stdout is byte-identical to Node 26.5.1 in both default and stress modes.One note for anyone reproducing: the subject's construction matters. A
+=accumulator chain leaves its buffer outside the movable nursery, so a subject built that way never relocates and the fixture passes with or without the bug. The fixture usesjoin(""); a measurement pass confirmedjoin/slice/repeat/toLowerCase/substringsubjects all evacuate mid-window while the+=one does not. The fixture carries a comment saying so.Gate
The
.tsfixture only runs in the full tier and only faults under stress knobs, so it is a regression fixture, not a gate. The gate is acargo-test-visible unit test,gc::tests::runtime_roots::string_normalize_form— it builds a form object whosetoStringforces a real copying minor and asserts three things together, so it cannot pass vacuously:It runs under
ProtectionModeGuard::PoisonOnlyso a stale read is guaranteed to be detected rather than left to whatever the allocator recycled into the page.Sabotage-checked: with the fix reverted and the test kept, it fails — SIGSEGV under poison, and a clean
left: 16, right: 15assertion without it. A green run means the detector works.No version bump (maintainer bumps at merge).
Summary by CodeRabbit
String.prototype.normalizereliability when form coercion triggers memory cleanup.