Uh oh!
There was an error while loading. Please reload this page.
GH-48335: [C++][Parquet] Fuzz encrypted files - #48336
Conversation
| std::unique_ptr<ColumnCryptoMetaData> crypto_metadata = col_chunk->crypto_metadata(); | ||
| if (crypto_metadata != nullptr) { | ||
| ParquetException::NYI("Cannot read encrypted bloom filter yet"); | ||
| ParquetException::NYI("BloomFilter decryption is not yet supported"); |
There was a problem hiding this comment.
This just ensures the same exception message in used in different places.
| : ParquetException(columnPath.c_str()) {} | ||
| }; | ||
| inline const uint8_t* str2bytes(const std::string& str) { |
| namespace { | ||
| Status FuzzReadData(std::unique_ptr<FileReader> reader) { |
There was a problem hiding this comment.
This code is moved away to fuzz_internal.cc so that parquet/arrow/reader.cc does not depend on Parquet encryption.
fe50a06 to
eb4c4c8Comparepitrou
commented
Dec 4, 2025
@github-actions crossbow submit -g cpp |
Revision: eb4c4c8 Submitted crossbow builds: ursacomputing/crossbow @ actions-40f4ae6d06 |
Uh oh!
There was an error while loading. Please reload this page.
eb4c4c8 to
219985aComparepitrou
commented
Dec 4, 2025
@github-actions crossbow submit -g cpp |
Revision: 219985a Submitted crossbow builds: ursacomputing/crossbow @ actions-cadb3008d8 |
pitrou
commented
Dec 4, 2025
@adamreeve@EnricoMi Do you want to take a look at this PR? |
### Rationale for this change While working on PR #48336 I quickly got irritated by the clumsiness of the Parquet encryption configuration API. This issue attempts to reduce the verbosity of `ColumnEncryptionProperties` construction. ### What changes are included in this PR? 1. Remove unused `column_path` member in `ColumnEncryptionProperties` 2. Add convenience `ColumnEncryptionProperties` factory functions to avoid going through the clumsy `Builder` API ### Are these changes tested? Yes ### Are there any user-facing changes? Two deprecated constructors and a removed accessor for an useless property. **This PR includes breaking changes to public APIs.** (If there are any breaking changes to public APIs, please explain which changes are breaking. If not, you can remove this.) * GitHub Issue: #48337 Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
219985a to
c72edefComparepitrou
commented
Dec 8, 2025
@github-actions crossbow submit -g cpp |
Revision: c72edef Submitted crossbow builds: ursacomputing/crossbow @ actions-4952d9d2f1 |
Uh oh!
There was an error while loading. Please reload this page.
pitrou
commented
Dec 8, 2025
Thanks for the review @adamreeve . I hope this will help uncover any latent issues in the decryption codepaths. |
… corpus (#48558) ### Rationale for this change In #48336 we skipped encrypted nested columns because it was too cumbersome to configure (each leaf column had to be configured independently). Now that #45462 has been merged we can configure nested columns the same way as non-nested ones. ### Are these changes tested? Manually and later by OSS-Fuzz. ### Are there any user-facing changes? No. * GitHub Issue: #48557 Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit fa4e593. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 13 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…48338) ### Rationale for this change While working on PR apache#48336 I quickly got irritated by the clumsiness of the Parquet encryption configuration API. This issue attempts to reduce the verbosity of `ColumnEncryptionProperties` construction. ### What changes are included in this PR? 1. Remove unused `column_path` member in `ColumnEncryptionProperties` 2. Add convenience `ColumnEncryptionProperties` factory functions to avoid going through the clumsy `Builder` API ### Are these changes tested? Yes ### Are there any user-facing changes? Two deprecated constructors and a removed accessor for an useless property. **This PR includes breaking changes to public APIs.** (If there are any breaking changes to public APIs, please explain which changes are breaking. If not, you can remove this.) * GitHub Issue: apache#48337 Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
### Rationale for this change Currently, if given an encrypted file (whether valid or invalid), the Parquet fuzz target will bail out almost immediately since it does not have any decryption key configured. This prevents the fuzzer from covering any significant part of the Parquet encryption codebase. ### What changes are included in this PR? 1. Improve the fuzz target so as to be able to read encrypted files present in the seed corpus (except those that require a external AAD or external key material) 2. Generate encrypted files in the seed corpus that the fuzz target is able to successfully decrypt ### Are these changes tested? In expanded unit test, and locally. ### Are there any user-facing changes? No. * GitHub Issue: apache#48335 Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
…z seed corpus (apache#48558) ### Rationale for this change In apache#48336 we skipped encrypted nested columns because it was too cumbersome to configure (each leaf column had to be configured independently). Now that apache#45462 has been merged we can configure nested columns the same way as non-nested ones. ### Are these changes tested? Manually and later by OSS-Fuzz. ### Are there any user-facing changes? No. * GitHub Issue: apache#48557 Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
Rationale for this change
Currently, if given an encrypted file (whether valid or invalid), the Parquet fuzz target will bail out almost immediately since it does not have any decryption key configured. This prevents the fuzzer from covering any significant part of the Parquet encryption codebase.
What changes are included in this PR?
Are these changes tested?
In expanded unit test, and locally.
Are there any user-facing changes?
No.