Uh oh!
There was an error while loading. Please reload this page.
GH-49752: [C++][Gandiva] Fix potential buffer overrun in Gandiva SSL function - #49780
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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
snprintfsizing 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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
lriggs
commented
Jun 1, 2026
@kou I think this is ready for review. Im not sure why its labelled awaiting change, I think I addressed and resolved them all. |
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit d586ed1. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them. |
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.