GH-50355: [C++][Gandiva] fix out-of-bounds read in utf8_length_ignore_invalid

[Arawoof06]

Rationale for this change

utf8_length_ignore_invalid extends char_len while scanning the bytes after a lead byte and never rechecks the buffer end, so an input ending in a truncated multi-byte utf8 sequence (a 0xF0 lead byte followed by non-continuation bytes) reads past data_len. It is reached from untrusted string data through lpad/rpad, which count the input glyphs before padding. Reproduced against a verbatim copy of the function under AddressSanitizer with the 4-byte input {0xF0, 'a', 'a', 'a'} in an exactly-sized heap buffer, giving heap-buffer-overflow READ ... 0 bytes after 4-byte region.

What changes are included in this PR?

Stop the inner scan with a break when a byte after the lead byte is not a continuation byte, instead of incrementing char_len. Growing char_len on each stray byte kept extending the loop past data_len; breaking leaves char_len bounded so the outer i + char_len <= data_len check keeps every read in range. Valid input counts the same, because a well-formed glyph has only continuation bytes after its lead byte and never hits the break.

Are these changes tested?

Yes. Added TestStringOps.TestPadMalformedUtf8NoOverread, which runs lpad/rpad on the truncated multi-byte input placed in an exactly-sized heap buffer so the over-read trips ASAN, and asserts the full padded output. The existing pad tests still pass.

Are there any user-facing changes?

No.

This PR contains a "Critical Fix". It fixes an out-of-bounds read in the Gandiva utf8 length helper reachable from lpad/rpad on crafted string data.

• GitHub Issue: [C++][Gandiva] Out-of-bounds read in utf8_length_ignore_invalid on truncated multi-byte input #50355

[github-actions]

⚠️ GitHub issue #50355 has been automatically assigned in GitHub to PR creator.

[kou]

Hmm, it seems that CODEOWNERS configuration by GH-50144 doesn't work...

@dmitry-chirkov-dremio @lriggs @akravchukdremio @xxlaykxx Could you take a look at this?

[kou]

Could you check out_str data entirely instead of checking only part of out_str?

[Arawoof06]

Done, now checks the whole out_str including the padding (" " + text for lpad).

[kou]

ditto.

[Arawoof06]

Same here, rpad now asserts the full out_str (text + trailing spaces).

[kou]

Hmm, should we break here instead?

Suggested change

	char_len += 1;
	break;

[Arawoof06]

Good call, switched to break. Once the scan stops growing char_len it never runs past the outer i + char_len <= data_len check, so the extra i + j < data_len guard isn't needed anymore. Valid input counts the same and the malformed case is clean under ASAN.

[kou]

Could you update the PR description?

I'll wait for a review from Gandiva developers before I merge this.

[Arawoof06]

Updated the description to match the current fix (the break instead of the earlier bound check). Thanks for waiting on the Gandiva folks.

[lriggs]

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 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 + " ");
}

[Arawoof06]

Good catch, you're right. Plain break kept char_len at the declared width so the outer loop skipped the bytes after the mismatch. Switched to char_len = j; break so only the confirmed bytes get consumed and the rest are re-parsed. Your {0xF0,'a',0xE2,0x82,0xAC} case now counts 3. I added that as TestPadMalformedUtf8KeepsValidGlyph, and the earlier {0xF0,'a','a','a'} test now expects length 4 (lead byte + three ascii) which was the same bug hiding in the old assertion. Verified the counts under ASAN.

[lriggs]

Looks good

[Copilot]

Pull request overview

This PR fixes an out-of-bounds read in Gandiva’s UTF-8 glyph counting helper (utf8_length_ignore_invalid), which is reachable via lpad/rpad when processing malformed/untrusted UTF-8 input.

Changes:

• Bound the inner “continuation byte” scan in utf8_length_ignore_invalid by stopping at the first non-continuation byte (preventing char_len from growing past the remaining buffer).
• Add regression tests that exercise lpad/rpad with malformed UTF-8 inputs (including a case ensuring valid glyphs after a malformed lead byte aren’t skipped).
• Add a test include needed by the new cases.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
cpp/src/gandiva/precompiled/string_ops.cc	Fixes the UTF-8 length helper to avoid extending reads past data_len on malformed sequences.
cpp/src/gandiva/precompiled/string_ops_test.cc	Adds regression tests covering malformed UTF-8 padding scenarios intended to catch the prior over-read under ASAN.

[Arawoof06]

Fair point, std::vector doesn't promise capacity == size. Switched both tests to an exact-sized new char[] behind a unique_ptr and memcpy the bytes in, so an over-read lands past the allocation and ASAN trips reliably.

[Arawoof06]

Pushed a small test-only tweak: the two malformed-utf8 tests now use an exact-sized new char[] instead of std::vector so the over-read reliably trips ASAN (vector doesn't guarantee capacity == size). The fix in string_ops.cc is unchanged.

[kou]

Can we remove this now?

Suggested change

#include <vector>

[Arawoof06]

Yep, nothing uses std::vector in these tests anymore. Dropped the include.