mruby-regexp: exempt a quoted String pattern from the subject check - #7127
mruby-regexp: exempt a quoted String pattern from the subject check#7127takumin 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: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRegexp APIs now reject invalid UTF-8 subjects with ChangesRegexp UTF-8 validation and literal handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant StringAPI
participant RegexpAPI
participant EncodingCheck
participant RegexpEngine
StringAPI->>RegexpAPI: submit subject and pattern
RegexpAPI->>EncodingCheck: validate non-binary UTF-8
EncodingCheck-->>RegexpAPI: valid subject or ArgumentError
RegexpAPI->>RegexpEngine: perform match or substitution
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 |
CRuby raises `ArgumentError` when a search is given a subject holding a byte
that spells no character. mruby answered for it instead, so the same program
took a result CRuby would not have produced:
```ruby
"あ\x80b" =~ /b/ # CRuby: ArgumentError, mruby: 2
"あ\x80b".scan(/./) # CRuby: ArgumentError, mruby: ["あ", "\x80", "b"]
"\xC0\xBC" =~ /[^<]/ # CRuby: ArgumentError, mruby: 0
```
Follow CRuby wherever a search reads the subject: `=~`, `match`, `match?`,
`===`, `index`, `rindex`, `byteindex`, `byterindex`, `[]`, `[]=`, `sub`, `sub!`,
`gsub`, `gsub!`, `scan`, `split`, `partition`, `rpartition`, `start_with?` and
`slice!`. `String#scrub` is how a subject like this becomes matchable.
A binary string goes through untouched, since it is indexed by byte from end to
end and its bytes make no claim that could be broken. A string derived from a
binary one does not carry `MRB_STR_BINARY` with it, so `b + ""` and `b.sub(...)`
are refused where `b` itself is not; that gap predates this change and is left
as it is.
A quoted String pattern is refused here as well, where CRuby answers for it.
CRuby searches for a literal byte by byte and reads the subject as UTF-8 nowhere
along the way, so
```ruby
"あ\x80b".sub("b", "!") # CRuby: "あ\x80!", here: ArgumentError
```
Exempting it means carrying the fact that the pattern was a literal from `sub`
down to the search it makes, which `sub!` and `gsub!` pay for by quoting the
pattern twice, so that is a change of its own and comes next. `scan` needs none
of it: CRuby refuses a literal there too.
The check goes in each search rather than at the entry to the methods that drive
one, because core remembers a string it has read as valid UTF-8. Most of the
entry points here search once for what arrives from Ruby, but `__byte_search`,
which the mrblib loops of `gsub`, `split` and `byteindex` drive, is called once
per match, and `__regexp_rsearch` steps `__search` over every match position.
Those turns read the flag core left on the subject after the first one, so what
they cost is a flag test and not a walk. The flag `String#length` leaves behind
cannot stand in for it, since a string of stray bytes has one byte per character
too, which is why only the walk decides.
What a subject nothing has walked yet pays is one walk. It skips a run of ASCII
a word at a time and decodes only where a byte leaves that range, so the cost
follows how much of the subject is multi-byte. At `-O3` on a full-core build,
the fastest of fifteen runs each, with `mb` for `"あ" * 100000` and `cs` for
`"あ," * 20000`:
| | before | after |
|---|---|---|
| `(mb + "z") =~ /z/`, 200 times, less the cost of building the subjects | 52.1 ms | 103.8 ms |
| the same with an ASCII subject of the same byte size | 1.2 ms | 0.7 ms |
| `"hello world" =~ /world/`, 100000 times | 133.6 ms | 133.0 ms |
| `cs.split(/,/)`, one walk and 20000 searches | 71.1 ms | 61.4 ms |
| `cs.gsub(/,/) { ";" }`, the same | 75.1 ms | 72.9 ms |
| `cs.rindex(/,/)`, one walk and 20000 match positions | 2105.9 ms | 2114.6 ms |
The first row builds a subject for every search so that every search walks one.
The rest reuse a subject, which is why the loop rows show what the flag costs
rather than what the walk does, and they and the ASCII row are inside the noise
of the machine they were taken on.
A `pos` outside the subject still answers nil rather than raising, since
`re_char_to_byte()` rejects it before the check runs. `String#index` answers nil
for one in CRuby too; `Regexp#match` raises there, where this answers nil.
`rindex` clamps a position past the end to the end rather than rejecting it, so
it reaches the check and refuses, which is what CRuby answers as well.
Most of the tests that ask what the engine does with a byte standing for no
character already put the question to a byte-indexed subject, so this refuses
none of them. What is added is the list of methods that refuse. The two blocks
that ask it of a subject read as UTF-8 keep both answers and branch on
`__ENCODING__`, since a build without `MRB_UTF8_STRING` reads no encoding for
the bytes to break. One turned on what mruby knows about the string and now
asks that the refusal not turn on it. The other measures a lookbehind whose
bytes spell no character, and a UTF-8 build refuses such a subject before the
rewind runs, so only the build that reads no encoding still reaches the
measurement; the byte rewind it also asserts is reached in both.
The commit before this refuses a search whose subject holds a byte that spells
no character. It refuses one made with a String pattern as well, where CRuby
answers: a literal is searched for byte by byte and the subject is read as UTF-8
nowhere along the way.
```ruby
"あ\x80b".sub("b", "!") # CRuby: "あ\x80!", before this: ArgumentError
"あ\x80b".scan("b") # ArgumentError in both
```
`sub`, `sub!`, `gsub` and `gsub!` quote a String pattern into a `Regexp` before
they search, which hides what it was. Take note of it before the quoting and
carry the note down to the search: `Regexp.__search`, `__byte_search`,
`__sub_str` and `__gsub_str` each take a `checked` argument saying the caller
has settled the encoding question and the search must not ask it again.
`sub!` and `gsub!` search once themselves and then call `sub` and `gsub`, which
search again. The resolved pattern used to go down to that second call so that a
String was not quoted twice; a literal goes down as the String it was instead,
since that is what tells `sub` to leave the subject unread. Quoting it a second
time is the price of saying so.
`scan` is not exempt: CRuby refuses a literal there, so it keeps the check.
`index`, `partition`, `start_with?`, `slice!` and `[]` search a String pattern
in core without reaching a Regexp at all, so they already answer what CRuby
answers. So does `split`, which is where the two part company:
`"あ\x80b".split("b")` answers here and raises in CRuby. That divergence
predates both commits and is left where it was.
The tests are what CRuby answers for the six calls this exempts, the position
`sub` leaves in `$~`, and `scan` refusing a literal.
456a708 to
434fc8f
Compare
mruby-regexp: exempt a quoted String pattern from the subject check Carries #7126 as well; the lookbehind assertions the subject check makes unaskable were moved to a byte-indexed subject in the commit before this.
|
Merged, and thank you for the split. The commits are in master as 80c781b and 456a708; I took them locally rather than through the button, so these stay open and I am closing them by hand. Two things happened on the way in that are worth recording. The lookbehind assertions. #7125 landed before these, and its subjects are the ones this refuses, so the block crashed once both were in. I resolved it by dropping those assertions to the byte-indexed forms. You had solved the same problem better in the rebase: asserting the refusal on a build that reads an encoding, and keeping the widths on the one that does not. I have taken your version (c2b7b3a). Mine recorded that the question could no longer be asked; yours asks the new one. A mistake of mine, since it touched your work. While measuring whether the lookbehind fix was still reachable after the refusal, I reverted What I checked before merging, on top of the verification in your description:
On the piece still to come: |
|
Thank you for taking the rebase version, and for the restore. On the collision: #7126 has a section on it (What the lookbehind tests ask now) carrying the same resolution you arrived at, but #7127 is the page the two commits were shown on and it says nothing about the lookbehind. Read from the top of the stack there was nothing to find. I will repeat an interaction like that in the description of every PR in a stack rather than only in the one that introduces it. On the piece to come, I put the three to CRuby 4.0.6 and to a full-core build with "あ\x80b".split("b") # CRuby: ArgumentError, here: ["あ\x80"]
"あ\x80b".byteindex("b") # CRuby: 4, here: 4
"あ\x80b".gsub("b") { "!" } # CRuby: "あ\x80!", here: "あ\x80!"Only That leaves "あ\x80b".split # CRuby: ArgumentError, here: ["あ\x80b"]
"あ\x80b".split(" ") # CRuby: ArgumentError, here: ["あ\x80b"]The check goes at the entry to |
Builds on #7126, which refuses a search whose subject holds a byte that spells no character. That PR refuses one made with a String pattern as well, where CRuby answers: a literal is searched for byte by byte and the subject is read as UTF-8 nowhere along the way.
sub,sub!,gsubandgsub!quote a String pattern into aRegexpbefore they search, which hides what it was. This takes note of it before the quoting and carries the note down to the search:Regexp.__search,__byte_search,__sub_strand__gsub_streach take acheckedargument saying the caller has settled the encoding question and the search must not ask it again.sub!andgsub!search once themselves and then callsubandgsub, which search again. The resolved pattern used to go down to that second call so that a String was not quoted twice; a literal goes down as the String it was instead, since that is what tellssubto leave the subject unread. Quoting it a second time is the price of saying so.What stays as it is
scanis not exempt: CRuby refuses a literal there, so it keeps the check.index,partition,start_with?,slice!and[]search a String pattern in core without reaching a Regexp at all, so they already answer what CRuby answers. So doessplit, which is where the two part company:"あ\x80b".split("b")answers here and raises in CRuby. That divergence predates both PRs and is left where it was.This exemption was part of #7110, which is where it was argued from; it is split out here so that #7126 is the refusal alone. What #7110 carried and this does not is the
Regexp.__check_encodingclass method, which #7126 does without.The first commit here is #7126, so this shows two until that one is merged.
Checked
rake teston the default gembox: 2063 assertions, 0 failures; bintest 105, 0 failuresrake teston a full-core build (MRB_UTF8_STRING): 2257 assertions, 0 failuressplit, which is the divergence named aboveSummary by CodeRabbit
Bug Fixes
ArgumentError.Tests