GH-43057: [C++] Thread-safe AesEncryptor / AesDecryptor

[EnricoMi]

Rationale for this change

OpenSSL encryption / decryption is wrapped by AesEncryptor / AesDencryptor, which is used by multiple threads of a single scanner or by multiple concurrent scanners when scanning a dataset. Some thread may call WipeOut while other threads still use the instance.

What changes are included in this PR?

• Remove the WipeOut methods and related datastructures entirely.
• Each call into CtrEncrypt / CtrDecrypt and GcmEncrypt / GcmDecrypt uses its own EVP_CIPHER_CTX instance, making this thread-safe.

After fixing this "AesDecryptor was wiped out" issue, two other segmentation faults surfaced: GH-44988. This has also been addressed here as it can only be exposed after fixing the wipe-out issue.

Fixes GH-43057.
Fixes GH-44852.
Fixes GH-44988.

Are these changes tested?

A unit test that scans a dataset concurrently reproduced the initial issue in 30% of the test runs.

Are there any user-facing changes?

No.

• GitHub Issue: [CI][C++] test-ubuntu-20.04-cpp fails sometimes with a segmentation fault on arrow-dataset-file-parquet-encryption-test #43057

[adamreeve]

The test failures look like they might be caused by openssl/openssl#21955

The failing ASAN test run is using OpenSSL 3.02, but builds with other versions are passing. Eg. the Ubuntu 20.04 build uses OpenSSL 1.1.1f and the Conda build has OpenSSL 3.3.2.

The fix for this was backported to the 3.0 branch and released in 3.0.13 (openssl/openssl#23102), but Ubuntu 22.04 still uses 3.0.2. If there's not an easy way around this, maybe we need a slower code path for 3.0.2 that creates a new context each time?

[pitrou]

If there's not an easy way around this, maybe we need a slower code path for 3.0.2 that creates a new context each time?

Let's use the "slower" code path everywhere? I'm skeptical it will be that slow...

[pitrou]

Thanks a lot for stepping in and submitting this @EnricoMi . You'll find a number of comments below.

[EnricoMi]

I have addressed all comments except for the test related ones. Will go over them tomorrow.

Thanks for your input, that is highly appreciated!

[adamreeve]

I tested concurrent scans of a larger dataset with uniform encryption, and this change doesn't completely fix that scenario:

--- a/cpp/src/arrow/dataset/file_parquet_encryption_test.cc
+++ b/cpp/src/arrow/dataset/file_parquet_encryption_test.cc
@@ -289,6 +289,8 @@ TEST_F(DatasetEncryptionTest, ReadSingleFile) {
 // processing encrypted datasets over 2^15 rows in multi-threaded mode.
 class LargeRowEncryptionTest : public DatasetEncryptionTestBase {
  public:
+  LargeRowEncryptionTest() : DatasetEncryptionTestBase(true) {}
+
   // The dataset is partitioned using a Hive partitioning scheme.
   void PrepareTableAndPartitioning() override {
     // Specifically chosen to be greater than batch size for triggering prefetch.
@@ -307,7 +309,7 @@ class LargeRowEncryptionTest : public DatasetEncryptionTestBase {
 
 // Test for writing and reading encrypted dataset with large row count.
 TEST_F(LargeRowEncryptionTest, ReadEncryptLargeRows) {
-  ASSERT_NO_FATAL_FAILURE(TestScanDataset());
+  ASSERT_NO_FATAL_FAILURE(TestScanDataset(true));
 }
 
 }  // namespace dataset

This gives errors like:

failed with IOError: Failed decryption finalization

I believe this is due to the decryptor AAD being mutable. It's updated for each data page read, so concurrent use will result in incorrect AADs being used in some threads.

Rather than mutating the decryptor state, it might be possible to refactor this so that the AAD values are passed in to the decrypt method as a parameter.

I think this should probably be addressed as a separate PR though as the changes should be orthogonal.

[EnricoMi]

failed with IOError: Failed decryption finalization

Confirmed in ec9b054. Multi-threaded uniform encryption read fails.

[EnricoMi]

failed with IOError: Failed decryption finalization

Confirmed in ec9b054. Multi-threaded uniform encryption read fails.

Attempt to fix multi-threaded AAD update: d77a081

The PageReader uses its own copy of the data_decryptor / data_decryptor. Happy to move into separate PR.

[mapleFU]

Any updates?

[pitrou]

@EnricoMi Sorry for the delay and thanks a lot for persevering on this. I think the fix is still not 100% correct, see comments below. But we're nearing a solution.

Also, I can help on this PR if you want.