GH-49752: [C++][Gandiva] Fix potential buffer overrun in Gandiva SSL function#49780
Conversation
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.
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.
|
@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.