lriggs commented on code in PR #50381:
URL: https://github.com/apache/arrow/pull/50381#discussion_r3531369710
##########
cpp/src/gandiva/precompiled/string_ops.cc:
##########
@@ -2480,13 +2495,6 @@ const char* byte_substr_binary_int32_int32(gdv_int64
context, const char* text,
return "";
}
- int32_t startPos = 0;
- if (offset >= 0) {
- startPos = offset - 1;
- } else if (text_len + offset >= 0) {
- startPos = text_len + offset;
- }
-
// calculate end position from length and truncate to upper value bounds
if (startPos + length > text_len) {
Review Comment:
startPos and length are both int32_t (gdv_int32). After the new early-return
guard, startPos is in [0, text_len-1], but length is only required to be > 0 —
no upper bound. When length is near INT32_MAX and startPos >= 1, the addition
startPos + length overflows signed int32_t to a large negative value. The
comparison negative > text_len is false, so the else branch fires: *out_len =
length ≈ INT32_MAX. Then memcpy(ret, text + startPos, INT32_MAX) is called
against a text_len-byte source and a text_len-byte arena destination — an ~2 GB
overread and overwrite on both buffers.
Concrete inputs: text_len=100, offset=2 → startPos=1, length=2147483647 →
*out_len = 2147483647 → memcpy(ret, text+1, 2147483647) with 100-byte buffers
on both ends.
The same fix that guards the source UTF-8 sibling substr_utf8_int64_int64
applies here: promote to 64-bit before comparing:
if ((int64_t)startPos + length > text_len) {
Or, since startPos < text_len is now guaranteed, even simpler and
overflow-free in int32_t:
if (length > text_len - startPos) {
--
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]
