Fix: Error rather than panic on invalid dictionary index bit width in Parquet reader - #10725
Conversation
Validate that dictionary index data pages have non-empty value sections and bit widths bounded by 0..=32 in DictIndexDecoder and ByteArrayDictionary, returning a clean ParquetError instead of panicking on malformed or untrusted input.
Rich-T-kid
left a comment
Rich-T-kid
left a comment
There was a problem hiding this comment.
this make sense to me.
i'm not super familiar with the arrow-rs parquet decode code, are all offsets/indexes validated or is it assumed that the offsets are correct? for example for variable length data I would assume that we check the prefix length against a buffer length to make sure we don't have OOB errors.
alamb
changed the title
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Makes sense to me -- thank you @dhruvxvaishnav and @Rich-T-kid
I have one question
|
|
||
| impl RleDecoder { | ||
| pub fn new(bit_width: u8) -> Self { | ||
| debug_assert!(bit_width <= 64, "Bit width must be <= 64"); |
There was a problem hiding this comment.
The checks above say 32 bits is the max, but this assert allows 64 -- why the discrepancy?
There was a problem hiding this comment.
I think it's somehow related to delta binary, which uses bit packing up to 64 bits. But the current delta decoder doesn't use RleDecoder and instead uses BitReader directly. Kind of confusing.
There was a problem hiding this comment.
RleDecoder is a generic Parquet RLE / bit-packing hybrid decoder that supports unpacking values up to 64 bits wide (using a u64 internal accumulator). However, Parquet dictionary indices are 32-bit signed integers (i32), so dictionary index bit widths are bounded by MAX_RLE_DICTIONARY_BIT_WIDTH (32). I have added doc comments on RleDecoder::new and MAX_RLE_DICTIONARY_BIT_WIDTH explaining this distinction.
etseidl
left a comment
etseidl
left a comment
There was a problem hiding this comment.
Looks fine to me, just one suggestion.
| let bit_width = *data | ||
| .first() | ||
| .ok_or_else(|| general_err!("dictionary index page is empty"))?; | ||
| if bit_width > 32 { |
There was a problem hiding this comment.
Maybe add a constant somewhere (MAX_RLE_DICTIONARY_BIT_WIDTH 🤷).
There was a problem hiding this comment.
Added MAX_RLE_DICTIONARY_BIT_WIDTH = 32 in parquet::encodings::rle and updated DictIndexDecoder, ByteArrayDictionaryReader (DictionaryDecoder), and DictDecoder to use it.
…oders Define MAX_RLE_DICTIONARY_BIT_WIDTH in encodings::rle and use it across DictIndexDecoder, ByteArrayDictionary, and DictDecoder to avoid magic numbers and clarify the 32-bit bound for dictionary index bit widths.
|
CI is backed up today...I'll merge this once it finishes |
|
Thanks @dhruvxvaishnav (and @alamb and @Rich-T-kid for reviews) |
4 participants
Which issue does this PR close?
Rationale for this change
When reading dictionary-encoded Parquet data pages with DictIndexDecoder or ByteArrayDictionaryReader, the first byte was read via data[0] without checking if data is empty, and the index �it_width was passed to RleDecoder without checking the