lriggs commented on code in PR #50356:
URL: https://github.com/apache/arrow/pull/50356#discussion_r3532858294
##########
cpp/src/gandiva/precompiled/string_ops.cc:
##########
@@ -208,7 +208,7 @@ gdv_int32 utf8_length_ignore_invalid(const char* data,
gdv_int32 data_len) {
}
for (int j = 1; j < char_len; ++j) {
if ((data[i + j] & 0xC0) != 0x80) { // bytes following head-byte of
glyph
- char_len += 1;
+ break;
Review Comment:
break silently swallows valid bytes inside a malformed sequence's declared
window
When break fires at index j, char_len still holds the original
utf8_char_length value (e.g. 4 for 0xF0). The outer loop then advances i +=
char_len, consuming all char_len bytes as a single glyph — including any valid
ASCII or UTF-8 characters that fall between position j and char_len-1.
Concrete: {0xF0, 'a', 0xE2, 0x82, 0xAC} (malformed 4-byte lead + 'a' + valid
€ U+20AC). Break fires at j=1; the loop advances by 4, eating 0xE2 and 0x82. At
i=4 the function sees only the orphaned 0xAC, counts it as an isolated invalid
byte, and returns 2. The € is gone.
The fix that preserves the OOB safety without this side-effect is char_len =
j; break — advance only the confirmed bytes before the mismatch (typically 1,
the lead byte itself), then let the outer loop parse 'a', 0xE2, 0x82, 0xAC each
on their own iteration.
Here is a test to confirm:
TEST(TestStringOps, TestUtf8LengthIgnoreInvalidSwallowsValidGlyph) {
// {0xF0, 'a', 0xE2, 0x82, 0xAC}: malformed 4-byte lead + ASCII 'a' +
U+20AC €.
//
// Correct: 0xF0 alone counts as 1 invalid glyph, 'a' = 1, € = 1 → 3.
// Actual (plain break): char_len stays 4 after breaking at j=1; i advances
// to 4, consuming 'a', 0xE2, 0x82. At i=4 only 0xAC is seen (isolated
// continuation byte) → 2.
std::vector<char> text = {'\xF0', 'a', '\xE2', '\x82', '\xAC'};
const auto text_len = static_cast<gdv_int32>(text.size());
gandiva::ExecutionContext ctx;
uint64_t ctx_ptr = reinterpret_cast<gdv_int64>(&ctx);
gdv_int32 out_len = 0;
const std::string text_str(text.data(), text.size());
// 3 glyphs → padding to width 5 adds 2 spaces → out_len = 2 + 5 = 7.
// With plain break the count is 2, adding 3 spaces → out_len = 8.
const char* out_str =
lpad_utf8_int32_utf8(ctx_ptr, text.data(), text_len, 5, " ", 1,
&out_len);
EXPECT_EQ(out_len, 7);
EXPECT_EQ(std::string(out_str, out_len), " " + text_str);
out_str = rpad_utf8_int32_utf8(ctx_ptr, text.data(), text_len, 5, " ", 1,
&out_len);
EXPECT_EQ(out_len, 7);
EXPECT_EQ(std::string(out_str, out_len), text_str + " ");
}
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
