GH-50075: [C++][Gandiva] fix buffer overrun in to_hex int32/int64#50076
Merged
Conversation
|
|
Member
|
Could you use our PR template instead of removing it entirely? |
Contributor
Author
|
Done, restored the template and filled in the sections. Sorry, I'd trimmed it out when opening the PR. |
lriggs
approved these changes
Jun 25, 2026
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit d091957. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
to_hex_int64andto_hex_int32incpp/src/gandiva/precompiled/string_ops.ccallocate the arena buffer with the bare hex-digit count (16 and 8 bytes) but pass that size + 1 as thesnprintfbound. For any full-width value (to_hex(-1::bigint)->FFFFFFFFFFFFFFFF,INT64_MIN,INT32_MIN, ...)snprintfwrites all digits plus the trailing NUL, one byte past the end of the allocation.gdv_fn_context_arena_mallocreturns exactly the requested bytes, so the NUL corrupts the adjacent arena allocation. Reachable from theto_hex(bigint)/to_hex(int)Gandiva functions over untrusted column data.What changes are included in this PR?
Allocate one extra byte in both functions so the allocation matches the
snprintfbound, the same fix as GH-49752 applied to the identical pattern inhash_utils.cc.Are these changes tested?
Yes. The existing
TestToHexInt64/TestToHexInt32cases already exercise the full-width path (INT64_MIN,INT32_MIN, -1) and run clean under ASAN with this change. Output behaviour is unchanged.Are there any user-facing changes?
No.
This PR contains a "Critical Fix". It fixes a one-byte out-of-bounds heap write reachable from the
to_hexGandiva functions over untrusted input.