Skip to content

GH-50075: [C++][Gandiva] fix buffer overrun in to_hex int32/int64#50076

Merged
kou merged 1 commit into
apache:mainfrom
metsw24-max:gandiva-to-hex-buffer-overrun
Jun 26, 2026
Merged

GH-50075: [C++][Gandiva] fix buffer overrun in to_hex int32/int64#50076
kou merged 1 commit into
apache:mainfrom
metsw24-max:gandiva-to-hex-buffer-overrun

Conversation

@metsw24-max

@metsw24-max metsw24-max commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

to_hex_int64 and to_hex_int32 in cpp/src/gandiva/precompiled/string_ops.cc allocate the arena buffer with the bare hex-digit count (16 and 8 bytes) but pass that size + 1 as the snprintf bound. For any full-width value (to_hex(-1::bigint) -> FFFFFFFFFFFFFFFF, INT64_MIN, INT32_MIN, ...) snprintf writes all digits plus the trailing NUL, one byte past the end of the allocation. gdv_fn_context_arena_malloc returns exactly the requested bytes, so the NUL corrupts the adjacent arena allocation. Reachable from the to_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 snprintf bound, the same fix as GH-49752 applied to the identical pattern in hash_utils.cc.

Are these changes tested?

Yes. The existing TestToHexInt64 / TestToHexInt32 cases 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_hex Gandiva functions over untrusted input.

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown

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

@kou

kou commented Jun 9, 2026

Copy link
Copy Markdown
Member

Could you use our PR template instead of removing it entirely?

@kou kou marked this pull request as draft June 9, 2026 21:44
@metsw24-max

Copy link
Copy Markdown
Contributor Author

Done, restored the template and filled in the sections. Sorry, I'd trimmed it out when opening the PR.

@metsw24-max metsw24-max marked this pull request as ready for review June 23, 2026 11:36
@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jun 25, 2026

@kou kou left a comment

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.

+1

@kou kou merged commit d091957 into apache:main Jun 26, 2026
55 of 56 checks passed
@kou kou removed the awaiting committer review Awaiting committer review label Jun 26, 2026
@github-actions github-actions Bot added the awaiting merge Awaiting merge label Jun 26, 2026
@conbench-apache-arrow

Copy link
Copy Markdown

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants