Copilot commented on code in PR #49780:
URL: https://github.com/apache/arrow/pull/49780#discussion_r3104923285
##########
cpp/src/gandiva/hash_utils.cc:
##########
@@ -102,7 +101,7 @@ const char* gdv_hash_using_openssl(int64_t context, const
void* message,
unsigned int result_length;
EVP_DigestFinal_ex(md_ctx, result, &result_length);
Review Comment:
The return value of `EVP_DigestFinal_ex` is not checked. On failure it can
leave `result_length` unset/unchanged, and the subsequent validation/loop can
read past `result` (allocated to `hash_digest_size`) and potentially write out
of bounds. Capture and validate the return status (and set `result_length = 0`
before calling) and error out if finalization fails.
```suggestion
unsigned int result_length = 0;
if (EVP_DigestFinal_ex(md_ctx, result, &result_length) !=
evp_success_status) {
gdv_fn_context_set_error_msg(context,
"Could not obtain the hash for the defined
value");
EVP_MD_CTX_free(md_ctx);
OPENSSL_free(result);
*out_length = 0;
return "";
}
```
##########
cpp/src/gandiva/hash_utils.h:
##########
@@ -37,7 +37,6 @@ GANDIVA_EXPORT
const char* gdv_sha1_hash(int64_t context, const void* message, size_t
message_length,
int32_t* out_length);
-GANDIVA_EXPORT
const char* gdv_hash_using_openssl(int64_t context, const void* message,
size_t message_length, const EVP_MD*
hash_type,
uint32_t result_buf_size, int32_t*
out_length);
Review Comment:
`gdv_hash_using_openssl` is still declared in the installed public header
but is no longer exported. That leaves a callable declaration that will
typically fail to link for downstream users when building shared libs (and it
also forces exposing OpenSSL types via a public header). If this helper is
truly internal, remove its declaration from `hash_utils.h` and keep it
`static`/in an unnamed namespace in `hash_utils.cc` (or move it to a
non-installed internal header).
##########
cpp/src/gandiva/hash_utils_test.cc:
##########
@@ -20,6 +20,7 @@
#include "gandiva/execution_context.h"
#include "gandiva/hash_utils.h"
+#include "openssl/evp.h"
Review Comment:
`hash_utils.h` already includes `<openssl/evp.h>`, and this test file
doesn’t reference any OpenSSL APIs directly. This extra include is redundant
and adds an unnecessary dependency/compile cost; consider removing it.
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
