fix: cast to unsigned char in ASCII case conversion (1/2)#748
Merged
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
zhjwpku
approved these changes
Jun 16, 2026
| private: | ||
| // std::tolower/std::toupper require their argument to be representable as | ||
| // unsigned char (or EOF); passing a raw char with a non-ASCII (negative) value is | ||
| // undefined behavior, so cast through unsigned char first. |
Collaborator
There was a problem hiding this comment.
+1, Quoting cppreference:
Like all other functions from <cctype>, the behavior of std::tolower is undefined if the argument's value is neither representable as unsigned char nor equal to EOF. To use these functions safely with plain chars (or signed chars), the argument should first be converted to unsigned char:
char my_tolower(char ch)
{
return static_cast<char>(std::tolower(static_cast<unsigned char>(ch)));
}
There was a problem hiding this comment.
Pull request overview
Fixes undefined behavior in ASCII case conversion utilities by ensuring std::tolower/std::toupper aren’t invoked with potentially-negative char values, and adds tests to cover non-ASCII byte behavior relevant to case-insensitive comparisons (Issue #613).
Changes:
- Add
<cctype>and routeToLower,ToUpper, andEqualsIgnoreCasethrough helper functions that cast viaunsigned charbefore callingstd::tolower/std::toupper. - Document ASCII-only intent and non-ASCII (UTF-8 multibyte) pass-through behavior.
- Add tests for non-ASCII byte pass-through and
EqualsIgnoreCasebehavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/iceberg/util/string_util.h | Adds safe ASCII case conversion helpers and updates case-insensitive utilities to use them. |
| src/iceberg/test/string_util_test.cc | Adds regression tests covering non-ASCII byte handling and EqualsIgnoreCase. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
wgtmac
approved these changes
Jun 17, 2026
Member
wgtmac
pushed a commit
that referenced
this pull request
Jul 11, 2026
Replaces the ASCII-only `StringUtils::ToLower` with a Unicode-aware implementation backed by [utf8proc](https://github.com/JuliaStrings/utf8proc), aligning case-insensitive name handling with Iceberg Java's `toLowerCase(Locale.ROOT)` for simple (1:1) case mappings. The few code points where simple and full case mapping differ (e.g. `İ` U+0130) are tracked in #808. - `ToLower` lower-cases UTF-8 using utf8proc simple (1:1) case mapping (e.g. `CAFÉ` → `café`, `GROẞE` → `große`). Pure-ASCII input takes a byte-wise fast path and never touches utf8proc. - Invalid UTF-8 is handled as a correctness guarantee (raised in review): `ToLower` is total — a byte that does not begin a valid UTF-8 sequence is passed through unchanged and decoding resumes at the next byte, so the valid code points around it are still lower-cased. The comparisons below therefore compare invalid bytes verbatim. - `EqualsIgnoreCase` and `StartsWithIgnoreCase` fold through `ToLower`, so they are case-insensitive for non-ASCII letters too. Both take an allocation-free ASCII fast path and fall back to `ToLower` only when a non-ASCII byte is present. - `StartsWithIgnoreCase` no longer assumes case mapping preserves byte length — the old byte-slice guard mishandled length-changing maps (e.g. `İ` U+0130 → `i`). Now `"İx"` starts with `"i"`, and `"i"` starts with `"İ"`. - `ToUpper` is intentionally left ASCII-only — it only normalizes ASCII enum/codec strings, and simple case mapping would be wrong for some letters (e.g. `ß` would stay unchanged instead of becoming `SS`). - utf8proc is pinned to 2.10.0 and wired into both the CMake (vendored via FetchContent / system package) and Meson (`subprojects/utf8proc.wrap`) builds, with matching source hashes and correct static-vs-shared linkage on Windows. ## Testing - `string_util_test.cc`: `ToLowerUnicode` (incl. invalid / truncated / stray-continuation bytes), `ToUpperAsciiOnly`, and Unicode + invalid-UTF-8 cases for `EqualsIgnoreCase` / `StartsWithIgnoreCase`. - `IgnoreCaseAgreesWithToLowerOracle` exhaustively checks both ASCII fast paths against the `ToLower` oracle over all short strings drawn from an alphabet with length-changing maps (`İ`→`i`, `K`→`k`) and invalid UTF-8. ## Remaining work (tracked in #808) - Full case-mapping parity with Java for the code points where simple ≠ full mapping (e.g. `İ` → `i̇`). - A streaming (allocation-free) non-ASCII path for `EqualsIgnoreCase` / `StartsWithIgnoreCase`. Part of #613 — does not fully fix it; #808 holds the design and remaining plan. Follow-up to #748.
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.
fix: cast to unsigned char in ASCII case conversion
std::tolower/toupperare UB when passed a negativechar, which is what you get for any non-ASCII byte in a signedchar.StringUtils::ToLower,ToUpper, andEqualsIgnoreCaseall did this. Cast through
unsigned char.Behavior for ASCII is unchanged; this doesn't add Unicode case folding, just fixes the UB.
Related: #613