dmitry-chirkov-dremio commented on code in PR #50441:
URL: https://github.com/apache/arrow/pull/50441#discussion_r3588715505
##########
cpp/src/gandiva/precompiled/time_test.cc:
##########
@@ -67,17 +65,15 @@ TEST(TestTime, TestCastDateInvalidUnterminated) {
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);
- EXPECT_EQ(context.get_error(), "Not a valid date value 1972222222");
+ // 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";
Review Comment:
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 again on pretty much everything I code ¯\_(ツ)_/¯
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
