Skip to content

mruby-regexp: exempt a quoted String pattern from the subject check - #7127

Closed
takumin wants to merge 2 commits into
mruby:masterfrom
takumin:regexp-exempt-literal-pattern
Closed

mruby-regexp: exempt a quoted String pattern from the subject check#7127
takumin wants to merge 2 commits into
mruby:masterfrom
takumin:regexp-exempt-literal-pattern

Conversation

@takumin

@takumin takumin commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

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.

"あ\x80b".sub("b", "!")    # CRuby: "あ\x80!", after #7126: 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. This takes note of it before the quoting and carries 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.

What stays as it is

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 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_encoding class method, which #7126 does without.

The first commit here is #7126, so this shows two until that one is merged.

Checked

  • rake test on the default gembox: 2063 assertions, 0 failures; bintest 105, 0 failures
  • rake test on a full-core build (MRB_UTF8_STRING): 2257 assertions, 0 failures
  • 14 calls with a String pattern over a broken subject compared against CRuby 4.0.6: every one agrees except split, which is the divergence named above

Summary by CodeRabbit

  • Bug Fixes

    • Regexp matching and substitution now consistently reject invalid UTF-8 text with ArgumentError.
    • Literal string patterns preserve byte-oriented behavior, including for substitution and global substitution.
    • Binary strings continue to support byte-based matching without UTF-8 validation.
    • Matching behavior is now consistent across search, scanning, predicates, and replacement operations.
  • Tests

    • Expanded coverage for malformed UTF-8, encoding-specific behavior, and literal string patterns.

@takumin
takumin requested a review from matz as a code owner August 12, 2026 15:14
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0fe871fd-d901-4489-b5ee-c94bfa6e56fd

📥 Commits

Reviewing files that changed from the base of the PR and between 456a708 and 434fc8f.

📒 Files selected for processing (1)
  • mrbgems/mruby-regexp/test/regexp_syntax.rb

📝 Walkthrough

Walkthrough

Regexp APIs now reject invalid UTF-8 subjects with ArgumentError, except for binary strings. String substitution methods preserve literal-pattern handling and propagate encoding-check state. Tests cover matching, substitution, scanning, mutation, offsets, and build modes.

Changes

Regexp UTF-8 validation and literal handling

Layer / File(s) Summary
Encoding validation and search entry points
mrbgems/mruby-regexp/src/regexp.c
Regexp matching, searching, scanning, and predicate paths validate non-binary subjects. Internal helpers accept a checked flag.
Literal-aware substitution wiring
mrbgems/mruby-regexp/mrblib/string_regexp.rb, mrbgems/mruby-regexp/src/regexp.c
sub, sub!, gsub, and gsub! preserve literal strings and pass checked state to search and replacement helpers.
Malformed UTF-8 regression coverage
mrbgems/mruby-regexp/test/regexp_utf8.rb, mrbgems/mruby-regexp/test/regexp_syntax.rb
Tests verify encoding-specific errors, byte-oriented handling, offsets, literal patterns, and lookbehind behavior.

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
Loading

Possibly related PRs

  • mruby/mruby#7119: Modifies the same regexp search, substitution, and UTF-8 validation APIs.
  • mruby/mruby#7115: Covers byte-indexed and literal String regexp behavior.
  • mruby/mruby#7061: Modifies the same String#sub! and String#gsub! delegation paths.

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: exempting quoted String patterns from subject encoding checks.
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.

@takumin
takumin marked this pull request as draft August 12, 2026 16:14
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.
@takumin
takumin force-pushed the regexp-exempt-literal-pattern branch from 456a708 to 434fc8f Compare August 12, 2026 16:33
@takumin
takumin marked this pull request as ready for review August 12, 2026 16:41
matz added a commit that referenced this pull request Aug 12, 2026
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.
@matz

matz commented Aug 12, 2026

Copy link
Copy Markdown
Member

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 re_compile.c with git checkout <commit> -- <path>, which stages as well as writes the file. Restoring the working tree with cp left the old version in the index, and the next commit took it. That put #7125's compute_fixed_len change back to counting lead bytes, and it was pushed. Restored in 1e6a2e6, with the history left alone.

What I checked before merging, on top of the verification in your description:

On the piece still to come: split, byteindex and gsub with a block are the three that still answer, and hoisting the check to the entry of each is the right shape for them. No hurry from me.

@takumin

takumin commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

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

"あ\x80b".split("b")        # CRuby: ArgumentError, here: ["あ\x80"]
"あ\x80b".byteindex("b")    # CRuby: 4, here: 4
"あ\x80b".gsub("b") { "!" } # CRuby: "あ\x80!", here: "あ\x80!"

Only split parts company, as it did in the 14 calls measured for #7127. byteindex with a String, and gsub with a block over a String pattern, are the literal case that PR exempts, and this gem answers them the way CRuby does, so a check at their entry would carry them away from CRuby rather than toward it. Their Regexp forms already refuse: byteindex(/b/) and gsub(/b/) { } reach __byte_search with no checked.

That leaves split, which reaches no Regexp when the pattern is a String or nil: String#split hands both to the C __split, and the awk form goes there too, so all three answer where CRuby raises.

"あ\x80b".split      # CRuby: ArgumentError, here: ["あ\x80b"]
"あ\x80b".split(" ") # CRuby: ArgumentError, here: ["あ\x80b"]

The check goes at the entry to __split, the one place all three pass through. I will send it on its own.

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.

3 participants