Repository navigation
GH-55388: [C++][Parquet] Reject out-of-bounds dictionary indices in all RLE_DICTIONARY decode paths - #55389
Open
b84330808 wants to merge 1 commit into
Open
GH-55388: [C++][Parquet] Reject out-of-bounds dictionary indices in all RLE_DICTIONARY decode paths#55389b84330808 wants to merge 1 commit into
b84330808 wants to merge 1 commit into
Conversation
…TIONARY decode paths DictDecoder::DecodeIndices() and DecodeIndicesSpaced() did not check the decoded indices against the dictionary length, so ReadBatchWithDictionary() and read_dictionary reads could return indices past the end of the dictionary. RleBitPackedDecoder also did not stop when a run it was resuming, or a bit-packed run longer than one decoding chunk, held an invalid dictionary index: it moved on to the next run. That silently skipped values, and for an RLE run it looped forever in release builds (DCHECK failure in debug builds). Stop decoding as soon as a run is not entirely consumed while the batch is not full, and check the indices in the DecodeIndices methods like the other dictionary decoding methods do.
|
|
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
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
Out-of-bounds dictionary indices are rejected by
DictDecoder::Decode()andDecodeArrow(), but:DecodeIndices()andDecodeIndicesSpaced()don't check them at all, soReadBatchWithDictionary()andread_dictionaryreads can return indices past the end of the dictionary;RleBitPackedDecoderdoesn't stop when a run it is resuming (or a long bit-packed run) has an invalid index: it moves on to the next run. That silently skips values, and for an RLE run it loops forever in release builds (and hits a DCHECK in debug builds).What changes are included in this PR?
RleBitPackedDecoder::ProcessValues()andGetSpaced()stop as soon as a run is not entirely consumed while the batch is not full, which only happens on invalid input, instead of resuming with the next run.DictDecoderImpl::DecodeIndices()(both overloads) andDecodeIndicesSpaced()check the decoded indices against the dictionary length, like the other dictionary decoding methods do.Are these changes tested?
Yes, new tests in
encoding_test.cc(allDictDecodermethods with an out-of-bounds index) andrle_encoding_test.cc(resuming a partially read RLE run). Without the fix, the Parquet tests fail and the RLE test hits the DCHECK in debug builds; in release builds it doesn't return.Are there any user-facing changes?
Reading Parquet data with out-of-bounds dictionary indices now errors out in these code paths.
This PR contains a "Critical Fix". Invalid dictionary indices could make the decoder silently return values from the following run (incorrect data) or loop forever.
Was AI used for this PR?
In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.
PR code and description written by:
Reviewed before submission by:
I used Claude Code to find this (with a local fuzz harness over the RecordReader / low-level reader APIs) and to write the change and tests.