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

EnricoMi · 2024-12-10T11:36:51Z

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

cpp/src/parquet/encryption/encryption_internal.cc

cpp/src/parquet/encryption/internal_file_decryptor.cc

adamreeve · 2024-12-11T03:22:54Z

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 · 2024-12-11T14:17:56Z

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...

cpp/src/parquet/encryption/encryption_internal.h

pitrou

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

cpp/src/arrow/dataset/file_parquet.cc

cpp/src/arrow/dataset/file_parquet_encryption_test.cc

cpp/src/parquet/encryption/encryption_internal.h

cpp/src/parquet/encryption/encryption_internal.cc

cpp/src/parquet/encryption/internal_file_decryptor.cc

cpp/src/parquet/encryption/internal_file_encryptor.cc

EnricoMi · 2024-12-11T19:50:54Z

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 · 2024-12-11T22:18:43Z

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 · 2024-12-12T09:01:15Z

failed with IOError: Failed decryption finalization

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

EnricoMi · 2024-12-12T10:59:49Z

failed with IOError: Failed decryption finalization

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

Attempt to fix multi-threaded AAD update: d77a081a49b3436fd0bc16ce3d10792b7222e026

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

cpp/src/arrow/dataset/file_parquet_encryption_test.cc

cpp/src/parquet/column_reader.cc

cpp/src/parquet/encryption/internal_file_decryptor.cc

mapleFU · 2025-01-21T03:32:46Z

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.

cpp/src/parquet/encryption/encryption_internal.cc

cpp/src/parquet/column_reader.cc

cpp/src/arrow/dataset/file_parquet_encryption_test.cc

EnricoMi · 2025-04-01T18:38:34Z

@pitrou suggestions applied and rebased

pitrou · 2025-04-02T08:08:45Z

@github-actions crossbow submit -g cpp -g python

pitrou · 2025-04-02T08:09:02Z

@github-actions crossbow submit cp313

github-actions · 2025-04-02T08:11:40Z

Revision: a587553

Submitted crossbow builds: ursacomputing/crossbow @ actions-05dac70ebf

Task	Status
wheel-macos-monterey-cp313-cp313-amd64
wheel-macos-monterey-cp313-cp313-arm64
wheel-macos-monterey-cp313-cp313t-amd64
wheel-macos-monterey-cp313-cp313t-arm64
wheel-manylinux-2-28-cp313-cp313-amd64
wheel-manylinux-2-28-cp313-cp313-arm64
wheel-manylinux-2-28-cp313-cp313t-amd64
wheel-manylinux-2-28-cp313-cp313t-arm64
wheel-manylinux-2014-cp313-cp313-amd64
wheel-manylinux-2014-cp313-cp313-arm64
wheel-manylinux-2014-cp313-cp313t-amd64
wheel-manylinux-2014-cp313-cp313t-arm64
wheel-musllinux-1-2-cp313-cp313-amd64
wheel-musllinux-1-2-cp313-cp313-arm64
wheel-musllinux-1-2-cp313-cp313t-amd64
wheel-musllinux-1-2-cp313-cp313t-arm64
wheel-windows-cp313-cp313-amd64
wheel-windows-cp313-cp313t-amd64

github-actions · 2025-04-02T08:12:08Z

Revision: a587553

Submitted crossbow builds: ursacomputing/crossbow @ actions-34c96e7a1d

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
example-python-minimal-build-fedora-conda
example-python-minimal-build-ubuntu-venv
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-meson
test-conda-cpp-valgrind
test-conda-python-3.10
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest-numpy-latest
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-latest-numpy-1.26
test-conda-python-3.11-pandas-latest-numpy-latest
test-conda-python-3.11-pandas-nightly-numpy-nightly
test-conda-python-3.11-pandas-upstream_devel-numpy-nightly
test-conda-python-3.11-spark-master
test-conda-python-3.12
test-conda-python-3.12-cpython-debug
test-conda-python-3.13
test-conda-python-3.9
test-conda-python-3.9-pandas-1.1.3-numpy-1.19.5
test-conda-python-emscripten
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1
test-cuda-python-ubuntu-22.04-cuda-11.7.1
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-debian-12-python-3-amd64
test-debian-12-python-3-i386
test-fedora-39-cpp
test-fedora-39-python-3
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-bundled
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-22.04-python-3
test-ubuntu-22.04-python-313-freethreading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-bundled-offline
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer
test-ubuntu-24.04-python-3

pitrou · 2025-04-02T08:31:51Z

@github-actions crossbow submit -g cpp

pitrou

LGTM. Thanks a lot for this @EnricoMi !

github-actions · 2025-04-02T08:34:27Z

Revision: 9961069

Submitted crossbow builds: ursacomputing/crossbow @ actions-ebb6807bc5

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-meson
test-conda-cpp-valgrind
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-bundled
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-bundled-offline
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer

EnricoMi · 2025-04-02T08:47:22Z

@pitrou thank you for your time! Will this make it into the 20.0.0 release?

pitrou · 2025-04-02T08:50:51Z

@EnricoMi It should, I'll wait for CI and merge if green.

zanmato1984 · 2025-04-02T08:58:23Z

I think the release 20.0.0 has been feature-freezed, and the maint-20.0.0 has been branched earlier today?

EnricoMi · 2025-04-02T09:01:41Z

Can we consider this a bug fix and not a feature so it could still make it into the 20.0.0 maint branch? Or is "feature freeze" more a general "code freeze"?

pitrou · 2025-04-02T09:04:29Z

This is definitely a bug fix (it fixes an actual bug) even though it's quite elaborate and goes deeper than simply preventing nasty behavior.

zanmato1984 · 2025-04-02T09:10:53Z

Well, I mean that with feature freeze done (== maintenance branch branched out), it doesn't happen naturally for this PR to be in 20.0.0. We may ask for help from release managers. And yes a bug fix still gets a chance to be back-ported (depending on the severity I guess?).

raulcd · 2025-04-02T09:20:45Z

The release manager (@assignUser ) has been notified and is happy to add it to the maintenance branch, as per Zulip conversation.

pitrou · 2025-04-02T09:22:15Z

CI failures are unrelated.

EnricoMi · 2025-04-02T10:49:17Z

Awesome, many thanks! Happy to help with the back-porting, please let me know if I can help!

conbench-apache-arrow · 2025-04-02T20:50:45Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 6452194.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 48 possible false positives for unstable benchmarks that are known to sometimes produce them.

…#44990) ### 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: apacheGH-44988. This has also been addressed here as it can only be exposed after fixing the wipe-out issue. Fixes apacheGH-43057. Fixes apacheGH-44852. Fixes apacheGH-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: apache#43057 Lead-authored-by: Enrico Minack <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

EnricoMi requested a review from wgtmac as a code owner December 10, 2024 11:36

github-actions bot added Component: Parquet Component: C++ awaiting review Awaiting review labels Dec 10, 2024

EnricoMi mentioned this pull request Dec 10, 2024

[C++] FileDecryptionProperties used after been freed #44988

Closed

adamreeve reviewed Dec 11, 2024

View reviewed changes

cpp/src/parquet/encryption/encryption_internal.cc Outdated Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Dec 11, 2024

adamreeve reviewed Dec 11, 2024

View reviewed changes

cpp/src/parquet/encryption/internal_file_decryptor.cc Outdated Show resolved Hide resolved

wgtmac reviewed Dec 11, 2024

View reviewed changes

cpp/src/parquet/encryption/encryption_internal.h Outdated Show resolved Hide resolved

pitrou requested changes Dec 11, 2024

View reviewed changes

EnricoMi force-pushed the thread-safe-endecryptor branch from a2e3992 to 108c8cf Compare December 11, 2024 19:49

EnricoMi force-pushed the thread-safe-endecryptor branch 2 times, most recently from 33ff64b to 6508c5a Compare December 12, 2024 09:49

adamreeve reviewed Dec 12, 2024

View reviewed changes

pitrou reviewed Dec 18, 2024

View reviewed changes

cpp/src/parquet/encryption/internal_file_decryptor.cc Outdated Show resolved Hide resolved

EnricoMi force-pushed the thread-safe-endecryptor branch from 4fc5b42 to b652e06 Compare January 21, 2025 08:55

EnricoMi force-pushed the thread-safe-endecryptor branch from b652e06 to fbe3a1c Compare January 29, 2025 18:25

pitrou requested changes Feb 6, 2025

View reviewed changes

cpp/src/parquet/encryption/encryption_internal.cc Outdated Show resolved Hide resolved

cpp/src/parquet/column_reader.cc Outdated Show resolved Hide resolved

cpp/src/arrow/dataset/file_parquet_encryption_test.cc Outdated Show resolved Hide resolved

EnricoMi force-pushed the thread-safe-endecryptor branch from 85c55ba to 4d388fa Compare February 6, 2025 17:17

This comment was marked as outdated.

Sign in to view

pitrou reviewed Feb 12, 2025

View reviewed changes

cpp/src/arrow/dataset/file_parquet_encryption_test.cc Outdated Show resolved Hide resolved

adamreeve mentioned this pull request Apr 1, 2025

Use a "SecureString" like type to store Parquet encryption keys apache/arrow-rs#7373

Open

Hopefully fix compilation error

9961069

pitrou approved these changes Apr 2, 2025

View reviewed changes

pitrou merged commit 6452194 into apache:main Apr 2, 2025
29 of 32 checks passed

pitrou removed the awaiting committer review Awaiting committer review label Apr 2, 2025

pitrou mentioned this pull request Apr 2, 2025

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

Closed

EnricoMi deleted the thread-safe-endecryptor branch April 2, 2025 10:49

benaryorg mentioned this pull request May 12, 2025

Build failure: ceph NixOS/nixpkgs#406306

Closed

3 tasks

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

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

Uh oh!

Conversation

EnricoMi commented Dec 10, 2024 • edited by pitrou Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Uh oh!

Uh oh!

adamreeve commented Dec 11, 2024

Uh oh!

pitrou commented Dec 11, 2024

Uh oh!

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EnricoMi commented Dec 11, 2024

Uh oh!

adamreeve commented Dec 11, 2024

Uh oh!

EnricoMi commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EnricoMi commented Dec 12, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mapleFU commented Jan 21, 2025

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

EnricoMi commented Apr 1, 2025

Uh oh!

pitrou commented Apr 2, 2025

Uh oh!

pitrou commented Apr 2, 2025

Uh oh!

github-actions bot commented Apr 2, 2025

Uh oh!

github-actions bot commented Apr 2, 2025

Uh oh!

pitrou commented Apr 2, 2025

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 2, 2025

Uh oh!

EnricoMi commented Apr 2, 2025

Uh oh!

pitrou commented Apr 2, 2025

Uh oh!

zanmato1984 commented Apr 2, 2025

Uh oh!

EnricoMi commented Dec 10, 2024 •

edited by pitrou

Loading

EnricoMi commented Dec 12, 2024 •

edited

Loading

raulcd commented Apr 2, 2025 •

edited by pitrou

Loading