numfmt: stop a multi-byte locale decimal separator from panicking - #14325
Open
arbelonson-source wants to merge 2 commits into
Open
numfmt: stop a multi-byte locale decimal separator from panicking#14325arbelonson-source wants to merge 2 commits into
arbelonson-source wants to merge 2 commits into
Conversation
Fixes uutils#13937. Under a locale whose decimal separator is multi-byte (e.g. Arabic ٫, 2 bytes), find_valid_number_with_suffix skipped past the numeric part by treating numeric_part.len() -- a byte length -- as a character count for Iterator::skip. Whenever the numeric part contained the multi-byte separator, that landed one or more characters too far, examining the wrong character as the "suffix" -- one that could itself be multi-byte (e.g. €) and, if it happened to look like a recognized suffix from that wrong position, get byte-sliced using an end index computed for a hardcoded 1-byte suffix width. On an actual multi-byte character there, that index falls inside it rather than on a char boundary, which panics: 'byte index 4 is not a char boundary; it is inside '€' (bytes 3..6)'. panic=abort turns that into a SIGABRT (exit 134) instead of GNU's clean 'invalid suffix in input' (exit 2). Slice by numeric_part.len() -- safe, since numeric_part is itself a byte-aligned prefix of the input -- and read characters directly from that point instead of skipping character-by-character from the start, and size each computed slice by the suffix's own real UTF-8 byte width rather than assuming 1. Also merged two match arms clippy correctly flagged as identical once both used the same real-width computation, rather than the previous hardcoded-width arithmetic that happened to differ only by coincidence. AI-assisted-by: Claude Opus 5, via Claude Code
|
GNU testsuite comparison: |
sylvestre
reviewed
Aug 31, 2026
Comment on lines
+60
to
+65
| // `numeric_part.len()` is a byte length; skipping that many *characters* | ||
| // would land past the real next character whenever the numeric part | ||
| // contains a multi-byte decimal separator (e.g. Arabic ٫). Slice by that | ||
| // same byte length instead, which -- since numeric_part is itself a | ||
| // prefix of s -- always falls on a char boundary, then read characters | ||
| // from there directly. |
Contributor
There was a problem hiding this comment.
make this shorter please
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.
Fixes #13937.
What
Under a locale whose decimal separator is multi-byte (e.g. Arabic
٫,2 bytes),
numfmtaborted the process instead of cleanly rejectingmalformed input:
Root cause
find_valid_number_with_suffixskipped past the numeric part bytreating
numeric_part.len()-- a byte length -- as acharacter count for
Iterator::skip. Whenever the numeric partcontained the multi-byte separator, that landed one or more
characters too far, examining the wrong character as the "suffix" --
one that could itself be multi-byte (like
€) and, if it happened tolook like a recognized suffix from that wrong position, get
byte-sliced using an end index computed for a hardcoded 1-byte suffix
width. On an actual multi-byte character there, that index lands
inside it rather than on a char boundary, which panics.
panic=abortturns that into a
SIGABRT(exit 134) instead of GNU's clean error(exit 2).
Fix
Slice by
numeric_part.len()-- safe, sincenumeric_partis itselfa byte-aligned prefix of the input, so that byte length always falls
on a char boundary -- and read characters directly from that point
instead of skipping character-by-character from the very start of the
string. Size each computed slice by the suffix's own real UTF-8 byte
width (
char::len_utf8()) rather than assuming 1. Also merged twomatch arms clippy correctly flagged as identical once both used the
same real-width computation, rather than the previous
hardcoded-width arithmetic that happened to only differ by
coincidence.
Testing
cargo test -p uu_numfmt/ fulltests/by-util/test_numfmt.rssuite: 186 passed, 0 failed.1٫€K,1٫€Kx,1٫€Ki), underLC_ALL=ar_SA.UTF-8. Verified this test actually fails with the exact reported panic against the pre-fix code (not vacuously passing due to locale unavailability).--from=si '1٫5K'-style input's behavior is unchanged by this fix (verified identical before and after against the pre-fix code) -- it's a separate, pre-existing difference (uutils currently accepts a locale decimal separator there that GNU's own--from=sidoesn't), out of scope for this crash fix.cargo clippy -p uu_numfmt --all-targets -- -D warningsandcargo fmt --check: clean.This PR was written with AI assistance (Claude Opus 5, via Claude Code). I've tested the changes but please review the code carefully.