(arrow) branch main updated: GH-50380: [C++][Gandiva] fix out-of-bounds read in byte_substr past end (#50381)

[kou]

This is an automated email from the ASF dual-hosted git repository.

kou pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git


The following commit(s) were added to refs/heads/main by this push:
     new 7cea8ada31 GH-50380: [C++][Gandiva] fix out-of-bounds read in 
byte_substr past end (#50381)
7cea8ada31 is described below

commit 7cea8ada31fd35e8e18f3a10328515b0932f932f
Author: Abdul Rawoof Khan <abd...@bugqore.com>
AuthorDate: Wed Jul 8 11:26:37 2026 +0530

    GH-50380: [C++][Gandiva] fix out-of-bounds read in byte_substr past end 
(#50381)
    
    ### Rationale for this change
    
    `byte_substr_binary_int32_int32` derives `startPos` from the offset 
argument but never confirms it falls inside the text. A positive `offset` 
larger than `text_len` leaves `startPos >= text_len`, so `text_len - startPos` 
is negative and that value is stored in `*out_len`. The following `memcpy` 
reads it as a large `size_t` and runs off the end of both the source text and 
the `text_len`-sized output buffer. The offset comes straight from user SQL, so 
`byte_substr(col, n, m)` with `n` p [...]
    
    ### What changes are included in this PR?
    
    Return an empty result when `startPos >= text_len`, before the length 
truncation can go negative. The check sits in the callee next to where 
`startPos` is computed so every caller is covered.
    
    ### Are these changes tested?
    
    Yes, `TestByteSubstr` gains a case with the offset past the end asserting 
an empty result and no error. The existing cases are unchanged.
    
    ### Are there any user-facing changes?
    
    No.
    
    **This PR contains a "Critical Fix".** It fixes an out-of-bounds read (and 
oversized copy) in `byte_substr` reachable from user-supplied offsets.
    * GitHub Issue: #50380
    
    Authored-by: abdul rawoof <abd...@bugqore.com>
    Signed-off-by: Sutou Kouhei <k...@clear-code.com>
---
 cpp/src/gandiva/precompiled/string_ops.cc      | 29 ++++++++++++++++++--------
 cpp/src/gandiva/precompiled/string_ops_test.cc | 14 +++++++++++++
 2 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/cpp/src/gandiva/precompiled/string_ops.cc 
b/cpp/src/gandiva/precompiled/string_ops.cc
index 90484f2970..70cad7c626 100644
--- a/cpp/src/gandiva/precompiled/string_ops.cc
+++ b/cpp/src/gandiva/precompiled/string_ops.cc
@@ -2472,6 +2472,21 @@ 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;
+  }
+
+  // an offset past the end of the text leaves nothing to copy; without this 
the
+  // truncation below yields a negative *out_len that memcpy reads as a huge 
size.
+  // check before allocating so a past-end offset needs no output buffer at all
+  if (startPos >= text_len) {
+    *out_len = 0;
+    return "";
+  }
+
   char* ret =
       reinterpret_cast<gdv_binary>(gdv_fn_context_arena_malloc(context, 
text_len));
 
@@ -2481,15 +2496,11 @@ 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) {
+  // calculate end position from length and truncate to upper value bounds.
+  // startPos < text_len is guaranteed above, so text_len - startPos is 
positive;
+  // comparing against it avoids the startPos + length overflow when length is
+  // near INT32_MAX, which would otherwise leave *out_len huge for the memcpy.
+  if (length > text_len - startPos) {
     *out_len = text_len - startPos;
   } else {
     *out_len = length;
diff --git a/cpp/src/gandiva/precompiled/string_ops_test.cc 
b/cpp/src/gandiva/precompiled/string_ops_test.cc
index 5a317d4595..c866b88540 100644
--- a/cpp/src/gandiva/precompiled/string_ops_test.cc
+++ b/cpp/src/gandiva/precompiled/string_ops_test.cc
@@ -1903,6 +1903,20 @@ TEST(TestStringOps, TestByteSubstr) {
   out_str = byte_substr_binary_int32_int32(ctx_ptr, "TestString", 10, -100, 
10, &out_len);
   EXPECT_EQ(std::string(out_str, out_len), "TestString");
   EXPECT_FALSE(ctx.has_error());
+
+  // offset past the end of the text must yield an empty result, not a negative
+  // length that memcpy reads as an out-of-bounds copy
+  out_str = byte_substr_binary_int32_int32(ctx_ptr, "TestString", 10, 15, 10, 
&out_len);
+  EXPECT_EQ(out_len, 0);
+  EXPECT_EQ(std::string(out_str, out_len), "");
+  EXPECT_FALSE(ctx.has_error());
+
+  // a huge length must be truncated to the remaining bytes, not overflow
+  // startPos + length and copy far past the end of the text
+  out_str =
+      byte_substr_binary_int32_int32(ctx_ptr, "TestString", 10, 2, 2147483647, 
&out_len);
+  EXPECT_EQ(std::string(out_str, out_len), "estString");
+  EXPECT_FALSE(ctx.has_error());
 }
 
 TEST(TestStringOps, TestStrPos) {