mruby-regexp: stop a repetition on its empty iteration - #7071
Conversation
A repetition whose body can match empty runs one final iteration that
consumes nothing. That iteration is the one that ends the loop, and the group
inside it keeps what it captured there: the empty string at the position the
loop stopped at. `pike_vm()` stops one iteration earlier, so the group still
holds the text of the last iteration that consumed something, or nothing at
all when there was none. A body that prefers its empty branch does not end
the loop either, which moves the whole match.
```ruby
/(a?)*/.match("a")[1] # CRuby: "", mruby: "a"
/(a?)*/.match("a").begin(1) # CRuby: 1, mruby: 0
/(a*)*b/.match("aab")[1] # CRuby: "", mruby: "aa"
/(a?)+/.match("a")[1] # CRuby: "", mruby: "a"
/(a?)*/.match("b")[1] # CRuby: "", mruby: nil
/(a*)*b/.match("b")[1] # CRuby: "", mruby: nil
/(|a)*/.match("a")[0] # CRuby: "", mruby: "a"
```
The capture reaches results the caller sees:
```ruby
"ab".split(/(a?)*/, -1) # CRuby: ["", "", "b", "", ""], mruby: ["", "a", "b", ""]
"ab".scan(/(a?)*/) # CRuby: [[""], [""], [""]], mruby: [["a"], [nil], [nil]]
```
`e*` compiles to a `RE_SPLIT` head, the body, and a backward `RE_JMP` to that
head; `e+` closes with a backward fork instead. `add_thread()` walks each `pc`
once per step, marking `visited[pc]` and returning on a revisit, which is what
makes an epsilon loop terminate. The empty iteration is walked and writes its
`RE_SAVE`s into a capture slot, then reaches a `pc` the same walk already
marked, so that slot dies. The thread that leaves the loop was queued by the
head before the body ran, and carries the slot from before the iteration.
Give the backward edge the rule Onigmo applies. Reaching it with the loop head
already marked at this position means the body just ran empty, so leave the
loop from there, carrying that iteration's captures. The head explores the
body before the exit, so this path claims the exit `pc` first and outranks the
stale exit the head queued.
That alone still loses the empty iteration when the closure resumed inside the
body: the character the previous iteration consumed enqueued the thread
mid-body, and that iteration's tail is already marked. So a walk that crosses
a backward edge whose head is unmarked continues in a fresh pass. `visited[pc]`
holds a key per pass, and a later pass may re-walk what an earlier one marked.
The pass count is bounded by how deeply the empty-capable repetitions nest,
which `mark_empty_loops()` computes once at compile time, and by
`RE_MAX_PASS`, so an epsilon loop still terminates and one `pc` still enqueues
a bounded number of threads. `RE_LIST_CAPA()` sizes the thread lists from that
same bound and is now shared with the cache the compiler preallocates for the
VM, so the two cannot drift.
`mark_empty_loops()` also flags each backward edge whose body can run empty,
and only a flagged edge takes any of this. A repetition that always consumes
keeps its single-pass walk, and a pattern with no empty-capable repetition is
unchanged in behaviour and in the memory it reserves.
Two cases stay divergent. Bounded repetitions are unrolled into copies with no
backward edge, so the rule has nothing to attach to:
```ruby
/(|a){2,3}b/.match("ab")[1] # CRuby: "", mruby: "a"
```
And CRuby keeps a loop running in a few shapes where an empty iteration
rewrote a capture, which this does not model:
```ruby
/(?:(?:a{1,3}|(c{0,2}){2,}){1,}|b{1,3})+/.match("ccab")[0]
# CRuby: "ccab", mruby: "cca"
```
Patterns with a non-greedy quantifier run on `bt_match()` and are untouched.
📝 WalkthroughWalkthroughThe regexp compiler now detects nullable repetition loops and records their nesting depth. The Pike VM uses bounded closure passes to stop empty iterations while preserving captures. Regression tests cover nested loops, offsets, ChangesNullable repetition handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RegexpCompiler
participant CompiledPattern
participant PikeVM
participant StringMethods
RegexpCompiler->>CompiledPattern: store nullable-loop depth
CompiledPattern->>PikeVM: provide loop depth and cache capacity
PikeVM->>PikeVM: expand closure with pass keys
PikeVM->>PikeVM: stop empty iteration or advance closure pass
PikeVM->>StringMethods: return preserved captures
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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
🧹 Nitpick comments (1)
mrbgems/mruby-regexp/src/re_compile.c (1)
1448-1497: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffConsider bounding the epsilon walk for very large patterns.
epsilon_pathrecurses once perRE_SPLIT/RE_SPLITNGon the path, with no depth limit. A deeply nested nullable pattern can therefore drive C stack depth proportional to the program length at compile time. The neighbouringcompute_first_setguards its recursivefirst_set_walkwithcode_len >= 4096, so this analysis is the only unguarded recursive walk in the file.
mark_empty_loopsalso callsepsilon_pathonce per backward edge, so compile cost is O(edges × code_len) for pattern shapes such as(a*)*(b*)*.... If patterns can come from untrusted input, add a size guard that returns 0 (keeping the previous behaviour) or convert the walk to an explicit stack.🛡️ Example size guard
static uint8_t mark_empty_loops(mrb_state *mrb, re_inst *code, uint32_t code_len) { + /* Very large programs: skip the analysis and keep the pre-existing + behaviour rather than risking deep recursion and quadratic work. */ + if (code_len >= 4096) return 0; int32_t *delta = (int32_t*)mrb_calloc(mrb, code_len + 1, sizeof(int32_t));🤖 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 `@mrbgems/mruby-regexp/src/re_compile.c` around lines 1448 - 1497, Bound the recursive epsilon analysis used by mark_empty_loops for large compiled patterns, matching the existing safety threshold used by first_set_walk in compute_first_set. For code_len at or above that threshold, return 0 before allocating or walking, preserving the prior no-loop-analysis behavior and preventing unbounded epsilon_path recursion and repeated compile-time traversal.
🤖 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 `@mrbgems/mruby-regexp/src/re_exec.c`:
- Around line 399-402: Prevent uint32_t generation-counter wraparound in pike_vm
by guarding both sites that advance s.gen and s.key_max: the seeding update near
add_thread and the per-input-step update. When the next increment would overflow
or collide with RE_LOOP_STOP, re-seed the generation counters and reset the
related visited state so add_thread can process PCs normally after rollover.
---
Nitpick comments:
In `@mrbgems/mruby-regexp/src/re_compile.c`:
- Around line 1448-1497: Bound the recursive epsilon analysis used by
mark_empty_loops for large compiled patterns, matching the existing safety
threshold used by first_set_walk in compute_first_set. For code_len at or above
that threshold, return 0 before allocating or walking, preserving the prior
no-loop-analysis behavior and preventing unbounded epsilon_path recursion and
repeated compile-time traversal.
🪄 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: 72910984-e3a1-4ca5-9ff8-1f415e31ecd3
📒 Files selected for processing (4)
mrbgems/mruby-regexp/include/re_internal.hmrbgems/mruby-regexp/src/re_compile.cmrbgems/mruby-regexp/src/re_exec.cmrbgems/mruby-regexp/test/regexp.rb
A repetition whose body can match empty runs one final iteration that consumes
nothing. That iteration ends the loop, and the group inside it keeps the empty
string it captured at the position the loop stopped at.
pike_vm()stops oneiteration earlier, so the group reports the last iteration that consumed
something, or nothing at all when there was none. A body that prefers its
empty branch does not end the loop either, which moves the whole match.
Why it happens
e*compiles to aRE_SPLIThead, the body, and a backwardRE_JMPto thathead;
e+closes with a backward fork instead.add_thread()walks eachpconce per step and returns on a revisit, which is what makes an epsilon loop
terminate. The empty iteration is walked and records its
RE_SAVEs, then hitsa
pcthe same walk already marked, so the slot holding those captures dies.The thread that leaves the loop was queued by the head before the body ran.
The change
body just ran empty: leave the loop from there with that iteration's
captures. The head explores the body before the exit, so this path claims
the exit
pcfirst and outranks the stale exit the head queued.tail, so the empty iteration would die before finishing. Crossing a backward
edge whose head is unmarked therefore continues in a fresh pass:
visited[]keys per pass, and a later pass may re-walk what an earlier one marked.
mark_empty_loops()runs once at compile time. It flags the backward edgeswhose body can run empty, and returns how deeply those loops nest, which
caps the passes (together with
RE_MAX_PASS) and sizes the thread lists viaRE_LIST_CAPA(), now shared with the cache the compiler preallocates.Only a flagged edge takes any of this, so a repetition that always consumes
keeps its single-pass walk, and a pattern with no empty-capable repetition is
unchanged in behaviour and in reserved memory.
Verification
rake testpasses; four assertions are added tomrbgems/mruby-regexp/test/regexp.rb.Two differential sweeps against CRuby 4.0.6, comparing
[0], every capture,and every
begin:(atom)(quantifier)(tail)over 18 atoms, 13 quantifiers, 5 tails, 11 subjectsOf the 148 remaining in the first sweep, 140 have a non-greedy body and run on
bt_match(), which this does not touch; the other 8 are the(|a){n,m}casebelow.
Timings, best of seven alternating runs of each binary:
(a?)*(\w*)*x[a-z]+\d+,(\w+)\s+(\w+),(quick|slow|lazy)\s+(\w+),^\w+.*dogThe cost falls on the repetitions whose semantics changed.
Known remaining divergences
Bounded repetitions are unrolled into copies with no backward edge, so the
rule has nothing to attach to:
CRuby also keeps a loop running in a few shapes where an empty iteration
rewrote a capture, which this does not model:
Four rows of the 56000-row random corpus regress this way; the same corpus
loses 1796 divergences overall.
Summary by CodeRabbit
Bug Fixes
String#split, andString#scan.Tests