mruby-bin-mirb: read UTF-8 through the core scanner - #7123
Merged
Conversation
mirb_buffer.c carried its own lead byte table and character length scanner, restating what string.c already answers. Both sit behind MRB_UTF8_STRING, so the copy was asked the same question as `mrb_utf8len` and `mrb_utf8_char_head`, and only the original has been kept correct. The copy checked continuation bytes and nothing else, so it read a sequence RFC 3629 rejects as one character. An overlong "\xE0\x80\xAF" measured 3 bytes to it and 1 to `mrb_utf8len`, a surrogate "\xED\xA0\x80" 3 and 1, and "\xF5\x80\x80\x80" above U+10FFFF 4 and 1. Walking back was looser still: it took the first lead byte within four bytes without asking whether that byte reaches the cursor, so a continuation byte no character covers swallowed the character before it. Left, right, and backspace now step over the bytes the core counts. `utf8_display_col` also passed `str + byte_pos + 4` as the end of what it scans, reading up to four bytes past the position it was asked about. It now passes the end it was given. The display width helpers stay: how wide a character prints is a question the core answers nothing about.
|
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)
📝 WalkthroughWalkthrough
Changesmirb UTF-8 handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 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 |
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
2 participants
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.
mrbgems/mruby-bin-mirb/tools/mirb/mirb_buffer.ccarries its own copy of theUTF-8 scanner that
src/string.calready has. Both are compiled only underMRB_UTF8_STRING, so the two are asked the same question, and the copy hasdrifted from the original.
mirb_buffer.csrc/string.cutf8_islead()utf8_isleadmacroutf8_len_table[]mrb_utf8len_table[]utf8_char_len()mrb_utf8len()utf8_prev_char_start()mrb_utf8_char_head()This drops the copy and calls
mrb_utf8len()andmrb_utf8_char_head()instead, removing 69 lines against 15 added.
What changes for the user
The copy validates continuation bytes and nothing else, so it reads a sequence
RFC 3629 rejects as a single character.
mrb_utf8len()rejects overlong forms,surrogates, and anything above U+10FFFF, so the cursor now steps over the same
bytes the core counts:
\xE0\x80\xAF(overlong U+002F)\xC1\x81(overlong U+0041)\xED\xA0\x80(surrogate U+D800)\xF4\x90\x80\x80(above U+10FFFF)\xF5\x80\x80\x80Well formed UTF-8 measures the same either way; ASCII, U+3042, U+1F363, and a
truncated lead byte all agree.
Walking back was looser still.
utf8_prev_char_start()took the first leadbyte it found within four bytes without asking whether that byte reaches the
cursor, so a continuation byte that no character covers swallowed the character
before it: with
"a\x81"in the line, cursor left jumped from byte offset 2 to0 rather than to 1.
mrb_utf8_char_head()treats such a byte as a character ofits own.
Two more things fall out of this:
utf8_display_col()passedstr + byte_pos + 4as the end of the string itscans, with a
/* +4 for safety */comment, so it could read up to four bytespast the position it was asked about. It now passes the end it was given.
mirb_buffer.cgains#include <mruby/internal.h>, whichmirb.cin thesame gem already includes.
The display width helpers (
utf8_char_width()andutf8_display_col()) stay asthey are. How wide a character prints on a terminal is a question the core
answers nothing about.
Testing
Built with the
full-coregembox, wheremruby-encodingdefinesMRB_UTF8_STRING, and with thedefaultgembox, which does not. Neither buildproduces a new warning.
bintest/mirb.rbdrives mirb through a pipe, so it never enters raw mode andnever reaches this code. I exercised the buffer through its public API instead
(insert, cursor left and right, backspace, delete, display column) over
hiragana, a 4 byte emoji, ASCII, and each of the ill formed sequences above, and
separately compared the removed functions against the core ones to produce the
table above.
Summary by CodeRabbit