Stop a broken string from being read one byte at a time - #7131
Merged
Conversation
`String#length` counts such a byte as a character of its own, and the tests that ask it do so of strings holding nothing else. What the same byte counts as beside whole characters, and what index it takes, is asked nowhere, and both are answers the character count decides. So ask them: a broken sequence next to ASCII and next to a character of several bytes, and `String#[]` reading a surrogate's bytes back one at a time. `String#chars` already pins the split itself, and these are the count and the index the split is read through.
…aracter `inspect` walks the string it prints, and what it meets on the way settles MRB_STR_SINGLE_BYTE on the receiver: the flag stays set unless a character of several bytes is copied across whole. A byte that spells no character is copied one at a time, like an ASCII one, so a string carrying one came out of `inspect` flagged as holding one character per byte. That flag is read as leave for stepping through the bytes without decoding them. `String#codepoints` and `String#ord` then hand the bytes back as the characters the string does not hold, and `String#scrub` finds nothing to replace, so what a broken string answers turns on whether `inspect` was called on it first: ```ruby s = "\xED\xA0\x80" s.codepoints # ArgumentError s.inspect s.codepoints #=> [237, 160, 128] s.scrub.bytes #=> [237, 160, 128], where it was [239, 191, 189] * 3 ``` CRuby answers those the same way whatever ran before. Ask the walk about the byte instead of about the character it failed to begin: a non-ASCII byte leaves the string more bytes than characters whether or not it spells one. What `inspect` returns is unchanged, and the flag it puts on that return is too. `\xNN` is what a byte spelling no character is escaped to, so the string that comes back is still one character per byte, which only a whole character copied across takes from it. The two answers needed separating to say so.
…cter Counting characters sets MRB_STR_SINGLE_BYTE when the count comes out equal to the byte count, and the flag is read as leave for stepping through the bytes without decoding them. A byte that spells no character is counted as one, so a string carrying nothing else counts one per byte and set the flag too. Its bytes then came back as the characters it does not hold: ```ruby s = "\xED\xA0\x80" s.codepoints # ArgumentError s.length #=> 3 s.codepoints #=> [237, 160, 128] s.ord #=> 237, where it raised s.scrub.bytes #=> [237, 160, 128], where it was [239, 191, 189] * 3 ``` CRuby answers those the same way whatever ran before. Every character a non-ASCII byte begins spells two bytes or more, and a non-ASCII byte that begins none spells no character at all, so a string holds one character per byte exactly when every byte of it is ASCII. Ask that instead: the count already looks for the first non-ASCII byte before it counts anything, so where there is none the answer is the byte count and the flag holds, and where there is one the count carries on from it. `String#length` still counts a byte spelling no character as a character, and `String#chars` and `String#[]` still give it a position of its own. Only what is remembered about the string changes, and the walk those two take without the flag reads such a byte as one character anyway. The test before this one asks for that. `RSTR_SET_ASCII_FLAG` is already a second name for setting this flag, and `String#ascii_only?` sets it exactly where every byte is ASCII, so this is the meaning the flag's readers were written against. The comment in `mrb_str_valid_encoding_p` argued from the wider reading and goes.
|
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 (4)
📝 WalkthroughWalkthroughUTF-8 character-length detection now preserves malformed non-ASCII state. String escaping tracks source and output flags separately. Tests cover inspection, indexing, sizing, character iteration, codepoints, ordinals, and scrubbing. ChangesMalformed UTF-8 handling
Estimated code review effort: 3 (Moderate) | ~20 minutes 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 |
takumin
pushed a commit
to takumin/mruby
that referenced
this pull request
Aug 12, 2026
A search under mruby-regexp refuses a subject whose bytes are not UTF-8 (mruby#7126), and what an unwalked subject pays there is one walk. The walk's answer is remembered on the string it read, and mruby#7131 lets a string already known ASCII answer without one, but the operations a subject is usually built with still dropped what their sources knew: `+` copied no flags at all, `<<` and interpolation took them down on every append, and a piece cut out of a walked string kept only the single-byte mark. So the subject of `(mb + "z") =~ /z/` walked 300001 bytes per search, 300000 of which a walk had already read. Validity survives every one of those builds, each for a reason the build can afford to check: - Concatenation: bytes that read as UTF-8 followed by bytes that do read as UTF-8 end to end, each side's sequences being complete on their own. `+`, `<<`, `concat` and interpolation hand the result the flag when both sides carry it, and two ASCII sides make an ASCII whole, which carries MRB_STR_SINGLE_BYTE the same way. An empty side carries both by holding nothing. - Repetition: `str * n` holds nothing but the source's bytes over and over, so the source's answer is the result's. - A byte range: a range of a valid string reads whole exactly when both cuts land on character boundaries, and in a valid string every byte that is not a continuation byte begins a character. So the byte each cut lands on decides it--the one at `beg`, and the one just past the range, with the end of the string standing for a boundary--in two byte tests, no walk. This is what hands a searched subject's flag down to the pieces `split`, `scan` and `byteslice` cut from it. - Measuring: past the ASCII run the count now returns early on, it decodes every sequence anyway, so the same walk also says whether each spelled a character, and leaves that answer where the next search finds it. A stray byte still counts as one character, which is the count this has always returned. What would defeat the hand-off is the short literal nothing has walked: the "z" above arrives from the pool flagless on every evaluation, and one flagless side spoils the pair. So a side of at most 16 bytes is walked on the spot--all-ASCII answers in a word test--bounded by the length check and below the cost of the copy its caller is doing anyway. That is what lets `mb + "z"`, `"#{mb}z"` and the `"あ" * 100000` that built `mb` come out flagged. The answer is not written back to the walked side: it is most often a literal this evaluation made and the next one remakes, so there is no later read to save a walk for, and re-walking a reused one costs these same few bytes. The flag never claims more than a walk of the same bytes would find, so no answer changes. The test that puts the same bytes through the same operations warm and cold now runs the new builders too, over broken bases as well as whole ones, and a new block pins the boundary-byte cases where a cut must not inherit. At `-O3` on a full-core build, the fastest of seven runs interleaved between builds, with `mb = "あ" * 100000`: | | master | with this | |---|---|---| | `(mb + "z") =~ /z/`, 200 times, less the builds | 178.2 ms | 96.1 ms | | `"#{mb}z" =~ /z/`, the same | 195.3 ms | 111.9 ms | | `"hello world" =~ /world/`, 100000 times | 235.0 ms | 242.0 ms | | `s << "abcdefgh"`, 100000 times, never searched | 15.1 ms | 16.1 ms | The first two land where they stood before mruby#7126 made subjects checkable at all (96.3 ms and 113.7 ms on the same machine): nothing walks but the "z". Pieces split off a walked subject stop paying their own first walk the same way. The last row is the cost's worst case, a loop of nothing but eight-byte appends to a buffer that stays known-valid, where the flag reads are around a third of the growth and the bounded walk the rest; it moved six to twelve percent across measurement rounds here. A loop appending sides longer than the bound does not walk them, and one whose buffer was never known valid drops to the flag tests alone. `split`, `gsub`, `rindex`, the third row and String#length move a few percent either way between rounds, with no changed code on their paths--the jitter code layout pays for this file growing. A literal born flagged would close the walk of the "z" too, but the pool entry has no bit left to carry the answer in (IREP_TT_STR spends them all), and stamping it at OP_STRING would only move the same walk there. That is a dump-format question for another day. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T6X4dd382CCZmfrXSpFsb8
This was referenced Aug 14, 2026
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
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.
The flag
MRB_STR_SINGLE_BYTEsays a string holds one character per byte, and itsreaders take it as leave to step through the bytes without decoding them.
RSTR_SET_ASCII_FLAGis a second name for setting it, andString#ascii_only?sets it exactly where every byte is ASCII.
Two places set it from a weaker fact.
mrb_str_char_len()sets it when thecharacter count comes out equal to the byte count, and
str_escape()sets it onthe receiver of
inspectwhen the walk copied no character of several bytesacross. A byte that spells no character is counted as one, and is escaped one at
a time, so a string carrying nothing else satisfies both.
What that cost
inspectleaves the string the same way, and the flag survivesdup,replace,String#*,freezeand a byte slice, so the answer travels. CRubyanswers all of these the same way whatever ran before.
String#scrubis the one that matters most: a string of broken bytes is exactlywhat it exists to repair, and it handed the string back untouched wherever
anything had counted it first.
The change
Every character a non-ASCII byte begins spells two bytes or more, and a
non-ASCII byte that begins none spells no character at all, so a string holds
one character per byte exactly when every byte of it is ASCII. Both places now
ask that.
mrb_str_char_len()already looks for the first non-ASCII byte before it countsanything, so where there is none the answer is the byte count and the flag
holds, and where there is one the count carries on from it.
str_escape()clears the receiver's flag as soon as it meets a non-ASCII byte. The flag it
puts on what
inspectreturns keeps its old rule, since\xNNis what a bytespelling no character is escaped to and the return is still one character per
byte; only a whole character copied across takes that from it. The two answers
needed separating to say so.
What does not change
String#length,#size,#chars,#[],#inspectand#dumpanswer exactlyas before. A byte that spells no character is still counted as a character and
still takes an index of its own. The walks that
#charsand#[]fall back towithout the flag read such a byte as one character anyway, which is what the
first commit pins.
The cost
A string that is not all ASCII is counted every time it is asked. Broken strings
used to be counted once and remembered; they now join the class every valid
multi-byte string was already in. Counting a 100 KB string 2000 times:
The ASCII path is one
search_nonascii()sweep either way, and the flag stillholds after the first count.
What is left
int_chr_binary()inmruby-string-extsets the flag on a one-byte stringwhose byte may be
0x80or above, and sets noMRB_STR_BINARYbeside it. Underthe narrower meaning that is a false statement, though what it produces is what
CRuby produces:
Setting
MRB_STR_BINARYthere instead would close it, and would settle thesecond line as well. That is a separate change and is not in this PR.
With it closed,
mrb_str_valid_encoding_p()could answertruefrom the flagalone and feed the
MRB_STR_VALID_ENCcache. This PR only removes the commentthat said the flag could not be read there.
Tests
The first commit pins what a byte spelling no character counts and indexes as,
before anything moves. Each of the other two carries the cases its own change
decides, and taking either change back out fails that commit's assertions and no
others.
rake testis green on three builds:full-corewithMRB_UTF8_STRING, thedefaultgembox without it, andfull-corewithMRB_UTF8_STRINGandMRB_INT32. Each commit was checked on its own. A sweep of nine broken stringsagainst twenty-four ways of touching one first found 91 places where the bytes
came back as characters, and none after.
Summary by CodeRabbit
Bug Fixes
ord,codepoints, andscrubnow produce the expected results for malformed strings while maintaining normal ASCII behavior.Tests