Skip to content

Check the Unicode range in mrb_utf8_to_buf instead of in its four callers - #7129

Merged
matz merged 3 commits into
mruby:masterfrom
takumin:utf8-to-buf-range-check
Aug 12, 2026
Merged

Check the Unicode range in mrb_utf8_to_buf instead of in its four callers#7129
matz merged 3 commits into
mruby:masterfrom
takumin:utf8-to-buf-range-check

Conversation

@takumin

@takumin takumin commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

The duplication

mrb_utf8_to_buf() took a uint32_t, so a value outside the Unicode range
wraps into a codepoint under the cast: (uint32_t)(0x100000000 + 0x41) is
0x41, and the encoder would spell A. Every caller therefore had to test
the range on the mrb_int before the call, and all four spelled the same
bound out:

caller check before the call
mruby-sprintf, %c code < 0 || 0x10FFFF < code
mruby-pack, pack("U") c < 0 || 0x10FFFF < c
mruby-string-ext, Integer#chr cp < 0 || 0x10FFFF < cp || surrogate
mruby-regexp, \u{...} cp > 0x10ffff || surrogate

Because all four rejected anything above U+10FFFF first, the return 0 the
encoder 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 bound
could not live in one place. Each caller keeps only the part that genuinely
differs between them, which is the exception:

sprintf("%c", 0x110000)   # ArgumentError: invalid character
[0x110000].pack("U")      # RangeError
0x110000.chr("UTF-8")     # RangeError

CRuby 4.0.6 raises those same three, so this split is not one the encoder can
absorb.

What stays where, and why

Integer#chr keeps its surrogate test. A surrogate is a value the encoder
does spell, and CRuby spells it too:

sprintf("%c", 0xD800).bytes   #=> [237, 160, 128]
[0xD800].pack("U").bytes      #=> [237, 160, 128]
0xD800.chr("UTF-8")           # RangeError: invalid codepoint 0xD800 in UTF-8

So refusing a surrogate is a rule of Integer#chr rather than a rule of
UTF-8 encoding. It also means what mrb_utf8_to_buf() writes is deliberately
wider than what mrb_utf8len() reads back under RFC 3629, and

sprintf("%c", 0xD800).valid_encoding?   #=> false

in 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-regexp keeps check_unicode_cp(). It reports at the point the
pattern is read rather than at the point bytes are emitted, and the message
carries the pattern:

Regexp.new("\\u{D800}")   # RegexpError: invalid Unicode range: /\u{D800}/

Its codepoint also comes from a hex scan capped at six digits, so it can
never reach the encoder unchecked.

mruby-pack gained its own version of this check in #7114, for the same
reason 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_STRING and MRB_INT32, agree with the previous code
on -1, -0x10FFFF, 0, 0x7F, 0x80, 0x7FF, 0x800, 0xD7FF,
0xD800, 0xDFFF, 0xE000, 0xFFFF, 0x10000, 0x10FFFF, 0x110000,
0x7FFFFFFF and 32-bit truncation candidates, across sprintf("%c"),
pack("U"), Integer#chr("UTF-8") and String#<<. rake test is green on
all three, and each commit was checked on its own.

Summary by CodeRabbit

  • Bug Fixes

    • Improved UTF-8 handling for integer-to-character conversions across packing, formatting, regular expressions, and string utilities.
    • Invalid negative, oversized, and out-of-range code points are now rejected consistently.
    • UTF-16 surrogate values now produce consistent UTF-8 output and round-trip behavior where supported.
    • %c formatting reports a clear “invalid character” error for unsupported values.
  • Tests

    • Expanded coverage for surrogate code points, adjacent Unicode characters, exact UTF-8 bytes, and values exceeding 32-bit ranges.

`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#<<`.
@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: a328df9b-aca7-4a4e-ac54-64eea2f37931

📥 Commits

Reviewing files that changed from the base of the PR and between 8706fb8 and 3dbd0f5.

📒 Files selected for processing (2)
  • mrbgems/mruby-pack/test/pack.rb
  • mrbgems/mruby-sprintf/test/sprintf.rb
🚧 Files skipped from review as they are similar to previous changes (2)
  • mrbgems/mruby-pack/test/pack.rb
  • mrbgems/mruby-sprintf/test/sprintf.rb

📝 Walkthrough

Walkthrough

The shared UTF-8 encoder now accepts mrb_int codepoints and returns zero for negative or above-U+10FFFF values. Pack, sprintf, regexp, and string conversion callers use this behavior. Tests cover surrogate and boundary codepoints.

Changes

UTF-8 codepoint validation

Layer / File(s) Summary
Update UTF-8 encoder contract
include/mruby/internal.h, src/string.c
mrb_utf8_to_buf now accepts mrb_int, rejects negative and above-U+10FFFF values, and documents surrogate encoding.
Adopt encoder-based validation
mrbgems/mruby-pack/src/pack.c, mrbgems/mruby-string-ext/src/string.c, mrbgems/mruby-sprintf/src/sprintf.c, mrbgems/mruby-regexp/src/re_compile.c
Callers use the encoder result for invalid codepoints and retain their specific error handling.
Extend Unicode regression coverage
mrbgems/mruby-pack/test/pack.rb, mrbgems/mruby-sprintf/test/sprintf.rb
Tests cover wrapping values, surrogate endpoints, adjacent codepoints, UTF-8 bytes, and surrogate round trips.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

  • mruby/mruby#7093: Both changes update UTF-8 encoding behavior for surrogate and out-of-range codepoints.
  • mruby/mruby#7113: Both changes update mruby-sprintf %c handling and tests.
  • mruby/mruby#7114: Both changes update pack_utf8 validation and tests.

Suggested reviewers: matz

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 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 moving Unicode range validation into mrb_utf8_to_buf and removing duplicated checks from its callers.
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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between ecd891b and 8706fb8.

📒 Files selected for processing (8)
  • include/mruby/internal.h
  • mrbgems/mruby-pack/src/pack.c
  • mrbgems/mruby-pack/test/pack.rb
  • mrbgems/mruby-regexp/src/re_compile.c
  • mrbgems/mruby-sprintf/src/sprintf.c
  • mrbgems/mruby-sprintf/test/sprintf.rb
  • mrbgems/mruby-string-ext/src/string.c
  • src/string.c

Comment thread mrbgems/mruby-pack/test/pack.rb Outdated
`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.
@matz
matz merged commit ef871b2 into mruby:master Aug 12, 2026
21 checks passed
@takumin
takumin deleted the utf8-to-buf-range-check branch August 13, 2026 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants