GH-3561 Harden variants against malformed metadata.#3562
Open
steveloughran wants to merge 2 commits into
Open
Conversation
steveloughran
changed the title
steveloughran
force-pushed
the
pr/variant-hardening
branch
from
636e75d to
9905728
Compare
steveloughran
force-pushed
the
pr/variant-hardening
branch
from
9905728 to
db1cfe2
Compare
- reject oversized metadata/value declarations - range checking Only low cost checks are made. There's also a depth check consistent with the json parser; it's arguable as to whether that is needed. It will defend against StackOverflowExceptions by anything trying to treewalk, but should that code be the place to do the checks? The new test suite TestHardenedReader can be configured to actually emit the malformed files, to see how applications deal with them. Contains contributions from Claude AI.
steveloughran
force-pushed
the
pr/variant-hardening
branch
from
db1cfe2 to
27d92e6
Compare
Contributor
Author
|
@rdblue why not review this PR before worrying about the iceberg one, as its where some things could be clarified and then repeated.
|
Contributor
Author
|
@Fokko could you look at this while I work on that thrift update? |
Fokko
reviewed
May 27, 2026
Contributor
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
This look good to me, thanks @steveloughran for working on this. I left a few comments. Should we also test the performance implications for list/dicts? Maybe a small JMH microbenchmark.
Comment on lines
+40
to
+41
| // version=1, offsetSize=1, dictSize=0, single end-offset=0 — minimum well-formed empty metadata | ||
| static final ByteBuffer EMPTY_METADATA = ByteBuffer.wrap(new byte[] {0b1, 0x00, 0x00}); |
Contributor
There was a problem hiding this comment.
I assume that we don't accept the single byte anymore? Should we still allow that, or make it explicit to the users?
| this.dictSize = 0; | ||
| } | ||
| this.metadataCache = null; | ||
| VariantUtil.validateValueShallow(this.value, dictSize); |
Contributor
There was a problem hiding this comment.
Since we now validate in the constructor, should we add some Javadoc to this public function? Including the @throws IllegalArgumentException
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
Malformed parquet files could be distruptive enough to not only affect the execution of a single worker thread (which will ultimately reject it), but other threads on the same process. This can be disruptive.
What changes are included in this PR?
Only low cost checks are made, equivalent to arrow variant
try_new_with_metadata_and_shallow_validation()There's no equivalent
with_full_validation()logic is omitted. The caching logic of #3481 may be able to do this when it builds a dictionary, as range checking the increasing dictionary offsets is the key work there.There's also a depth check consistent with the json parser; it's arguable as to whether that is needed. It will defend against StackOverflowExceptions by anything trying to treewalk, but shouldn't that code be the place to do the checks?
Are these changes tested?
The new test suite TestHardenedReader can be configured to actually emit the malformed files, to see how applications deal with them.
Existing tests needed changes because they'd been getting away with malformed byte arrays; now this stuff is being checked the test cases need valid variants.
Are there any user-facing changes?
No
Closes #3561