adamreeve commented on code in PR #50972:
URL: https://github.com/apache/arrow/pull/50972#discussion_r3847827023
##########
cpp/src/parquet/thrift_internal.h:
##########
@@ -586,57 +587,64 @@ class ThriftDeserializer {
container_size_limit_(container_size_limit) {}
// Deserialize a thrift message from buf/len. buf/len must at least contain
- // all the bytes needed to store the thrift message. On return, len will be
- // set to the actual length of the header.
+ // all the bytes needed to store the thrift message.
+ // The actual length of the header is returned.
template <class T>
- void DeserializeMessage(const uint8_t* buf, uint32_t* len, T*
deserialized_msg,
- Decryptor* decryptor = NULLPTR) {
+ int64_t DeserializeMessage(const uint8_t* buf, int64_t len, T*
deserialized_msg,
+ Decryptor* decryptor = NULLPTR) {
if (decryptor == NULLPTR) {
// thrift message is not encrypted
- DeserializeUnencryptedMessage(buf, len, deserialized_msg);
+ return DeserializeUnencryptedMessage(buf, len, deserialized_msg);
} else {
// thrift message is encrypted
- uint32_t clen;
- clen = *len;
- if (clen > static_cast<uint32_t>(std::numeric_limits<int32_t>::max())) {
+ if (len > std::numeric_limits<int32_t>::max()) {
std::stringstream ss;
- ss << "Cannot decrypt buffer with length " << clen << ", which
overflows int32\n";
+ ss << "Cannot decrypt buffer with length " << len << ", which
overflows int32\n";
throw ParquetException(ss.str());
}
// decrypt
auto decrypted_buffer = AllocateBuffer(
- decryptor->pool(),
decryptor->PlaintextLength(static_cast<int32_t>(clen)));
- std::span<const uint8_t> cipher_buf(buf, clen);
- uint32_t decrypted_buffer_len =
+ decryptor->pool(),
decryptor->PlaintextLength(static_cast<int32_t>(len)));
+ std::span<const uint8_t> cipher_buf(buf, len);
+ int32_t decrypted_buffer_len =
decryptor->Decrypt(cipher_buf,
decrypted_buffer->mutable_span_as<uint8_t>());
if (decrypted_buffer_len <= 0) {
throw ParquetException("Couldn't decrypt buffer\n");
}
- *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?
Review Comment:
I think LE is correct here, for consistency with the method documentation
and signature. Even if they are usually equal in practice.
##########
cpp/src/parquet/metadata.h:
##########
@@ -311,7 +320,7 @@ class PARQUET_EXPORT FileMetaData {
const ApplicationVersion& writer_version() const;
/// \brief Size of the original thrift encoded metadata footer.
- uint32_t size() const;
+ int64_t size() const;
Review 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.
##########
cpp/src/parquet/thrift_internal.h:
##########
@@ -586,57 +587,64 @@ class ThriftDeserializer {
container_size_limit_(container_size_limit) {}
// Deserialize a thrift message from buf/len. buf/len must at least contain
- // all the bytes needed to store the thrift message. On return, len will be
- // set to the actual length of the header.
+ // all the bytes needed to store the thrift message.
+ // The actual length of the header is returned.
template <class T>
- void DeserializeMessage(const uint8_t* buf, uint32_t* len, T*
deserialized_msg,
- Decryptor* decryptor = NULLPTR) {
+ int64_t DeserializeMessage(const uint8_t* buf, int64_t len, T*
deserialized_msg,
+ Decryptor* decryptor = NULLPTR) {
if (decryptor == NULLPTR) {
// thrift message is not encrypted
- DeserializeUnencryptedMessage(buf, len, deserialized_msg);
+ return DeserializeUnencryptedMessage(buf, len, deserialized_msg);
} else {
// thrift message is encrypted
- uint32_t clen;
- clen = *len;
- if (clen > static_cast<uint32_t>(std::numeric_limits<int32_t>::max())) {
+ if (len > std::numeric_limits<int32_t>::max()) {
std::stringstream ss;
- ss << "Cannot decrypt buffer with length " << clen << ", which
overflows int32\n";
+ ss << "Cannot decrypt buffer with length " << len << ", which
overflows int32\n";
throw ParquetException(ss.str());
}
// decrypt
auto decrypted_buffer = AllocateBuffer(
- decryptor->pool(),
decryptor->PlaintextLength(static_cast<int32_t>(clen)));
- std::span<const uint8_t> cipher_buf(buf, clen);
- uint32_t decrypted_buffer_len =
+ decryptor->pool(),
decryptor->PlaintextLength(static_cast<int32_t>(len)));
+ std::span<const uint8_t> cipher_buf(buf, len);
+ int32_t decrypted_buffer_len =
decryptor->Decrypt(cipher_buf,
decrypted_buffer->mutable_span_as<uint8_t>());
if (decrypted_buffer_len <= 0) {
throw ParquetException("Couldn't decrypt buffer\n");
}
- *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?
+ DeserializeUnencryptedMessage(decrypted_buffer->data(),
decrypted_buffer_len,
deserialized_msg);
+ return read_bytes;
}
}
private:
// On Thrift 0.14.0+, we want to use TConfiguration to raise the max message
size
// limit (ARROW-13655). If we wanted to protect against huge messages, we
could
// do it ourselves since we know the message size up front.
- std::shared_ptr<ThriftBuffer> CreateReadOnlyMemoryBuffer(uint8_t* buf,
uint32_t len) {
+ std::shared_ptr<ThriftBuffer> CreateReadOnlyMemoryBuffer(uint8_t* buf,
int64_t len) {
+ if (len >= static_cast<int64_t>(std::numeric_limits<uint32_t>::max())) {
+ std::stringstream ss;
+ ss << "Cannot decrypt deserialize Thrift message with length " << len
+ << ", which overflows uint32\n";
Review Comment:
This is used for reading unencrypted messages
```suggestion
ss << "Cannot deserialize Thrift message with length " << len
<< ", which overflows uint32\n";
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
