mruby-regexp: refuse a broken subject at the entry to split - #7133
Merged
Conversation
The two commits before this refuse a search whose subject holds a byte that
spells no character, and exempt the literal a quoted String pattern is searched
for. `split` is what they left: CRuby refuses it whatever the pattern, and here
a String or nil pattern answered.
```ruby
"あ\x80b".split("b") # CRuby: ArgumentError, before this: ["あ\x80"]
"あ\x80b".split # CRuby: ArgumentError, before this: ["あ\x80b"]
"あ\x80b".split(" ") # CRuby: ArgumentError, before this: ["あ\x80b"]
```
`String#split` hands those three to core's `split`, which this gem keeps under
`__split`, so the path reaches no search of the gem's and the check every other
call runs never fires. The refusal goes at the entry instead, through a
`Regexp.__check_encoding` class method that is `re_check_encoding()` under a
name mrblib can call.
That name was in mruby#7110 for a different reason: to run the check once at the
entry to each mrblib loop rather than once per match. It went away when the
flag core leaves on a string it has read made the two placements measure the
same. What brings it back is a path no search covers, not what a search costs.
A limit of 1 hands the subject back whole without reading it, whatever the
pattern, and CRuby answers there too, so the check waits behind it. The limit
is converted before either way: `"あ\x80b".split("b", "x")` raises `TypeError`
in both.
A binary subject goes through as it did, since `mrb_str_valid_encoding_p()`
takes a byte-indexed string as valid whatever its bytes are, and a build
without `MRB_UTF8_STRING` reads no encoding for them to break.
|
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)
📝 WalkthroughWalkthrough
ChangesUTF-8 split validation
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to This localized change makes split reject invalid UTF-8 subjects consistently with CRuby, with no actionable merge-blocking risk remaining after normal checks and review. Possibly related PRs
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 |
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.
Builds on #7126 and #7127, which refuse a search whose subject holds a byte that spells no character and exempt the literal a quoted String pattern is searched for.
splitis what they left: CRuby refuses it whatever the pattern, and here a String or nil pattern answers.String#splithands those three to core'ssplit, which this gem keeps under__split, so the path reaches no search of the gem's and the check every other call runs never fires. The refusal goes at the entry instead, through aRegexp.__check_encodingclass method that isre_check_encoding()under a name mrblib can call.On the name
#7110 carried a class method of this name to run the check once at the entry to each mrblib loop rather than once per match. It went away when the flag core leaves on a string it has read made the two placements measure the same. What brings it back is a path no search covers, not what a search costs.
What stays as it is
byteindexwith a String pattern, andgsubwith a block over one, answer a broken subject in CRuby too, so they keep the exemption #7127 gives them. Their Regexp forms already refuse.A limit of 1 hands the subject back whole without reading it, whatever the pattern, and CRuby answers there too, so the check waits behind it. The limit is converted before either way:
"あ\x80b".split("b", "x")raisesTypeErrorin both.A binary subject goes through as it did, since
mrb_str_valid_encoding_p()takes a byte-indexed string as valid whatever its bytes are.Checked
rake teston the default gembox: 2040 assertions, 0 failures; bintest 105, 0 failuresrake teston a full-core build (MRB_UTF8_STRING): 2259 assertions, 0 failuresrake teston a build withoutMRB_UTF8_STRING, holding mruby-regexp and mruby-string-ext: 1037 assertions, 0 failuressplitform above compared against CRuby 4.0.6, which agrees with all of themSummary by CodeRabbit
String#splithandling for invalid UTF-8 strings.ArgumentErrorwhen applicable, whilelimit == 1preserves the original string.TypeErrorconsistently.