GH-50971: [C++][Parquet] Fix usage of disparate length types for metadata reading - #50972
GH-50971: [C++][Parquet] Fix usage of disparate length types for metadata reading#50972pitrou wants to merge 1 commit into
Conversation
|
|
|
@HuaHuaY @adamreeve @wgtmac Do you want to take a look? |
…r metadata reading
| *len = decryptor->CiphertextLength(static_cast<int32_t>(decrypted_buffer_len)); | ||
| DeserializeUnencryptedMessage(decrypted_buffer->data(), &decrypted_buffer_len, | ||
| int64_t read_bytes = decryptor->CiphertextLength(decrypted_buffer_len); | ||
| ARROW_DCHECK_LE(read_bytes, len); // XXX should they be equal? |
There was a problem hiding this comment.
I think LE is correct here, for consistency with the method documentation and signature. Even if they are usually equal in practice.
| ss << "Cannot decrypt deserialize Thrift message with length " << len | ||
| << ", which overflows uint32\n"; |
There was a problem hiding this comment.
This is used for reading unencrypted messages
| ss << "Cannot decrypt deserialize Thrift message with length " << len | |
| << ", which overflows uint32\n"; | |
| ss << "Cannot deserialize Thrift message with length " << len | |
| << ", which overflows uint32\n"; |
|
|
||
| /// \brief Size of the original thrift encoded metadata footer. | ||
| uint32_t size() const; | ||
| int64_t size() const; |
There was a problem hiding this comment.
Should this be considered a breaking change? I think it's probably OK. Any existing consumer code might end up with a narrowing conversion when storing the result of this, or get a different type if they assign to an auto typed variable, but that should work out OK.
There was a problem hiding this comment.
I would optimistically say it's ok too. More importantly, I'm not sure anyone but us has a use for this API.
github-actions
Bot
added
awaiting committer review
2 participants
Rationale for this change
The usage of disparate integer types (
int64_t,uint32_t) makes our checks and computations fragile, especially with C++ adding its own integer promotion rules across arithmetic operations.We have had at least one report (courtesy of Ada Logics and Claude) where a carefully crafted Parquet file can read from an invalid pointer due to arithmetic overflow in the 32-bit domain.
What changes are included in this PR?
Use
int64_tthroughout most internal APIs and code paths when reading Parquet metadata. Other types such asuint32_tshould only be used where necessary when interacting with third-party libraries such as Thrift C++.Are these changes tested?
By existing tests, and manually using said hand-crafted Parquet file.
Are there any user-facing changes?
Some APIs taking a
uint32_t*inout-parameter are deprecated, alternatives taking aint64_tvalue are available.