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 4 commits into
base: main
Choose a base branch
from
+55 −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 1 commit
Commits
Show all changes
4 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
aac25da
Restore exactly-sized unterminated buffers in cast error tests
Arawoof06 Jul 16, 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
22 changes: 22 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,10 @@
#include <gmock/gmock.h>
#include <gtest/gtest.h>

#include <cstring>
#include <memory>
#include <string>

#include "arrow/util/logging_internal.h"
#include "gandiva/execution_context.h"
#include "gandiva/precompiled/testing.h"
Expand Down Expand Up @@ -59,6 +63,24 @@ 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. Hold
// the input in an exactly-sized heap buffer with no trailing NUL so that
// formatting the error must respect `length` instead of scanning for a NUL;
// any over-read past the buffer trips AddressSanitizer.
const char bytes[] = {'1', '9', '7', '2', '2', '2', '2', '2', '2', '2'};
const auto length = static_cast<int32_t>(sizeof(bytes));
std::unique_ptr<char[]> input(new char[length]);
std::memcpy(input.get(), bytes, length);

EXPECT_EQ(castDATE_utf8(context_ptr, input.get(), length), 0);

@kou kou Jul 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we simplify this? I think that this will include X in the error message without this fix.

Suggested change
// The invalid-value error message is built from the raw input pointer. Hold
// the input in an exactly-sized heap buffer with no trailing NUL so that
// formatting the error must respect `length` instead of scanning for a NUL;
// any over-read past the buffer trips AddressSanitizer.
const char bytes[] = {'1', '9', '7', '2', '2', '2', '2', '2', '2', '2'};
const auto length = static_cast<int32_t>(sizeof(bytes));
std::unique_ptr<char[]> input(new char[length]);
std::memcpy(input.get(), bytes, length);
EXPECT_EQ(castDATE_utf8(context_ptr, input.get(), length), 0);
const std::string input = "1972222222X";
EXPECT_EQ(castDATE_utf8(context_ptr, input.get(), input.length() - 1), 0);

BTW, can we use different numbers instead of multiple 2 something like 197201234X.

@Arawoof06 Arawoof06 Jul 15, 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.

Good call, that's cleaner. Switched all three to a std::string with a trailing sentinel and pass length - 1, so the over-read shows up as the sentinel leaking into the message even without ASAN. Used 197201234X for the date one per your suggestion.

EXPECT_EQ(context.get_error(), "Not a valid date value 1972222222");
context.Reset();
}

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