adamreeve commented on code in PR #43222:
URL: https://github.com/apache/arrow/pull/43222#discussion_r1674805803
##########
cpp/src/parquet/metadata.cc:
##########
@@ -649,9 +649,12 @@ class FileMetaData::FileMetaDataImpl {
std::string key = file_decryptor_->GetFooterKey();
std::string aad = encryption::CreateFooterAad(file_decryptor_->file_aad());
- auto aes_encryptor = encryption::AesEncryptor::Make(
- file_decryptor_->algorithm(), static_cast<int>(key.size()), true,
- false /*write_length*/, nullptr);
+ auto aes_encryptor =
encryption::AesEncryptor::Make(file_decryptor_->algorithm(),
+
static_cast<int>(key.size()),
+ true, false
/*write_length*/);
+ if (ARROW_PREDICT_FALSE(aes_encryptor == nullptr)) {
+ throw ParquetException("Could not create AES encryptor for signature
verification");
Review Comment:
Yes I think this should be checked and I agree an exception is nicer than
abort.
It looks like other places that construct an AesEncryptor don't check for
null either though, eg. the encryptors returned from
`InternalFileEncryptor::GetMetaAesEncryptor` and
`InternalFileEncryptor::GetDataAesEncryptor` appear to be used without checking
for null. Maybe it would be better to change `Make` to return
`Result<std::unique_ptr<AesEncryptor>>` to enforce checking for failure, or
possibly it makes sense to just change the `_nossl` versions of `Make` to throw
an exception rather than return null?
`AesDecryptor::Make` also seems to have the same problem.
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
