Skip to content

GH-49752: [C++][Gandiva] Fix potential buffer overrun in gandiva ssl function#49780

Open
lriggs wants to merge 7 commits intoapache:mainfrom
lriggs:GH-49752
Open

GH-49752: [C++][Gandiva] Fix potential buffer overrun in gandiva ssl function#49780
lriggs wants to merge 7 commits intoapache:mainfrom
lriggs:GH-49752

Conversation

@lriggs
Copy link
Copy Markdown
Contributor

@lriggs lriggs commented Apr 16, 2026

Rationale for this change

Fixes security related problems found in gdv_hash_using_openssl. Those problems were not deemed to be a security risk.

What changes are included in this PR?

[hash_utils.h:41, hash_utils.cc:66] Removed GANDIVA_EXPORT from gdv_hash_using_openssl — it's an internal helper, not part of the public API.

[hash_utils.cc:105] Changed && → || in the validation condition. The original only errored when both checks failed; now it errors when either result_length != hash_digest_size or result_buf_size != (2 * hash_digest_size).

[hash_utils.cc:135] Fixed snprintf buffer size, so it correctly accounts for the already-written bytes and prevents potential out-of-bounds writes. Allocate result_buf_size + 1 bytes — the extra byte absorbs the final null terminator. Pass result_buf_size - result_buff_index + 1 to snprintf — reflects the actual remaining space (2 hex chars + 1 null = 3 bytes on the last call), preventing any potential overflow if the format ever changed.

Are these changes tested?

Yes, unit tests.

Are there any user-facing changes?

No.

Comment thread cpp/src/gandiva/hash_utils_test.cc Outdated
@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Apr 16, 2026
@kou kou changed the title GH-49752: Fix potential buffer overrun in gandiva ssl function. GH-49752: [C++][Gandiva] Fix potential buffer overrun in gandiva ssl function Apr 18, 2026
@kou kou requested a review from Copilot April 18, 2026 09:13
Comment thread cpp/src/gandiva/hash_utils_test.cc Outdated
Comment thread cpp/src/gandiva/hash_utils.cc
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Apr 18, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses correctness/safety issues in Gandiva’s OpenSSL-based hashing helper (gdv_hash_using_openssl) and reduces its intended API exposure.

Changes:

  • Stops exporting gdv_hash_using_openssl (intended to be internal).
  • Fixes validation logic so mismatched digest size or result buffer size triggers an error.
  • Adjusts result-buffer allocation and snprintf sizing to avoid potential out-of-bounds writes when hex-encoding the digest.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
cpp/src/gandiva/hash_utils_test.cc Adds an OpenSSL header include in the hash utils unit tests.
cpp/src/gandiva/hash_utils.h Removes GANDIVA_EXPORT from gdv_hash_using_openssl declaration.
cpp/src/gandiva/hash_utils.cc Fixes validation condition and tightens result buffer allocation / snprintf bounds.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cpp/src/gandiva/hash_utils.h Outdated
Comment thread cpp/src/gandiva/hash_utils.cc
Comment thread cpp/src/gandiva/hash_utils_test.cc Outdated
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 18, 2026
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.

4 participants