This is an automated email from the ASF dual-hosted git repository.
apitrou pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new b763e226d0 GH-43221: [C++][Parquet] Refactor
parquet::encryption::AesEncryptor to use unique_ptr (#43222)
b763e226d0 is described below
commit b763e226d098a369fe02e11cc225c67dd860991a
Author: mwish <[email protected]>
AuthorDate: Mon Jul 22 18:54:46 2024 +0800
GH-43221: [C++][Parquet] Refactor parquet::encryption::AesEncryptor to use
unique_ptr (#43222)
### Rationale for this change
See https://github.com/apache/arrow/issues/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]>
---
cpp/src/parquet/encryption/encryption_internal.cc | 15 ++++++---------
cpp/src/parquet/encryption/encryption_internal.h | 9 ++++-----
.../parquet/encryption/encryption_internal_nossl.cc | 12 +++++++-----
cpp/src/parquet/encryption/internal_file_decryptor.cc | 8 ++++----
cpp/src/parquet/encryption/internal_file_encryptor.cc | 19 ++++++++++++-------
cpp/src/parquet/encryption/internal_file_encryptor.h | 4 +---
cpp/src/parquet/metadata.cc | 7 +++----
7 files changed, 37 insertions(+), 37 deletions(-)
diff --git a/cpp/src/parquet/encryption/encryption_internal.cc
b/cpp/src/parquet/encryption/encryption_internal.cc
index 6168dd2a9b..99d1707f4a 100644
--- 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
index a9a17f1ab9..c874b137ad 100644
--- 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
index 2f6cdc8200..2cce83915d 100644
--- 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
index a900a4d2eb..fae5ce1f7a 100644
--- 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
index a423cc678c..285c2100be 100644
--- 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
index 41ffc6fd51..91b6e9fe5a 100644
--- 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
index 4ea3b05340..ee83918189 100644
--- 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);
