Skip to content

GH-50440: [C++][Gandiva] fix out-of-bounds read in set_error_for_date #50441

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Arawoof06 wants to merge 3 commits into
base: main
Choose a base branch
from
+47 −1
Open

GH-50440: [C++][Gandiva] fix out-of-bounds read in set_error_for_date #50441

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a393675
GH-50440: [C++][Gandiva] fix out-of-bounds read in set_error_for_date
Arawoof06 Jul 9, 2026
0f5c3eb
GH-50440: [C++][Gandiva] cover timestamp and time cast error paths
Arawoof06 Jul 14, 2026
d7b6264
Simplify invalid-value tests with bounded input
Arawoof06 Jul 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cpp/src/gandiva/precompiled/time.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,7 +631,7 @@ void set_error_for_date(gdv_int32 length, const char* input, const char* msg,
int64_t execution_context) {
int size = length + static_cast<int>(strlen(msg)) + 1;
char* error = reinterpret_cast<char*>(malloc(size));
snprintf(error, size, "%s%s", msg, input);
snprintf(error, size, "%s%.*s", msg, length, input);
gdv_fn_context_set_error_msg(execution_context, error);
free(error);
}
Expand Down
46 changes: 46 additions & 0 deletions cpp/src/gandiva/precompiled/time_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
#include <gmock/gmock.h>
#include <gtest/gtest.h>

#include <string>

#include "arrow/util/logging_internal.h"
#include "gandiva/execution_context.h"
#include "gandiva/precompiled/testing.h"
Expand Down Expand Up @@ -59,6 +61,50 @@ TEST(TestTime, TestCastDate) {
EXPECT_EQ(castDATE_date32(1), 86400000);
}

TEST(TestTime, TestCastDateInvalidUnterminated) {

@dmitry-chirkov-dremio dmitry-chirkov-dremio Jul 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that time and timestamp are going through the same code path. Is this so?
If yes, would there be value in adding two more tests for those data types?

p.s. rest of PR looks good to me just need an evaluation of value of additional tests as helper isn't really data type specific.

@Arawoof06 Arawoof06 Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, both castTIMESTAMP_utf8 and castTIME_utf8 hit the same set_error_for_date helper, so the fix already covers them. Added a test for each anyway (2000-01-01 24:00:00 for timestamp, 24H00H00 for time) so the over-read path is exercised from all three entry points with an unterminated buffer.

ExecutionContext context;
int64_t context_ptr = reinterpret_cast<int64_t>(&context);

// The invalid-value error message is built from the raw input pointer. Pass a
// length that stops one byte short of the trailing sentinel so the formatter
// must respect `length` rather than scanning for a NUL; without the fix the
// sentinel leaks into the error message.
const std::string input = "197201234X";

@dmitry-chirkov-dremio dmitry-chirkov-dremio Jul 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we keep the exactly-sized, non-NUL-terminated buffer here and in the timestamp/time tests?

With std::string, both the sentinel and trailing NUL are accessible, so ASAN won't detect an out-of-bounds read. Also, the old formatter sized its destination using length, so although %s reads through the sentinel, snprintf truncates it from the stored message. In other words, the old vulnerable implementation still produces the expected "Not a valid date value 197201234" string, and this test passes without the fix.

Please restore the exactly-sized unterminated buffers from the previous commit, or use an equivalent guarded allocation, for all three entry points.

p.s. In general it's a good local development practice to stash all fixes and just run the new tests to see how they behave - with agentic coding I started to use that approach again on pretty much everything I code ¯_(ツ)_/¯

EXPECT_EQ(castDATE_utf8(context_ptr, input.data(),
static_cast<int32_t>(input.length()) - 1),
0);
EXPECT_EQ(context.get_error(), "Not a valid date value 197201234");
context.Reset();
}

TEST(TestTime, TestCastTimestampInvalidUnterminated) {
ExecutionContext context;
int64_t context_ptr = reinterpret_cast<int64_t>(&context);

// castTIMESTAMP_utf8 reaches the same set_error_for_date helper, so exercise
// the over-read path from that entry point too.
const std::string input = "2000-01-01 24:00:00X";
EXPECT_EQ(castTIMESTAMP_utf8(context_ptr, input.data(),
static_cast<int32_t>(input.length()) - 1),
0);
EXPECT_EQ(context.get_error(),
"Not a valid time for timestamp value 2000-01-01 24:00:00");
context.Reset();
}

TEST(TestTime, TestCastTimeInvalidUnterminated) {
ExecutionContext context;
int64_t context_ptr = reinterpret_cast<int64_t>(&context);

// castTIME_utf8 reaches set_error_for_date as well.
const std::string input = "24H00H00X";
EXPECT_EQ(castTIME_utf8(context_ptr, input.data(),
static_cast<int32_t>(input.length()) - 1),
0);
EXPECT_EQ(context.get_error(), "Invalid character in time 24H00H00");
context.Reset();
}

TEST(TestTime, TestCastTimestamp) {
ExecutionContext context;
int64_t context_ptr = reinterpret_cast<int64_t>(&context);
Expand Down
Loading