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 22a2b51f76 GH-50140: [C++][Gandiva] Fix castVARCHAR(decimal128) native
memory corruption / SIGSEGV on allocation failure (#50141)
22a2b51f76 is described below
commit 22a2b51f763a297829abf57a68f8e788ceda4b59
Author: Logan Riggs <[email protected]>
AuthorDate: Tue Aug 4 15:07:25 2026 -0700
GH-50140: [C++][Gandiva] Fix castVARCHAR(decimal128) native memory
corruption / SIGSEGV on allocation failure (#50141)
### Rationale for this change
The Gandiva `castVARCHAR_decimal128_int64` path could corrupt native memory
and
crash the process (SIGSEGV) when the output-string arena allocation failed
(e.g. `CAST(decimal AS VARCHAR)` under memory pressure). Three independent
defects combined to cause this:
1. The `castVARCHAR` decimal128 registry entry was missing
`NativeFunction::kCanReturnErrors`, so generated code skipped the error
check
and ignored any error the function reported.
2. `gdv_fn_dec_to_string` set the output length to a positive value *before*
checking whether the allocation succeeded, then returned `nullptr` —
leaving
the caller to copy from an invalid buffer with a positive length.
3. `castVARCHAR_decimal128_int64` did not validate a negative requested
output
length and did not handle an upstream allocation failure.
### What changes are included in this PR?
- **`function_registry_string.cc`**: Add `NativeFunction::kCanReturnErrors`
to the
`castVARCHAR` `decimal128` entry so the generated code checks for and
propagates errors instead of assuming the function never fails.
- **`gdv_function_stubs.cc`** (`gdv_fn_dec_to_string`): Only write the
output
length *after* a successful allocation. On allocation failure, set
`*dec_str_len = 0` and return an empty string so callers never copy from
an
invalid buffer using a stale, positive length.
- **`precompiled/decimal_wrapper.cc`** (`castVARCHAR_decimal128_int64`):
- Reject a negative output length with a graceful error
(`"Output buffer length can't be negative"`) instead of using it as a
copy
size.
- Bail out safely (zero length, empty string) if the upstream
`gdv_fn_dec_to_string` call failed, since the error has already been
set.
- **`tests/decimal_test.cc`**: Add `TestCastVarCharDecimalNegativeLength`, a
regression test that casts a decimal to varchar with a negative output
length
and asserts the query fails gracefully with the expected error message
rather
than crashing. This also exercises the `kCanReturnErrors` flag — without
it the
error would not propagate and the test would fail.
### Behavior change
Queries such as `CAST(decimal AS VARCHAR)` that previously crashed the
process
(SIGSEGV) under memory pressure now fail gracefully with an error message
about
the allocation failure / invalid length, and the rest of the system is
unaffected.
### Are these changes tested?
Yes, unit tests.
### Are there any user-facing changes?
No.
* GitHub Issue: #50140
Authored-by: [email protected] <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
---
cpp/src/gandiva/function_registry_string.cc | 2 +-
cpp/src/gandiva/gdv_function_stubs.cc | 11 +++++---
cpp/src/gandiva/precompiled/decimal_wrapper.cc | 16 ++++++++++--
cpp/src/gandiva/tests/decimal_test.cc | 35 ++++++++++++++++++++++++++
4 files changed, 58 insertions(+), 6 deletions(-)
diff --git a/cpp/src/gandiva/function_registry_string.cc
b/cpp/src/gandiva/function_registry_string.cc
index be57ce4f47..bce317ade8 100644
--- a/cpp/src/gandiva/function_registry_string.cc
+++ b/cpp/src/gandiva/function_registry_string.cc
@@ -199,7 +199,7 @@ std::vector<NativeFunction> GetStringFunctionRegistry() {
NativeFunction("castVARCHAR", {"varchar"}, DataTypeVector{decimal128(),
int64()},
utf8(), kResultNullIfNull, "castVARCHAR_decimal128_int64",
- NativeFunction::kNeedsContext),
+ NativeFunction::kNeedsContext |
NativeFunction::kCanReturnErrors),
NativeFunction("crc32", {}, DataTypeVector{utf8()}, int64(),
kResultNullIfNull,
"gdv_fn_crc_32_utf8", NativeFunction::kNeedsContext),
diff --git a/cpp/src/gandiva/gdv_function_stubs.cc
b/cpp/src/gandiva/gdv_function_stubs.cc
index 94c5454cf2..1b359524c2 100644
--- a/cpp/src/gandiva/gdv_function_stubs.cc
+++ b/cpp/src/gandiva/gdv_function_stubs.cc
@@ -222,14 +222,19 @@ char* gdv_fn_dec_to_string(int64_t context, int64_t
x_high, uint64_t x_low,
int32_t x_scale, int32_t* dec_str_len) {
arrow::Decimal128 dec(arrow::BasicDecimal128(x_high, x_low));
std::string dec_str = dec.ToString(x_scale);
- *dec_str_len = static_cast<int32_t>(dec_str.length());
- char* ret = reinterpret_cast<char*>(gdv_fn_context_arena_malloc(context,
*dec_str_len));
+ auto dec_str_length = static_cast<int32_t>(dec_str.length());
+ char* ret =
+ reinterpret_cast<char*>(gdv_fn_context_arena_malloc(context,
dec_str_length));
if (ret == nullptr) {
std::string err_msg = "Could not allocate memory for string: " + dec_str;
gdv_fn_context_set_error_msg(context, err_msg.data());
+ // Report zero length so a caller can never combine a positive length with
the
+ // null buffer (the original bug: memcpy(dst, nullptr, positive_len) ->
SIGSEGV).
+ *dec_str_len = 0;
return nullptr;
}
- memcpy(ret, dec_str.data(), *dec_str_len);
+ *dec_str_len = dec_str_length;
+ memcpy(ret, dec_str.data(), dec_str_length);
return ret;
}
diff --git a/cpp/src/gandiva/precompiled/decimal_wrapper.cc
b/cpp/src/gandiva/precompiled/decimal_wrapper.cc
index cffb7ae978..f232f35e4e 100644
--- a/cpp/src/gandiva/precompiled/decimal_wrapper.cc
+++ b/cpp/src/gandiva/precompiled/decimal_wrapper.cc
@@ -423,11 +423,23 @@ FORCE_INLINE
char* castVARCHAR_decimal128_int64(int64_t context, int64_t x_high, uint64_t
x_low,
int32_t x_precision, int32_t x_scale,
int64_t out_len_param, int32_t* out_length)
{
+ if (out_len_param < 0) {
+ gdv_fn_context_set_error_msg(context, "Output buffer length can't be
negative");
+ *out_length = 0;
+ return const_cast<char*>("");
+ }
int32_t full_dec_str_len;
char* dec_str =
gdv_fn_dec_to_string(context, x_high, x_low, x_scale, &full_dec_str_len);
- int32_t trunc_dec_str_len =
- out_len_param < full_dec_str_len ? out_len_param : full_dec_str_len;
+ if (dec_str == nullptr) {
+ // Allocation failed upstream; error message is already set. Avoid copying
from
+ // an invalid buffer with a non-zero length.
+ *out_length = 0;
+ return const_cast<char*>("");
+ }
+ int32_t trunc_dec_str_len = out_len_param < full_dec_str_len
+ ? static_cast<int32_t>(out_len_param)
+ : full_dec_str_len;
*out_length = trunc_dec_str_len;
return dec_str;
}
diff --git a/cpp/src/gandiva/tests/decimal_test.cc
b/cpp/src/gandiva/tests/decimal_test.cc
index f8d049fd80..043bdc4605 100644
--- a/cpp/src/gandiva/tests/decimal_test.cc
+++ b/cpp/src/gandiva/tests/decimal_test.cc
@@ -976,6 +976,41 @@ TEST_F(TestDecimal, TestCastVarCharDecimal) {
EXPECT_ARROW_ARRAY_EQUALS(exp, outputs[1]);
}
+// Regression test for GH-50140: castVARCHAR(decimal) must fail gracefully
instead
+// of corrupting native memory (SIGSEGV) when given an invalid output length.
+TEST_F(TestDecimal, TestCastVarCharDecimalNegativeLength) {
+ constexpr int32_t precision = 38;
+ constexpr int32_t scale = 2;
+ auto decimal_type = std::make_shared<arrow::Decimal128Type>(precision,
scale);
+
+ auto field_dec = field("dec", decimal_type);
+ auto schema = arrow::schema({field_dec});
+ auto field_res_str = field("res_str", utf8());
+
+ auto node_dec = TreeExprBuilder::MakeField(field_dec);
+ // A negative output length must not be used as a memcpy size.
+ auto neg_len = TreeExprBuilder::MakeLiteral(static_cast<int64_t>(-1));
+ auto cast_varchar =
+ TreeExprBuilder::MakeFunction("castVARCHAR", {node_dec, neg_len},
utf8());
+ auto expr = TreeExprBuilder::MakeExpression(cast_varchar, field_res_str);
+
+ std::shared_ptr<Projector> projector;
+ auto status = Projector::Make(schema, {expr}, TestConfiguration(),
&projector);
+ EXPECT_TRUE(status.ok()) << status.message();
+
+ auto array_dec =
+ MakeArrowArrayDecimal(decimal_type, MakeDecimalVector({"10.51"}, scale),
{true});
+ auto in_batch = arrow::RecordBatch::Make(schema, 1, {array_dec});
+
+ arrow::ArrayVector outputs;
+ status = projector->Evaluate(*in_batch, pool_, &outputs);
+ // The evaluation should report a graceful error rather than crash.
+ EXPECT_FALSE(status.ok()) << status.message();
+ EXPECT_NE(status.message().find("Output buffer length can't be negative"),
+ std::string::npos)
+ << status.message();
+}
+
TEST_F(TestDecimal, TestCastDecimalVarChar) {
// schema for input fields
constexpr int32_t precision = 4;
