EnricoMi commented on code in PR #44990:
URL: https://github.com/apache/arrow/pull/44990#discussion_r2009855471
##########
cpp/src/parquet/encryption/internal_file_decryptor.cc:
##########
@@ -64,17 +66,20 @@
InternalFileDecryptor::InternalFileDecryptor(FileDecryptionProperties* propertie
properties_->set_utilized();
}
+InternalFileDecryptor::~InternalFileDecryptor() { WipeOutDecryptionKeys(); }
+
void InternalFileDecryptor::WipeOutDecryptionKeys() {
- std::lock_guard<std::mutex> lock(mutex_);
+ std::unique_lock lock(mutex_);
properties_->WipeOutDecryptionKeys();
- for (auto const& i : all_decryptors_) {
- if (auto aes_decryptor = i.lock()) {
- aes_decryptor->WipeOut();
- }
- }
+ footer_key_.clear();
Review Comment:
@pitrou why do we clear the footer key? Do we not trust the string
destructor to wipe memory before releasing it?
The same reason probably why we wipe out keys in `properties`.
##########
cpp/src/parquet/encryption/internal_file_decryptor.cc:
##########
@@ -64,17 +66,20 @@
InternalFileDecryptor::InternalFileDecryptor(FileDecryptionProperties* propertie
properties_->set_utilized();
}
+InternalFileDecryptor::~InternalFileDecryptor() { WipeOutDecryptionKeys(); }
+
void InternalFileDecryptor::WipeOutDecryptionKeys() {
- std::lock_guard<std::mutex> lock(mutex_);
+ std::unique_lock lock(mutex_);
Review Comment:
This method is only called from the destructor, so there is no need for a
lock, as its guaranteed to be called only once.
##########
cpp/src/parquet/encryption/internal_file_decryptor.cc:
##########
@@ -64,17 +66,20 @@
InternalFileDecryptor::InternalFileDecryptor(FileDecryptionProperties* propertie
properties_->set_utilized();
}
+InternalFileDecryptor::~InternalFileDecryptor() { WipeOutDecryptionKeys(); }
+
void InternalFileDecryptor::WipeOutDecryptionKeys() {
- std::lock_guard<std::mutex> lock(mutex_);
+ std::unique_lock lock(mutex_);
properties_->WipeOutDecryptionKeys();
Review Comment:
@pitrou Variable `properties` is shared across decryptor instances, wiping
out keys mutates `properties` for other decryptors. Firstly, we revoke keys for
other decryptors. Secondly, multiple decryptors might call
`WipeOutDecryptionKeys` concurrently causing issues. A lock here does not help
as individual decryptors have individual locks.
The keys should be wiped out in the destructor of `properties`, so they
exist for the lifetime of `properties`.
--
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]
