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

[Arawoof06]

Rationale for this change

set_error_for_date formats the invalid-date/timestamp message with snprintf(error, size, "%s%s", msg, input), but input is a Gandiva string passed as pointer plus length and is not NUL-terminated, so the %s conversion scans past the length bytes looking for a terminator and over-reads the values buffer (past its end for the last value). It is reached from castDATE_utf8/castTIMESTAMP_utf8 on any invalid CAST(str AS DATE/TIMESTAMP) input. Switching to the bounded %.*s form with the explicit length keeps the read inside the string, matching every other error formatter in the precompiled sources.

What changes are included in this PR?

Use %.*s with length in set_error_for_date, which covers all of its call sites in both castDATE_utf8 and castTIMESTAMP_utf8.

Are these changes tested?

Yes. Added TestTime.TestCastDateInvalidUnterminated, which feeds an invalid date held in an exactly-sized heap buffer with no trailing NUL so the over-read trips AddressSanitizer, and checks the error string is bounded to the input length.

Are there any user-facing changes?

No.

This PR contains a "Critical Fix". It fixes a heap out-of-bounds read reachable from user-supplied CAST-to-date/timestamp input.

• GitHub Issue: [C++][Gandiva] Out-of-bounds read when formatting invalid date/timestamp error messages #50440

[github-actions]

⚠️ GitHub issue #50440 has been automatically assigned in GitHub to PR creator.

[kou]

@dmitry-chirkov-dremio @lriggs @akravchukdremio @xxlaykxx Could you review this?

[dmitry-chirkov-dremio]

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]

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.

[dmitry-chirkov-dremio]

LGTM

[kou]

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]

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.

[dmitry-chirkov-dremio]

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 ¯_(ツ)_/¯

[kou]

Oh, sorry. snprintf(size) truncates the error message without %.*s.

[Arawoof06]

Good catch, you're right on both counts. Restored the exactly-sized non-NUL-terminated heap buffers for all three tests so the over-read actually reaches ASAN and the test fails without the fix. Kept 197201234 as the date value per the distinct-digits suggestion.

[dmitry-chirkov-dremio]

@kou could you confirm whether Gandiva tests run under ASAN in CI (looks like they don't) and whether we should enable ARROW_GANDIVA in the sanitizer job so these tests catch the original over-read?

As is PR looks good to me and I'm willing to approve.

[kou]

Our CI job did it: https://github.com/apache/arrow/actions/runs/29475467957/job/87547412304?pr=50441#step:8:4429

We can check it by reverting the fix temporary.