fix(regex): do not cap RegExp operations by work (#10164) - #10176
proggeramlug wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe regex work allowance changes to ChangesRegex work policy
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant RuntimeRootTest
participant PerexAPI
participant GC
RuntimeRootTest->>PerexAPI: Execute regex with resource budgets
PerexAPI->>GC: Poll during QUANTUM slices
PerexAPI-->>RuntimeRootTest: Return no-match result and charged work
Merge Risk: ⚪ Minimal · up to The feature-disabled build path excludes the Perex-specific test module, so no actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Full perry-runtime lib suite, replayed locally at
|
…oss calls (#10166) A search per JavaScript call (an exec or test loop, matchAll's next(), search) bound its subject and program from scratch every time: binding a subject decodes the entire string and binding a program revalidates every word. A loop over one string therefore did O(n) binding work per call and O(n²) overall, and `.test()` paid a program validation per call. Perex 0.1.3 adds constant-work rebinding for what the host already validated: BoundSubject::new_counted(storage, utf16_len) and a ProgramWitness from BoundProgram::witness() that BoundProgram::new_witnessed checks against the program's length and header. Perry now keeps both, as plain data, with no allocation and nothing new traced: - Program: ProgramCell gains `witness: Option<ProgramWitness>` in its pointer-free prefix, beside the words it describes. The first validating bind records it; later binds use new_witnessed, falling back to validation (recording a fresh witness) if it does not match. A recompile emits a new cell that starts with none, so a witness can never describe other words. RegExpHeader stays 56 bytes. Debug builds assert no witness is written while a view of the cell's words is live. - Subject: StringHeader gains STRING_FLAG_WTF8_VALIDATED. Perry strings are not all valid WTF-8 (raw Buffer/FFI payloads reach regex operations), so nothing is assumed: the first bind decodes, and only if that succeeds and the decoded UTF-16 length equals the header's is the header marked; later binds use new_counted. A validated header is already shared, so its payload is never mutated in place. init_string_header strips the bit from every constructed string, and the in-place writers (js_string_append, js_string_append_chain) clear it, so it never reaches another string; concat's memo check ignores it. Deliberately excluded: a cross-call lastIndex position hint for non-ASCII exec loops. Recognising the same string across calls without a traced reference would need a heap generation counter bumped on every free and move path, and one missed path gives silent wrong answers. The remaining cost is one seek from the nearer end of the subject per call on non-ASCII subjects; it is charged but uncapped (#10176), so those loops finish, but are not linear. Tests: - string::tests_validated_flag: slice, substring, trim, concat, repeat, padStart, toUpperCase, join, string_copy_range and js_string_from_bytes_known_utf16 (passed the source's whole flags word) never inherit the bit; both in-place append paths clear it. - gc::tests::runtime_roots::perex_cross_call: a program cell records its witness on first bind and a recompiled program's new cell has none; a foreign equal-header witness (x(b) on x(a)) still answers with the cell's own words; a mismatched witness falls back and is replaced; a subject is marked only with an exact length, never with a corrupted utf16_len or malformed bytes; marks survive moving collections; a witness write under a live view is caught. - perex_reuse's accounting now expects one validation per program. Fault injection, each confirmed to fail its test: marking without the length check; never recording a witness; a mismatch without fallback; removing the view guard; keeping the whole flags word in init_string_header; keeping the bit in the in-place writers. Requires perex 0.1.3 (crates.io checksum 060b4682849d20ebcba05d68f9584a1bba20af4b7838c688cfb37af572f562f4), resolved once with CARGO_RESOLVER_INCOMPATIBLE_PUBLISH_AGE=allow with the maintainer's approval; ordinary --locked builds use it without the override. Claude-Session: https://claude.ai/code/session_01Da12JXeG5XuVBma5yWp5C9
|
Heads-up for landing this on current The fix is one line, passing |
Every RegExp operation ran under one fixed Budget of 100,000,000 work units. Valid programs Node completes threw `RangeError: Regular expression work limit exceeded`: a 32,000-unit non-ASCII split and a 60,000-unit global replace (from the per-search seek charge), and after #10165's fixes even linear splits and replaces of 11-15 million units. No finite allowance separates valid programs from pathological ones. Perex charges per subject unit an amount set by the program, not the subject: about 1 for `/x/`, 9 for `/\w+/g`, 60 for `/([a-z]+)([0-9]+)/g`, over 200 for a 32-unit lookahead. Any cap therefore throws on some large linear input, while a quadratic pattern on a short subject never reaches it. JavaScript engines never abort matching for work. WORK becomes usize::MAX. Searches still run in QUANTUM slices with a GC poll between them, so collection and cancellation keep working, and the scratch, program and output memory limits are unchanged. A catastrophic pattern now runs as long as it does in Node instead of throwing. The existing tests that exercise ExecError::WorkLimit all pass their own small budgets, so they still cover the error mapping and accounting. Test: gc::tests::runtime_roots::perex_work_policy runs one valid, linear search that charges about 1.2e8 units (a failing 32-unit lookahead at every position, with no required literal that admission could reject up front) and asserts it completes and charges more than the former limit. It fails when the old 100,000,000 cap is restored. Claude-Session: https://claude.ai/code/session_01Da12JXeG5XuVBma5yWp5C9
|
Rebased onto Both PRs merged cleanly as text, but the combined tree did not compile. This PR's Re-verified on the rebased head (perrymaster,
Before the rebase, the full replay of the old head |
c61d617 to
b895f32
Compare
…oss calls (#10166) A search per JavaScript call (an exec or test loop, matchAll's next(), search) bound its subject and program from scratch every time: binding a subject decodes the entire string and binding a program revalidates every word. A loop over one string therefore did O(n) binding work per call and O(n²) overall, and `.test()` paid a program validation per call. Perex 0.1.3 adds constant-work rebinding for what the host already validated: BoundSubject::new_counted(storage, utf16_len) and a ProgramWitness from BoundProgram::witness() that BoundProgram::new_witnessed checks against the program's length and header. Perry now keeps both, as plain data, with no allocation and nothing new traced: - Program: ProgramCell gains `witness: Option<ProgramWitness>` in its pointer-free prefix, beside the words it describes. The first validating bind records it; later binds use new_witnessed, falling back to validation (recording a fresh witness) if it does not match. A recompile emits a new cell that starts with none, so a witness can never describe other words. RegExpHeader stays 56 bytes. Debug builds assert no witness is written while a view of the cell's words is live. - Subject: StringHeader gains STRING_FLAG_WTF8_VALIDATED. Perry strings are not all valid WTF-8 (raw Buffer/FFI payloads reach regex operations), so nothing is assumed: the first bind decodes, and only if that succeeds and the decoded UTF-16 length equals the header's is the header marked; later binds use new_counted. A validated header is already shared, so its payload is never mutated in place. init_string_header strips the bit from every constructed string, and the in-place writers (js_string_append, js_string_append_chain) clear it, so it never reaches another string; concat's memo check ignores it. Deliberately excluded: a cross-call lastIndex position hint for non-ASCII exec loops. Recognising the same string across calls without a traced reference would need a heap generation counter bumped on every free and move path, and one missed path gives silent wrong answers. The remaining cost is one seek from the nearer end of the subject per call on non-ASCII subjects; it is charged but uncapped (#10176), so those loops finish, but are not linear. Tests: - string::tests_validated_flag: slice, substring, trim, concat, repeat, padStart, toUpperCase, join, string_copy_range and js_string_from_bytes_known_utf16 (passed the source's whole flags word) never inherit the bit; both in-place append paths clear it. - gc::tests::runtime_roots::perex_cross_call: a program cell records its witness on first bind and a recompiled program's new cell has none; a foreign equal-header witness (x(b) on x(a)) still answers with the cell's own words; a mismatched witness falls back and is replaced; a subject is marked only with an exact length, never with a corrupted utf16_len or malformed bytes; marks survive moving collections; a witness write under a live view is caught. - perex_reuse's accounting now expects one validation per program. Fault injection, each confirmed to fail its test: marking without the length check; never recording a witness; a mismatch without fallback; removing the view guard; keeping the whole flags word in init_string_header; keeping the bit in the in-place writers. Requires perex 0.1.3 (crates.io checksum 060b4682849d20ebcba05d68f9584a1bba20af4b7838c688cfb37af572f562f4), resolved once with CARGO_RESOLVER_INCOMPATIBLE_PUBLISH_AGE=allow with the maintainer's approval; ordinary --locked builds use it without the override. Claude-Session: https://claude.ai/code/session_01Da12JXeG5XuVBma5yWp5C9
…h limit (#10164) `String.prototype.replace`, `replaceAll` and `split` threw "Regular expression memory limit exceeded" once an operation's list reached 8,388,608 entries: `List::push` refused past `SCRATCH_BYTES / 8`. A replacement stores three entries per output piece and a callback replace appends two pieces per match, so `'ab12 cd345;'.repeat(n).replace(/[0-9]+/g, cb)` threw from exactly 699,050 records, where Node finishes in about 235 ms. That limit is for native scratch buffers; these lists are ordinary GC arrays whose length follows the subject. The cap is removed. The lists are bounded by what can be allocated, and output by the existing maximum-string-length check on piece units. The work budget has been unlimited since #10176 for the same reason. Test: a `replaceAll` over one match more than the former cap allows now produces the complete output; with the cap restored the same test aborts on the thrown error. Claude-Session: https://claude.ai/code/session_01RJkA4Fhqz9J5F5fzDk5HWv
Fixes the
RangeErrorhalf of #10164.Problem
Every RegExp operation ran under one fixed
Budgetof 100,000,000 work units (perex_api::WORK), so valid programs that Node completes threwRangeError: Regular expression work limit exceeded:splitand a 60,000-unit globalreplace— the per-search seek charge summed to about n²/4 (the cost placement is on bug(regex): split and replace throw "Regular expression work limit exceeded" on 32,000-unit strings Node handles in under a millisecond #10164);split(15M units) andreplace(11M units) reach the end of the input and throw, where Node takes 240 and 281 ms.Why no cap
No finite allowance separates valid programs from pathological ones. Perex's charge per subject unit is set by the program, not the subject:
/x/g, no match/\w+/g/[,; ]+/, sticky attempt per position/([a-z]+)([0-9]+)/g/\w(?=[\w,; ]{32})/gAny cap throws on some large linear input, and a quadratic pattern on a short subject never reaches it. JavaScript engines never abort matching for work. The Perex maintainer set out three options — no cap, a length-scaled cap, or a wall-clock/cancellation guard — and the maintainer chose no cap, for parity.
Change
WORKbecomesusize::MAX, with the reasoning documented on the constant.QUANTUMslices with a GC poll between them, so collection and cancellation keep working.The existing tests that exercise
ExecError::WorkLimitall pass their own small budgets (0, 3, 37, …), so they still cover the error mapping and accounting.Test
gc::tests::runtime_roots::perex_work_policyruns one valid, linear search that charges about 1.2e8 units and asserts that it completes and charged more than the former limit. The search is a failing 32-unit lookahead at every position of 540,000 units.The pattern deliberately has no required literal. My first candidate,
\w(?=\w{32}!), charged only 600,040 units, because admission rejects a search whose required!never occurs in one pass. So a witness has to defeat that shortcut to prove anything.gc::tests::runtime_roots::perex*tests and all 71regex::unit tests pass, and rustfmt is clean.Related
https://claude.ai/code/session_01Da12JXeG5XuVBma5yWp5C9
Summary by CodeRabbit