apache / arrow

Apache Arrow is a multi-language toolbox for accelerated data interchange and in-memory processing
https://arrow.apache.org/
Apache License 2.0
14.16k stars 3.46k forks source link

[C++][Parquet] AesEncryptor::Make should return smart pointer, not a raw pointer #43221

Closed pitrou closed 1 month ago

pitrou commented 1 month ago

Describe the bug, including details regarding any error messages, version, and platform.

Currently, AesEncryptor::Make returns a raw AesEncryptor* and relies on the caller to explicit call delete when done with the instance. This is unsafe, since if an exception is thrown while using the encryptor, it won't be deallocated; not only this is a memory leak, but it might (?) also be a security problem if the corresponding OpenSSL cipher object keeps confidential data around.

Component(s)

C++, Parquet

pitrou commented 1 month ago

FTR @adamreeve @mapleFU @wgtmac

mapleFU commented 1 month ago
  static AesEncryptor* Make(ParquetCipher::type alg_id, int key_len, bool metadata,
                            std::vector<AesEncryptor*>* all_encryptors);

  static AesEncryptor* Make(ParquetCipher::type alg_id, int key_len, bool metadata,
                            bool write_length,
                            std::vector<AesEncryptor*>* all_encryptors);

I agree this interface is a bit weird...We'd better change that, especially std::vector<AesEncryptor*> all_encryptors making it looks more weird...

mapleFU commented 1 month ago

Besides, should we remove legacy method or deprecate it first?

pitrou commented 1 month ago

It's an internal API, so we can remove whatever we want :-)

mapleFU commented 1 month ago
  static std::unique_ptr<AesEncryptor> Make(ParquetCipher::type alg_id, int key_len, bool metadata);

  static std::unique_ptr<AesEncryptor>  Make(ParquetCipher::type alg_id, int key_len, bool metadata,
                            bool write_length);

The api above seems to be better 🤔

pitrou commented 1 month ago

Issue resolved by pull request 43222 https://github.com/apache/arrow/pull/43222