GH-43221: [C++][Parquet] Refactor parquet::encryption::AesEncryptor t…

…o use unique_ptr (#43222) ### Rationale for this change See #43221 ### What changes are included in this PR? Change raw-pointer to unique_ptr ### Are these changes tested? Covered by existing ### Are there any user-facing changes? Maybe change user interface * GitHub Issue: #43221 Authored-by: mwish <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
apache · Jul 22, 2024 · b763e22 · b763e22
1 parent 05ab846
commit b763e22
Show file tree

Hide file tree

Showing 7 changed files with 37 additions and 37 deletions.
diff --git a/cpp/src/parquet/encryption/encryption_internal.cc b/cpp/src/parquet/encryption/encryption_internal.cc
@@ -469,23 +469,20 @@ AesDecryptor::AesDecryptorImpl::AesDecryptorImpl(ParquetCipher::type alg_id, int
   }
 }
 
-AesEncryptor* AesEncryptor::Make(ParquetCipher::type alg_id, int key_len, bool metadata,
-                                 std::vector<AesEncryptor*>* all_encryptors) {
-  return Make(alg_id, key_len, metadata, true /*write_length*/, all_encryptors);
+std::unique_ptr<AesEncryptor> AesEncryptor::Make(ParquetCipher::type alg_id, int key_len,
+                                                 bool metadata) {
+  return Make(alg_id, key_len, metadata, true /*write_length*/);
 }
 
-AesEncryptor* AesEncryptor::Make(ParquetCipher::type alg_id, int key_len, bool metadata,
-                                 bool write_length,
-                                 std::vector<AesEncryptor*>* all_encryptors) {
+std::unique_ptr<AesEncryptor> AesEncryptor::Make(ParquetCipher::type alg_id, int key_len,
+                                                 bool metadata, bool write_length) {
   if (ParquetCipher::AES_GCM_V1 != alg_id && ParquetCipher::AES_GCM_CTR_V1 != alg_id) {
     std::stringstream ss;
     ss << "Crypto algorithm " << alg_id << " is not supported";
     throw ParquetException(ss.str());
   }
 
-  AesEncryptor* encryptor = new AesEncryptor(alg_id, key_len, metadata, write_length);
-  if (all_encryptors != nullptr) all_encryptors->push_back(encryptor);
-  return encryptor;
+  return std::make_unique<AesEncryptor>(alg_id, key_len, metadata, write_length);
 }
 
 AesDecryptor::AesDecryptor(ParquetCipher::type alg_id, int key_len, bool metadata,

diff --git a/cpp/src/parquet/encryption/encryption_internal.h b/cpp/src/parquet/encryption/encryption_internal.h
@@ -52,12 +52,11 @@ class PARQUET_EXPORT AesEncryptor {
   explicit AesEncryptor(ParquetCipher::type alg_id, int key_len, bool metadata,
                         bool write_length = true);
 
-  static AesEncryptor* Make(ParquetCipher::type alg_id, int key_len, bool metadata,
-                            std::vector<AesEncryptor*>* all_encryptors);
+  static std::unique_ptr<AesEncryptor> Make(ParquetCipher::type alg_id, int key_len,
+                                            bool metadata);
 
-  static AesEncryptor* Make(ParquetCipher::type alg_id, int key_len, bool metadata,
-                            bool write_length,
-                            std::vector<AesEncryptor*>* all_encryptors);
+  static std::unique_ptr<AesEncryptor> Make(ParquetCipher::type alg_id, int key_len,
+                                            bool metadata, bool write_length);
 
   ~AesEncryptor();
 

diff --git a/cpp/src/parquet/encryption/encryption_internal_nossl.cc b/cpp/src/parquet/encryption/encryption_internal_nossl.cc
@@ -72,14 +72,15 @@ void AesDecryptor::WipeOut() { ThrowOpenSSLRequiredException(); }
 
 AesDecryptor::~AesDecryptor() {}
 
-AesEncryptor* AesEncryptor::Make(ParquetCipher::type alg_id, int key_len, bool metadata,
-                                 std::vector<AesEncryptor*>* all_encryptors) {
+std::unique_ptr<AesEncryptor> AesEncryptor::Make(ParquetCipher::type alg_id, int key_len,
+                                                 bool metadata) {
+  ThrowOpenSSLRequiredException();
   return NULLPTR;
 }
 
-AesEncryptor* AesEncryptor::Make(ParquetCipher::type alg_id, int key_len, bool metadata,
-                                 bool write_length,
-                                 std::vector<AesEncryptor*>* all_encryptors) {
+std::unique_ptr<AesEncryptor> AesEncryptor::Make(ParquetCipher::type alg_id, int key_len,
+                                                 bool metadata, bool write_length) {
+  ThrowOpenSSLRequiredException();
   return NULLPTR;
 }
 
@@ -91,6 +92,7 @@ AesDecryptor::AesDecryptor(ParquetCipher::type alg_id, int key_len, bool metadat
 std::shared_ptr<AesDecryptor> AesDecryptor::Make(
     ParquetCipher::type alg_id, int key_len, bool metadata,
     std::vector<std::weak_ptr<AesDecryptor>>* all_decryptors) {
+  ThrowOpenSSLRequiredException();
   return NULLPTR;
 }
 

diff --git a/cpp/src/parquet/encryption/internal_file_decryptor.cc b/cpp/src/parquet/encryption/internal_file_decryptor.cc
@@ -27,7 +27,7 @@ namespace parquet {
 Decryptor::Decryptor(std::shared_ptr<encryption::AesDecryptor> aes_decryptor,
                      const std::string& key, const std::string& file_aad,
                      const std::string& aad, ::arrow::MemoryPool* pool)
-    : aes_decryptor_(aes_decryptor),
+    : aes_decryptor_(std::move(aes_decryptor)),
       key_(key),
       file_aad_(file_aad),
       aad_(aad),
@@ -156,9 +156,9 @@ std::shared_ptr<Decryptor> InternalFileDecryptor::GetFooterDecryptor(
   }
 
   footer_metadata_decryptor_ = std::make_shared<Decryptor>(
-      aes_metadata_decryptor, footer_key, file_aad_, aad, pool_);
-  footer_data_decryptor_ =
-      std::make_shared<Decryptor>(aes_data_decryptor, footer_key, file_aad_, aad, pool_);
+      std::move(aes_metadata_decryptor), footer_key, file_aad_, aad, pool_);
+  footer_data_decryptor_ = std::make_shared<Decryptor>(std::move(aes_data_decryptor),
+                                                       footer_key, file_aad_, aad, pool_);
 
   if (metadata) return footer_metadata_decryptor_;
   return footer_data_decryptor_;

diff --git a/cpp/src/parquet/encryption/internal_file_encryptor.cc b/cpp/src/parquet/encryption/internal_file_encryptor.cc
@@ -53,8 +53,15 @@ InternalFileEncryptor::InternalFileEncryptor(FileEncryptionProperties* propertie
 void InternalFileEncryptor::WipeOutEncryptionKeys() {
   properties_->WipeOutEncryptionKeys();
 
-  for (auto const& i : all_encryptors_) {
-    i->WipeOut();
+  for (auto const& i : meta_encryptor_) {
+    if (i != nullptr) {
+      i->WipeOut();
+    }
+  }
+  for (auto const& i : data_encryptor_) {
+    if (i != nullptr) {
+      i->WipeOut();
+    }
   }
 }
 
@@ -136,7 +143,7 @@ InternalFileEncryptor::InternalFileEncryptor::GetColumnEncryptor(
   return encryptor;
 }
 
-int InternalFileEncryptor::MapKeyLenToEncryptorArrayIndex(int key_len) {
+int InternalFileEncryptor::MapKeyLenToEncryptorArrayIndex(int key_len) const {
   if (key_len == 16)
     return 0;
   else if (key_len == 24)
@@ -151,8 +158,7 @@ encryption::AesEncryptor* InternalFileEncryptor::GetMetaAesEncryptor(
   int key_len = static_cast<int>(key_size);
   int index = MapKeyLenToEncryptorArrayIndex(key_len);
   if (meta_encryptor_[index] == nullptr) {
-    meta_encryptor_[index].reset(
-        encryption::AesEncryptor::Make(algorithm, key_len, true, &all_encryptors_));
+    meta_encryptor_[index] = encryption::AesEncryptor::Make(algorithm, key_len, true);
   }
   return meta_encryptor_[index].get();
 }
@@ -162,8 +168,7 @@ encryption::AesEncryptor* InternalFileEncryptor::GetDataAesEncryptor(
   int key_len = static_cast<int>(key_size);
   int index = MapKeyLenToEncryptorArrayIndex(key_len);
   if (data_encryptor_[index] == nullptr) {
-    data_encryptor_[index].reset(
-        encryption::AesEncryptor::Make(algorithm, key_len, false, &all_encryptors_));
+    data_encryptor_[index] = encryption::AesEncryptor::Make(algorithm, key_len, false);
   }
   return data_encryptor_[index].get();
 }

diff --git a/cpp/src/parquet/encryption/internal_file_encryptor.h b/cpp/src/parquet/encryption/internal_file_encryptor.h
@@ -88,8 +88,6 @@ class InternalFileEncryptor {
   std::shared_ptr<Encryptor> footer_signing_encryptor_;
   std::shared_ptr<Encryptor> footer_encryptor_;
 
-  std::vector<encryption::AesEncryptor*> all_encryptors_;
-
   // Key must be 16, 24 or 32 bytes in length. Thus there could be up to three
   // types of meta_encryptors and data_encryptors.
   std::unique_ptr<encryption::AesEncryptor> meta_encryptor_[3];
@@ -105,7 +103,7 @@ class InternalFileEncryptor {
   encryption::AesEncryptor* GetDataAesEncryptor(ParquetCipher::type algorithm,
                                                 size_t key_len);
 
-  int MapKeyLenToEncryptorArrayIndex(int key_len);
+  int MapKeyLenToEncryptorArrayIndex(int key_len) const;
 };
 
 }  // namespace parquet
diff --git a/cpp/src/parquet/metadata.cc b/cpp/src/parquet/metadata.cc
@@ -651,9 +651,9 @@ 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*/);
 
     std::shared_ptr<Buffer> encrypted_buffer = AllocateBuffer(
         file_decryptor_->pool(), aes_encryptor->CiphertextLength(serialized_len));
@@ -662,7 +662,6 @@ class FileMetaData::FileMetaDataImpl {
         encrypted_buffer->mutable_span_as<uint8_t>());
     // Delete AES encryptor object. It was created only to verify the footer signature.
     aes_encryptor->WipeOut();
-    delete aes_encryptor;
     return 0 ==
            memcmp(encrypted_buffer->data() + encrypted_len - encryption::kGcmTagLength,
                   tag, encryption::kGcmTagLength);