mruby-regexp: type-check the sub, gsub, scan, split and =~ pattern - #7001
Merged
Conversation
…split`
`String#=~` rejects a String argument up front, because dispatching one to
the argument would come straight back to this method and recurse. The guard
asked the argument for its type through `is_a?`, which is redefinable, so a
String subclass denying that it is a String slipped past the guard and
reached the very dispatch the guard exists to prevent.
```ruby
class Denier < String
def is_a?(klass)
false
end
end
d = Denier.new("b")
d =~ d # CRuby: TypeError (type mismatch: String given)
# mruby: SystemStackError (stack level too deep)
```
`String#split` has the mirror image of the problem. It sends `nil` and
String patterns to the core implementation before its own type check is
reached, and asked the argument with `nil?` and `is_a?`, both redefinable,
so an object answering either one could route itself to `__split` and never
reach the check at all.
Both now read the real type through `Module#===`, which cannot be
redefined.
The accept path of `sub`, `gsub`, `scan` and `split` is pinned by a test
alongside, covering a Regexp subclass and a quoted String pattern, so that
the check itself can be reworked without the accepted cases moving with it.
`sub`, `gsub` and `scan` compiled a String pattern and passed everything else through untouched, so a pattern that is neither a Regexp nor a String reached an internal helper and failed there with a `NoMethodError` naming it. `split` did check its pattern, but built the message from the class in every case, so `true` and `false` were named `TrueClass` and `FalseClass`. All four now go through `Regexp.__match_pattern`, the check `match` and `match?` already use, which names `nil`, `true` and `false` by value and everything else by class. The check stays in C so the argument cannot pose as a Regexp through a redefined `is_a?` or `class`, and what to do with an accepted String stays in Ruby, as fd5e075 established: `match` compiles it, `sub`, `gsub` and `scan` quote it first. `split` needs no such line, having already sent `nil` and String patterns to the core implementation before the check is reached. The block form of `sub` returned a wrong string rather than raising, since it reached the pattern through `match`, and a Symbol answers that call with the operands reversed: the symbol becomes the subject and the receiver the pattern. ```ruby "ab".sub(:xaby) { "Z" } # CRuby: TypeError (wrong argument type Symbol (expected Regexp)) # mruby: "xZy" ``` Two orderings that already match CRuby are kept: `"abc".gsub(:b)` returns an Enumerator and raises on the first iteration, and `"abc".split(true, 1)` returns `["abc"]` without examining the pattern.
|
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 (3)
📝 WalkthroughWalkthroughRegexp-related String methods now use ChangesRegexp pattern validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
takumin
added a commit
to takumin/mruby
that referenced
this pull request
Aug 3, 2026
The limit conversion is guarded by `limit.is_a?(Integer)`, which asks the
argument for its own type. `is_a?` is redefinable, so an argument claiming
to be an Integer skips the conversion entirely and reaches the split loop
as itself.
Answering none of the operators the loop uses, the failure surfaces as a
`NoMethodError` naming an internal comparison, which says nothing about the
limit being wrong. Answering `==`, `>` and `-`, there is no error at all:
the loop runs to completion with a non-Integer limit and returns a
plausible wrong result.
```ruby
class Cmp
def is_a?(klass); true; end
def ==(other); false; end
def >(other); true; end
def -(other); 1; end
end
"a,b,c".split(/,/, Cmp.new) # CRuby: TypeError
# mruby: ["a", "b,c"]
```
`Module#===` reads the real type and cannot be redefined, the same
substitution mruby#7001 made for the pattern argument a few lines below.
The two halves of the method used to disagree about one argument. A String
pattern delegates to `__split`, which converts the limit again in C, where
the argument gets no say, so only the regexp path was reachable with an
unconverted limit. Both paths now raise the same `TypeError`.
8f85a84 dropped the `to_int` dispatch this guard used to stand in front
of, so the block behind it is now a single `__to_int` call. That call is
what a redefined `is_a?` still skips, so the guard remains the only thing
between such an argument and the loop.
takumin
added a commit
to takumin/mruby
that referenced
this pull request
Aug 3, 2026
The limit conversion is guarded by `limit.is_a?(Integer)`, which asks the
argument for its own type. `is_a?` is redefinable, so an argument claiming
to be an Integer skips the conversion entirely and reaches the split loop
as itself.
Answering none of the operators the loop uses, the failure surfaces as a
`NoMethodError` naming an internal comparison, which says nothing about the
limit being wrong. Answering `==`, `>` and `-`, there is no error at all:
the loop runs to completion with a non-Integer limit and returns a
plausible wrong result.
```ruby
class Cmp
def is_a?(klass); true; end
def ==(other); false; end
def >(other); true; end
def -(other); 1; end
end
"a,b,c".split(/,/, Cmp.new) # CRuby: TypeError
# mruby: ["a", "b,c"]
```
`Module#===` reads the real type and cannot be redefined, the same
substitution mruby#7001 made for the pattern argument a few lines below.
The two halves of the method used to disagree about one argument. A String
pattern delegates to `__split`, which converts the limit again in C, where
the argument gets no say, so only the regexp path was reachable with an
unconverted limit. Both paths now raise the same `TypeError`.
8f85a84 dropped the `to_int` dispatch this guard used to stand in front
of, so the block behind it is now a single `__to_int` call. That call is
what a redefined `is_a?` still skips, so the guard remains the only thing
between such an argument and the loop.
Related to mruby#7003.
takumin
added a commit
to takumin/mruby
that referenced
this pull request
Aug 3, 2026
The limit conversion is guarded by `limit.is_a?(Integer)`, which asks the
argument for its own type. `is_a?` is redefinable, so an argument claiming
to be an Integer skips the conversion entirely and reaches the split loop
as itself.
Answering none of the operators the loop uses, the failure surfaces as a
`NoMethodError` naming an internal comparison, which says nothing about the
limit being wrong. Answering `==`, `>` and `-`, there is no error at all:
the loop runs to completion with a non-Integer limit and returns a
plausible wrong result.
```ruby
class Cmp
def is_a?(klass); true; end
def ==(other); false; end
def >(other); true; end
def -(other); 1; end
end
"a,b,c".split(/,/, Cmp.new) # CRuby: TypeError
# mruby: ["a", "b,c"]
```
`Module#===` reads the real type and cannot be redefined, the same
substitution mruby#7001 made for the pattern argument a few lines below.
The two halves of the method used to disagree about one argument. A String
pattern delegates to `__split`, which converts the limit again in C, where
the argument gets no say, so only the regexp path was reachable with an
unconverted limit. Both paths now raise the same `TypeError`.
8f85a84 dropped the `to_int` dispatch this guard used to stand in front
of, so the block behind it is now a single `__to_int` call. That call is
what a redefined `is_a?` still skips, so the guard remains the only thing
between such an argument and the loop.
Related to mruby#7003.
This was referenced Aug 3, 2026
This was referenced Aug 10, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
String#sub,#gsuband#scancompiled a String pattern and passedeverything else through untouched, so a pattern that is neither a Regexp nor a
String reached an internal helper and failed there with a
NoMethodErrornaming it.
#splitdid check its pattern, but built the message from theclass in every case, so
trueandfalsewere namedTrueClassandFalseClass.The block form of
#subwas worse than a wrong message: it reached thepattern through
match, and a Symbol answers that call with the operandsreversed, so the symbol became the subject and the receiver the pattern.
The type checks that did exist read the argument through
is_a?andnil?,both redefinable.
String#=~rejects a String up front because dispatchingone to the argument would come straight back to this method and recurse, and a
String subclass denying that it is a String slipped past the guard into that
very recursion.
String#splithas the mirror image of the problem: it sendsniland Stringpatterns to the core implementation before its own check is reached, so an
object answering
nil?oris_a?could route itself to__splitand neverreach the check at all.
Changes
String#=~and#splitread the real type throughModule#===, whichcannot be redefined.
#sub,#gsub,#scanand#splitgo throughRegexp.__match_pattern,the check
matchandmatch?already use, which namesnil,trueandfalseby value and everything else by class.The check stays in C so the argument cannot pose as a Regexp through a
redefined
is_a?orclass, and what to do with an accepted String stays inRuby, as fd5e075 established:
matchcompiles it,sub,gsubandscanquote it first, and
splitneeds no such line, having already sentnilandString patterns to the core implementation before the check is reached.
Two orderings that already match CRuby are kept and pinned by a test:
"abc".gsub(:b)returns an Enumerator and raises on the first iteration, and"abc".split(true, 1)returns["abc"]without examining the pattern. Theaccept path is pinned as well, covering a Regexp subclass and a quoted String
pattern, so that the check can be reworked without the accepted cases moving
with it.
rake testpasses.Summary by CodeRabbit
gsubenumerators andsplitedge cases.