Skip to content

mruby-regexp: stop a repetition on its empty iteration - #7071

Merged
matz merged 1 commit into
mruby:masterfrom
takumin:regexp-empty-iteration
Aug 10, 2026
Merged

mruby-regexp: stop a repetition on its empty iteration#7071
matz merged 1 commit into
mruby:masterfrom
takumin:regexp-empty-iteration

Conversation

@takumin

@takumin takumin commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

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 one
iteration 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.

/(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"

"ab".split(/(a?)*/, -1)      # CRuby: ["", "", "b", "", ""],  mruby: ["", "a", "b", ""]
"ab".scan(/(a?)*/)           # CRuby: [[""], [""], [""]],     mruby: [["a"], [nil], [nil]]

Why it happens

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 and returns on a revisit, which is what makes an epsilon loop
terminate. The empty iteration is walked and records its RE_SAVEs, then hits
a pc the 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

  • At a backward edge whose loop head is already marked at this position, the
    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 pc first and outranks the stale exit the head queued.
  • A closure that resumed inside the body has already marked that iteration's
    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 edges
    whose 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 via
    RE_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 test passes; four assertions are added to
mrbgems/mruby-regexp/test/regexp.rb.

Two differential sweeps against CRuby 4.0.6, comparing [0], every capture,
and every begin:

corpus rows before after
(atom)(quantifier)(tail) over 18 atoms, 13 quantifiers, 5 tails, 11 subjects 12870 1674 differ 148 differ
randomly generated patterns, two seeds 56000 each 1812 / 1765 differ 16 / 20 differ

Of 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} case
below.

Timings, best of seven alternating runs of each binary:

pattern before after
(a?)* 59.7 ms 80.8 ms
(\w*)*x 3.1 ms 3.7 ms
[a-z]+\d+, (\w+)\s+(\w+), (quick|slow|lazy)\s+(\w+), ^\w+.*dog within noise

The 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:

/(|a){2,3}b/.match("ab")[1]  # CRuby: "",  mruby: "a"

CRuby also keeps a loop running in a few shapes where an empty iteration
rewrote a capture, which this does not model:

/(?:(?:a{1,3}|(c{0,2}){2,}){1,}|b{1,3})+/.match("ccab")[0]
# CRuby: "ccab",  mruby: "cca"

Four rows of the 56000-row random corpus regress this way; the same corpus
loses 1796 divergences overall.

Summary by CodeRabbit

  • Bug Fixes

    • Improved regular expression handling for repetitions that can match empty text.
    • Prevented infinite or excessive repetition while preserving captures and match offsets.
    • Improved behavior for nested repetitions, String#split, and String#scan.
    • Kept consuming-only repetition behavior unchanged.
  • Tests

    • Added regression coverage for empty-iteration captures and nested nullable repetitions.

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.
@takumin
takumin requested a review from matz as a code owner August 10, 2026 09:10
@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The 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, String#split, and String#scan.

Changes

Nullable repetition handling

Layer / File(s) Summary
Compile-time empty-loop analysis
mrbgems/mruby-regexp/include/re_internal.h, mrbgems/mruby-regexp/src/re_compile.c
The compiler marks backward paths that can match empty, records maximum loop nesting, and sizes Pike VM caches from that depth.
Pike VM pass control and regression coverage
mrbgems/mruby-regexp/src/re_exec.c, mrbgems/mruby-regexp/test/regexp.rb
The VM tracks closure-pass keys, terminates empty iterations, preserves captures, and validates nested nullable repetition and string-method behavior.

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
Loading

Suggested labels: mrbgems

Suggested reviewers: matz

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: stopping repetitions on an empty iteration.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
mrbgems/mruby-regexp/src/re_compile.c (1)

1448-1497: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoff

Consider bounding the epsilon walk for very large patterns.

epsilon_path recurses once per RE_SPLIT/RE_SPLITNG on 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 neighbouring compute_first_set guards its recursive first_set_walk with code_len >= 4096, so this analysis is the only unguarded recursive walk in the file.

mark_empty_loops also calls epsilon_path once 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

📥 Commits

Reviewing files that changed from the base of the PR and between 7b2d9f4 and 45c588a.

📒 Files selected for processing (4)
  • mrbgems/mruby-regexp/include/re_internal.h
  • mrbgems/mruby-regexp/src/re_compile.c
  • mrbgems/mruby-regexp/src/re_exec.c
  • mrbgems/mruby-regexp/test/regexp.rb

Comment thread mrbgems/mruby-regexp/src/re_exec.c
@matz
matz merged commit 3cc8975 into mruby:master Aug 10, 2026
21 checks passed
@takumin
takumin deleted the regexp-empty-iteration branch August 10, 2026 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants