Check the Unicode range in mrb_utf8_to_buf instead of in its four callers - #7129
Conversation
`mrb_utf8_to_buf()` encodes every codepoint through U+10FFFF, so a surrogate gets the three bytes its number spells, while `mrb_utf8len()` reads those bytes back as no character at all (RFC 3629). The asymmetry is deliberate: CRuby writes the same three bytes at both of these call sites and refuses the value in `Integer#chr` instead, which `mruby-string-ext` already pins. Nothing pinned the writing side, so a later change that moves the range check into the encoder could quietly narrow what the encoder spells.
Every caller of the encoder repeated the same bound, because the function
took a `uint32_t` and a value outside the Unicode range wraps into a
codepoint under that cast, so the range had to be tested on the `mrb_int`
before the call. The `return 0` the encoder already had for a value it cannot
spell was therefore unreachable from every one of them.
Take an `mrb_int` instead. The cast disappears, and with it the reason the
bound could not live in one place. What each caller keeps is the part that
genuinely differs: which exception it raises, since CRuby raises ArgumentError
for `sprintf("%c")` and RangeError for `pack("U")` and `Integer#chr`.
`Integer#chr` keeps its surrogate test as well. A surrogate is a value the
encoder does spell, so refusing it is a rule of that method rather than of
UTF-8 encoding, and the encoder now says so where it is defined. Likewise the
regexp compiler keeps `check_unicode_cp()`, which reports at the point the
pattern is read rather than at the point bytes are emitted.
No behavior changes. A build with `MRB_UTF8_STRING`, one without, and one with
`MRB_INT32` agree with the previous code on every boundary value across
`sprintf("%c")`, `pack("U")`, `Integer#chr` and `String#<<`.
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe shared UTF-8 encoder now accepts ChangesUTF-8 codepoint 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mrbgems/mruby-pack/test/pack.rb`:
- Around line 217-224: The wrapping regression checks must first verify that the
value is representable as mrb_int rather than relying on is_a?(Integer), which
also accepts bigint values. In mrbgems/mruby-pack/test/pack.rb lines 217-224,
and mrbgems/mruby-sprintf/test/sprintf.rb lines 140-147, use an
mrb_int-requiring operation such as []\[wrapping\]; skip the respective
pack("U") or sprintf("%c") assertion when that operation raises RangeError,
while preserving the existing assertion for representable values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 31d5429d-8139-4d50-b8e3-8abcdc873450
📒 Files selected for processing (8)
include/mruby/internal.hmrbgems/mruby-pack/src/pack.cmrbgems/mruby-pack/test/pack.rbmrbgems/mruby-regexp/src/re_compile.cmrbgems/mruby-sprintf/src/sprintf.cmrbgems/mruby-sprintf/test/sprintf.rbmrbgems/mruby-string-ext/src/string.csrc/string.c
`is_a?(Integer)` is true for a big integer as well, so on a build with a
32-bit `mrb_int` and bigint the two assertions ran on a value the encoder
never sees. `pack("U")` refuses a big integer while converting the element,
and `%c` takes one down the branch for an argument that is not an integer
and refuses it there, so both passed without the truncation they guard
against having been reachable at all.
Probe with `[][wrapping]`, which answers `nil` for an `mrb_int` index and
raises `RangeError` for a big integer, the way `mruby-socket` and
`mruby-random` already ask the same question.
The duplication
mrb_utf8_to_buf()took auint32_t, so a value outside the Unicode rangewraps into a codepoint under the cast:
(uint32_t)(0x100000000 + 0x41)is0x41, and the encoder would spellA. Every caller therefore had to testthe range on the
mrb_intbefore the call, and all four spelled the samebound out:
mruby-sprintf,%ccode < 0 || 0x10FFFF < codemruby-pack,pack("U")c < 0 || 0x10FFFF < cmruby-string-ext,Integer#chrcp < 0 || 0x10FFFF < cp || surrogatemruby-regexp,\u{...}cp > 0x10ffff || surrogateBecause all four rejected anything above U+10FFFF first, the
return 0theencoder already carried for a value it cannot spell was unreachable from
every one of them. Two of the four also carried a copy of the same comment
explaining the cast.
The change
Take an
mrb_int. The cast disappears, and with it the reason the boundcould not live in one place. Each caller keeps only the part that genuinely
differs between them, which is the exception:
CRuby 4.0.6 raises those same three, so this split is not one the encoder can
absorb.
What stays where, and why
Integer#chrkeeps its surrogate test. A surrogate is a value the encoderdoes spell, and CRuby spells it too:
So refusing a surrogate is a rule of
Integer#chrrather than a rule ofUTF-8 encoding. It also means what
mrb_utf8_to_buf()writes is deliberatelywider than what
mrb_utf8len()reads back under RFC 3629, andin CRuby as well as in mruby. The encoder's comment now states that where the
encoder is defined, instead of leaving it to be inferred from which callers
happen to carry a surrogate test.
mruby-regexpkeepscheck_unicode_cp(). It reports at the point thepattern is read rather than at the point bytes are emitted, and the message
carries the pattern:
Its codepoint also comes from a hex scan capped at six digits, so it can
never reach the encoder unchecked.
mruby-packgained its own version of this check in #7114, for the samereason the other three carried one. With the cast gone, that reason is gone
with it.
Tests
The first commit pins what the encoder spells for a surrogate, before
anything moves. The upper bound, negative values and the truncation case
already had tests; the writing side had none. Adding a surrogate rejection to
mrb_utf8_to_buf()fails exactly the two new assertions and nothing else.No behavior change. Three builds, one with
MRB_UTF8_STRING, one without,and one with
MRB_UTF8_STRINGandMRB_INT32, agree with the previous codeon
-1,-0x10FFFF,0,0x7F,0x80,0x7FF,0x800,0xD7FF,0xD800,0xDFFF,0xE000,0xFFFF,0x10000,0x10FFFF,0x110000,0x7FFFFFFFand 32-bit truncation candidates, acrosssprintf("%c"),pack("U"),Integer#chr("UTF-8")andString#<<.rake testis green onall three, and each commit was checked on its own.
Summary by CodeRabbit
Bug Fixes
%cformatting reports a clear “invalid character” error for unsupported values.Tests