Conversation
In mask_utf8_utf8_utf8_utf8, the output buffer size is computed as std::max(upper_length, std::max(lower_length, num_length)) * data_len without an overflow guard. The multiplication is done in int32_t, so a large replacement string combined with a large input wraps around, producing a small or negative max_length. The arena allocation is then far smaller than needed, causing a heap buffer overflow in the subsequent memcpy loop. Fix: use arrow::internal::MultiplyWithOverflow to detect the overflow and return an error, consistent with sibling functions (repeat_utf8_int32, to_hex_binary, etc.). Fixes apache#50472
soundex_utf8 in gdv_string_function_stubs.cc used locale-sensitive isalpha()/toupper() to classify characters, which under non-C locales (accepts 0x80-0xFF bytes as letters) causes the mappings[] table (26 entries, A-Z) to be indexed past its bounds. Replace with ASCII-only range checks, consistent with the soundex specification (which only processes A-Z).
|
Thanks for opening a pull request! This pull request has been automatically converted to a draft because its title doesn't match Arrow's required format. If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or After updating the title, you can mark the pull request as ready for review. See also: |
github-actions
Bot
added
Component: C++
Component: Gandiva
awaiting review
Remove the unrelated mask allocation change. Exercise all high bytes in C and ISO-8859-1 locales as initial and subsequent input bytes. Assisted-by: Codex:GPT-6
VrtxOmega
changed the title
VrtxOmega
marked this pull request as ready for review
September 24, 2026 01:44
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
1 participant
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 #50893.
soundex_utf8uses a 26-entry A–Z mapping table. Locale-sensitiveisalpha/touppercan accept non-ASCII bytes in a single-byte locale such as ISO-8859-1 and generate an invalid table index. Use ASCII range checks for both the first retained letter and subsequent letters.This PR contains only the Soundex implementation and its tests. The unrelated mask allocation change has been removed; that work remains in #50947. Tests exercise every byte from 128 through 255 before and inside
Robert, in both the C and ISO-8859-1 locales.Local Linux validation at head
057e53ddbe874c3bd82f20da42fe59d308fb6f7b(treeb843accd774f016f07c0e398abb8b64926371bfd): Clang 18 Debug with ASan and UBSan; all three Soundex tests passed, including the installed ISO-8859-1 locale, with no skip. Reverting the implementation makes UBSan report index -151 out of bounds forchar[26]. Restoring the fix passes all 136 precompiled tests. That full run required the standardtzdata-legacypackage for an existing Canada/Pacific timestamp fixture.These are local results; upstream CI and maintainer review remain separate. This follow-up was assisted by Codex and is disclosed in the commit trailer.