GH-49753: [C++][Gandiva] Fix overflow in string functions. by abtom87 · Pull Request #49813 · apache/arrow

abtom87 · 2026-04-20T19:42:11Z

Rationale for this change

Fix the overflow in functions where strings are used

What changes are included in this PR?

Fixes overflow and handles negative lengths

Are these changes tested?

Yes, modified relevant google tests and ran them locally.

Are there any user-facing changes?

No

This PR contains a "Critical Fix". (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.)

GitHub Issue: [C++][Gandiva] Some string functions may be overflowed #49753

Copilot

Pull request overview

Fixes potential integer-overflow/invalid-length issues in Gandiva string functions by adding overflow-checked allocation sizing and expanding unit tests to cover extreme and negative lengths.

Changes:

Add overflow-checked allocation calculations for quote_utf8 and to_hex_binary.
Introduce a shared allocation-length helper for gdv_fn_lower_utf8, gdv_fn_upper_utf8, and gdv_fn_initcap_utf8, including negative-length rejection.
Add/extend GoogleTest coverage for overflow and negative-length scenarios.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
cpp/src/gandiva/precompiled/string_ops.cc	Adds overflow-checked allocation sizing for `quote_utf8` and `to_hex_binary`.
cpp/src/gandiva/precompiled/string_ops_test.cc	Adds overflow-focused tests for `quote_utf8` and `to_hex_binary`.
cpp/src/gandiva/gdv_string_function_stubs.cc	Adds shared allocation-length helper and overflow checks; adjusts some integer types in `translate`.
cpp/src/gandiva/gdv_function_stubs_test.cc	Adds tests for max-length overflow and negative-length handling in upper/lower/initcap and translate.

Comments suppressed due to low confidence (1)

cpp/src/gandiva/gdv_string_function_stubs.cc:792

In the multibyte path, the from_for <= from_len loop reads from[from_for] (to compute len_char_from) before checking from_for == from_len. When from_for reaches from_len (the sentinel iteration used to handle the "not found" case), this does a 1-past-the-end read. Reorder the loop so the from_for == from_len case is handled before indexing into from (or change the loop bounds to avoid the sentinel iteration and handle the fallback after the loop).

        for (int32_t from_for = 0; from_for <= from_len; from_for += len_char_from) {
          // Updating len to char in this position
          len_char_from = gdv_fn_utf8_char_length(from[from_for]);
          // Making copy to std::string with length for this char position
          std::string copy_from_compare(from + from_for, len_char_from);
          if (from_for == from_len) {
            // If it's not in the FROM list, just add it to the map and the result.
            std::string insert_copy_value(in + in_for, len_char_in);
            // Insert in map to next loops

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-22T10:01:59Z

+  int32_t bad_in_len = std::numeric_limits<int32_t>::max() / 2 + 20;
+  out_str = to_hex_binary(ctx_ptr, binary_string, bad_in_len, &out_len);
+  EXPECT_EQ(out_len, 0);
+  EXPECT_EQ(out_str, "");


These assertions compare const char* pointers (EXPECT_EQ(out_str, "")) rather than string contents, which can be non-deterministic. Use EXPECT_STREQ(out_str, "") or compare std::string(out_str, out_len) instead, and consider asserting/resetting the context error since the overflow path sets an error message.

Suggested change

EXPECT_EQ(out_str, "");

EXPECT_STREQ(out_str, "");

EXPECT_TRUE(ctx.has_error());

ctx.Reset();

Copilot · 2026-04-22T10:02:00Z

    }
-  } else {  // If there are no multibytes in the input, work with std::strings
+  } else {
+    // If there are no multibytes in the input, work with std::strings


The comment in the has_multi_byte branch is inverted: this else executes when multibyte UTF-8 is present, but the comment says "If there are no multibytes". Please correct the comment to match the control flow to avoid confusion during future maintenance.

Suggested change

// If there are no multibytes in the input, work with std::strings

// If there are multibytes in the input, work with std::strings

Copilot · 2026-04-22T10:02:00Z

+  int32_t alloc_length = 0;
+  // Check overflow: 2 * in_len
+  if (ARROW_PREDICT_FALSE(
+          arrow::internal::MultiplyWithOverflow(2, in_len, &alloc_length))) {
+    gdv_fn_context_set_error_msg(context, "Would overflow maximum output size");
+    *out_len = 0;
+    return "";
+  }
+
  // try to allocate double size output string (worst case)
  auto out =
-      reinterpret_cast<char*>(gdv_fn_context_arena_malloc(context, (in_len * 2) + 2));
+      reinterpret_cast<char*>(gdv_fn_context_arena_malloc(context, alloc_length + 2));
  if (out == nullptr) {


quote_utf8 checks overflow for 2 * in_len, but the allocation size is alloc_length + 2. For boundary values (e.g., in_len == INT32_MAX/2), 2 * in_len fits in int32_t while + 2 overflows, which can lead to a negative/incorrect allocation size being passed to gdv_fn_context_arena_malloc. Please include the + 2 in the overflow-safe size computation (or separately check that alloc_length <= INT32_MAX - 2).

fixed where/how?

These lines check for the suggested overflow. using the AddwithOverflow method.

Copilot · 2026-04-22T10:02:00Z

  auto ret =
-      reinterpret_cast<char*>(gdv_fn_context_arena_malloc(context, text_len * 2 + 1));
+      reinterpret_cast<char*>(gdv_fn_context_arena_malloc(context, alloc_length + 1));



to_hex_binary only special-cases text_len == 0. If text_len is negative, the overflow check may not trigger and alloc_length can become negative, which then gets passed to gdv_fn_context_arena_malloc (takes an int32_t size) and can result in invalid/huge allocations or crashes. Treat negative lengths as invalid (or at least as empty) by changing the guard to text_len <= 0 (and ideally setting an error for negative input, consistent with other functions).

dmitry-chirkov-dremio · 2026-04-23T03:59:20Z

+  out_str = to_hex_binary(ctx_ptr, binary_string, neg_in_len, &out_len);
+  EXPECT_EQ(out_len, 0);
+  EXPECT_STREQ(out_str, "");
+  ctx.Reset();


Could we add a couple more boundary tests here just to make the hardening feel more complete? I’m thinking of cases like quote_utf8 right around the INT32_MAX / 2 boundary, substring_index with negative lengths, and maybe one concat_ws_* case where the combined output size would overflow.

Yes I will add more tests to cover these, later today.

I'd love to see the commit history in pull request - easier to review diffs that way.
You found few issues in the implementation via these new tests, correct?

@dmitry-chirkov-dremio Done! Could you please check? I think the concat_ws* function needs to be reimplemented using variadic template or so, a lot of redundant code can be avoided.

dmitry-chirkov-dremio · 2026-04-23T17:49:03Z

    return "";
  }

+  int32_t temp = 0;


There are also 3, 4 and 5 argument versions of concat_ws below

Yes, they have been handled, as well. Perhaps a better way might be to approach them using variadic template or so.

dmitry-chirkov-dremio · 2026-04-23T17:58:31Z

  EXPECT_EQ(std::string(out_str, out_len), "'\\'\\'\\'\\'\\'\\'\\'\\'\\''");
  EXPECT_FALSE(ctx.has_error());
+
+  int32_t bad_in_len = std::numeric_limits<int32_t>::max() / 2 + 20;


Could you check int32_t bad_in_len = std::numeric_limits<int32_t>::max() / 2 + 1? Might need to have UndefinedBehaviorSanitizer to actually catch the issue 🤔

I believe that worked, I can try adding that too. What did not work though is (max/2) -1. The logic should allocate that memory, but i got a segfault, probably because the size was too huge.

Edit: added.

…ction_stubs Fixes potential integer-overflow/invalid-length issues in gdv_string_function_stubs.cc. Expanded unit-tests. Incorporated review comments.

Fixes potential integer-overflow/invalid-length issues in string_ops.cc. Expanded unit-tests for concat_ws*. Incorporated review comments.

abtom87

Added suggested changes

abtom87 · 2026-04-23T18:37:11Z

  EXPECT_EQ(std::string(out_str, out_len), "'\\'\\'\\'\\'\\'\\'\\'\\'\\''");
  EXPECT_FALSE(ctx.has_error());
+
+  int32_t bad_in_len = std::numeric_limits<int32_t>::max() / 2 + 20;


I believe that worked, I can try adding that too. What did not work though is (max/2) -1. The logic should allocate that memory, but i got a segfault, probably because the size was too huge.

Edit: added.

abtom87 · 2026-04-23T18:58:25Z

    return "";
  }

+  int32_t temp = 0;


Yes, they have been handled, as well. Perhaps a better way might be to approach them using variadic template or so.

github-actions Bot added Component: C++ Component: Gandiva awaiting review Awaiting review labels Apr 20, 2026

abtom87 mentioned this pull request Apr 20, 2026

H-49753: [C++][Gandiva] Fix overflow in string functions. #49809

Closed

abtom87 force-pushed the fix-overflow-in-string-func branch 3 times, most recently from 14851b5 to f1be8c1 Compare April 21, 2026 17:10

kou requested a review from Copilot April 22, 2026 09:56

Copilot started reviewing on behalf of kou April 22, 2026 09:57 View session

Copilot AI reviewed Apr 22, 2026

View reviewed changes

abtom87 force-pushed the fix-overflow-in-string-func branch 3 times, most recently from d0280e4 to 8bb980f Compare April 23, 2026 03:45

dmitry-chirkov-dremio reviewed Apr 23, 2026

View reviewed changes

abtom87 force-pushed the fix-overflow-in-string-func branch from 8bb980f to 0ccb5ee Compare April 23, 2026 17:30

dmitry-chirkov-dremio reviewed Apr 23, 2026

View reviewed changes

abtom87 force-pushed the fix-overflow-in-string-func branch from 0ccb5ee to f02d531 Compare April 23, 2026 17:54

dmitry-chirkov-dremio reviewed Apr 23, 2026

View reviewed changes

abtom87 force-pushed the fix-overflow-in-string-func branch from f02d531 to e7ff043 Compare April 23, 2026 17:58

apacheGH-49753: [C++][Gandiva] Fix string functions in gdv_string_fun…

a3f57cb

…ction_stubs Fixes potential integer-overflow/invalid-length issues in gdv_string_function_stubs.cc. Expanded unit-tests. Incorporated review comments.

abtom87 force-pushed the fix-overflow-in-string-func branch from e7ff043 to be80053 Compare April 23, 2026 18:48

apacheGH-49753: [C++][Gandiva] Fix string based functions in string_ops.

5713446

Fixes potential integer-overflow/invalid-length issues in string_ops.cc. Expanded unit-tests for concat_ws*. Incorporated review comments.

abtom87 force-pushed the fix-overflow-in-string-func branch from be80053 to 5713446 Compare April 23, 2026 18:56

abtom87 commented Apr 23, 2026

View reviewed changes

abtom87 requested a review from dmitry-chirkov-dremio April 24, 2026 04:44

	// If there are no multibytes in the input, work with std::strings
	// If there are multibytes in the input, work with std::strings

Conversation

abtom87 commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abtom87 Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abtom87 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

abtom87 commented Apr 20, 2026 •

edited

Loading

abtom87 Apr 23, 2026 •

edited

Loading