fix(runtime): coerce lastIndex before borrowing the regex subject - #8446
Conversation
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 back-edge safepoint polls and can therefore run a moving minor today. `js_regexp_exec` took the subject's inline-payload borrow (`string_as_str`) BEFORE that coercion. Rooting rewrites slots, never an already materialized `&str`, so a collection inside the callback left the entire match running over from-space bytes: the match evaporates, or reads whatever was recycled into those pages. `String.prototype.matchAll` had the same defect through Rust's left-to-right argument evaluation — `materialize_match_all_results(s, re, regex_last_index_offset(re))` binds `s` and `re` before the coercion runs, so the snapshot phase rooted a pre-move subject. Both entry points now root the regex header and the subject, run the coercion, and read the refreshed addresses. This is also strictly closer to spec: step 4 precedes reading the flags (steps 5-7) and [[RegExpMatcher]] (step 12), which the old ordering did the other way round. The fancy-regex arm additionally writes `lastIndex` before the result array is allocated (spec step 15 precedes step 16's ArrayCreate, and it keeps `re` out of the window the capture allocations open), and the match array's `.input` decoration re-boxes the current subject rather than the address bound before those allocations. Audited in the same pass, no change needed: `set_last_index_throwing` reads property attributes and stores a number, so it cannot run user code (its only allocation is on the throwing arm, which never returns to the borrow); `js_string_match` never reads `lastIndex`; the `replace_*` callback paths already re-derive the subject per use. The new witness `gc::tests::runtime_roots::regexp_last_index` plants a `valueOf` that runs a copying minor and refills the retired Eden, asserts its subject was live both ways (a copying minor ran; the subject moved), and fails on pre-fix code.
|
Warning Review limit reached
Next review available in: 32 seconds Limit details: You’ve used all 8 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
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 |
|
Validated as part of an 11-PR batch (#8439, #8440, #8441, #8442, #8443, #8444, #8446,
One thing stated plainly: these are hardening, not demonstrated repairsI could not make the underlying bugs reproduce. My probe passes on unmodified That is consistent with the string audit having found these windows by reading rather than by |
1 participant
Closes #8428.
Problem
RegExpBuiltinExecstep 4 isToLength(Get(R, "lastIndex")). WhenlastIndexholds an object, the ToNumber half runs
OrdinaryToPrimitive— uservalueOf/toString, i.e. arbitrary JS, which reaches back-edge safepoint pollsand can therefore run a moving minor today.
js_regexp_exectook the subject's inline-payload borrow (string_as_str,regex/exec.rs:36) before that coercion (:51). Rooting rewrites slots,never an already-materialized
&str(theHeapKeyBytesdoc states the rule), soa collection inside the callback left the entire match running over from-space
bytes — the match silently evaporates, or returns text from whatever was
recycled into those pages.
String.prototype.matchAllhad the same defect through Rust's left-to-rightargument evaluation:
materialize_match_all_results(s, re, regex_last_index_offset(re))evaluates
sandrebefore the coercion runs, so the snapshot phase rootedand read a pre-move subject.
Fix
regex/exec.rs— rootreands, run thelastIndexcoercion, take theborrow from the refreshed addresses. This is also strictly closer to spec:
step 4 precedes reading the flags (steps 5–7) and
[[RegExpMatcher]](step 12), which the old ordering did in the other order.
regex/exec.rs— the fancy-regex arm now writeslastIndexbefore the resultarray is allocated, matching both the spec (step 15 precedes step 16's
ArrayCreate) and the standard arm; that also keeps
reout of the window thecapture allocations open.
regex/exec.rs— the match array's.inputdecoration re-boxes the subject,so it now reads the current address rather than the one bound before the
capture/groups allocations.
regex/match_all.rs— bothmatchAllentry points root the subject and theregex header across the coercion and hand over refreshed pointers.
Audit (issue asked for these explicitly)
set_last_index_throwingdoes not reopen the window. It reads the propertyattributes (a side-table read) and stores a number; Perry has no user-defined
lastIndexsetter on this path. Its only allocation is on the non-writable arm,which throws and therefore never returns to the borrow. Noted in the code.
regex/match_string.rs(js_string_match) never readslastIndex, so ithas no user-JS window ahead of its borrow;
js_string_match_valuealreadyroots the subject across the pattern coercion.
regex/replace_fn.rs/regex/replace_expand.rs— the user-callback pathsalready re-derive the subject per use (
cur_str = || string_as_str(s_handle…));the string/named paths run pure Rust over the borrow with a single terminal
allocation.
Not in scope, filed separately:
exec.rs/exec_array.rsstill readstr_data(andregex::Capturesborrowing it) after the result-array,capture-string and groups-object allocations. That is the allocation-point
window — the same latent class as #8423 — which is closed today only by the
#7682 guard forcing a conservative scan (hence a non-moving minor) at alloc
points. Closing it properly means the owned-snapshot restructure
match_all.rsalready carries, which is a much larger change to a hot path and deserves its own
measurement. Filed as #8449.
Validation
gc::tests::runtime_roots::regexp_last_index— plants avalueOfthat runs a copying minor and then refills the retired Eden with adistinctive pattern. It asserts its subject was live both ways (a copying
minor ran; the subject address actually changed) before checking the captures.
Sabotage-checked: reverting only
exec.rs/match_all.rstomainmakes itfail (
/(young)-(\d+)/g must match the relocated subject— the match evaporates),and it passes with the fix.
test-files/test_issue_8428_exec_lastindex_reentrant.ts— std-regex arm, fancy-regex (lookbehind) arm, a non-zero coerced
lastIndex(which additionally walks the borrow in
utf16_index_to_byte), named captureswith the
dflag,matchAll, andtest(). Runs underPERRY_GC_SCHEDULE_SEED/RATE=1+FORCE_EVACUATE+VERIFY_EVACUATION;byte-compared against Node 26.5.1.
scripts/raw_handle_debt.pystays at baseline (978) — the new code usesacross_*/with_*_ptrthroughout and adds no bare handle reads.No version bump (maintainer bumps at merge).