You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/11/16 12:52:02 UTC
[GitHub] [arrow] pitrou opened a new pull request #11710: ARROW-14721: [C++] Strength DELTA_BYTE_ARRAY decoder
pitrou opened a new pull request #11710:
URL: https://github.com/apache/arrow/pull/11710
OSS-Fuzz found situations where invalid data could crash the DELTA_BYTE_ARRAY decoder.
Should fix the following issues:
* https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=40865
* https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=40902
* https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=40922
* https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=41003
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #11710: ARROW-14721: [C++] Strengthen DELTA_BYTE_ARRAY decoder
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11710:
URL: https://github.com/apache/arrow/pull/11710#issuecomment-971421937
Benchmark runs are scheduled for baseline = 6b0248d5b5502a1cf76d5853d15be9844b84522c and contender = 35cc9d1409ed4bab07a40af704a13a6b2011f691. 35cc9d1409ed4bab07a40af704a13a6b2011f691 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/447cab751bb74e22a4f8c9c5ced6f090...da6bfe1d83a54bde9e431f7daf3ae004/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/fb965a90766c4205964a4a63f239a6a7...10d3ef86158e44c19851a29f8e3a57fe/)
[Finished :arrow_down:1.06% :arrow_up:0.13%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/7372a2b9b1594ca69938182e34532378...992bc3c783754e5e8ef2fae595ef92a1/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #11710: ARROW-14721: [C++] Strengthen DELTA_BYTE_ARRAY decoder
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #11710:
URL: https://github.com/apache/arrow/pull/11710#issuecomment-971348302
LGTM module a nit and a question about data sizing.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield edited a comment on pull request #11710: ARROW-14721: [C++] Strengthen DELTA_BYTE_ARRAY decoder
Posted by GitBox <gi...@apache.org>.
emkornfield edited a comment on pull request #11710:
URL: https://github.com/apache/arrow/pull/11710#issuecomment-971348302
LGTM except for a nit and a question about data sizing.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #11710: ARROW-14721: [C++] Strength DELTA_BYTE_ARRAY decoder
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #11710:
URL: https://github.com/apache/arrow/pull/11710#issuecomment-970243625
@shanhuuang FYI
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #11710: ARROW-14721: [C++] Strengthen DELTA_BYTE_ARRAY decoder
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #11710:
URL: https://github.com/apache/arrow/pull/11710#discussion_r751027622
##########
File path: cpp/src/parquet/encoding.cc
##########
@@ -2401,24 +2413,31 @@ class DeltaByteArrayDecoder : public DecoderImpl,
reinterpret_cast<const int32_t*>(buffered_prefix_length_->data()) +
prefix_len_offset_;
for (int i = 0; i < max_values; ++i) {
- data_size += prefix_len_ptr[i] + buffer[i].len;
+ if (AddWithOverflow(data_size, prefix_len_ptr[i], &data_size) ||
+ AddWithOverflow(data_size, buffer[i].len, &data_size)) {
+ throw ParquetException("excess expansion in DELTA_BYTE_ARRAY");
+ }
}
PARQUET_THROW_NOT_OK(buffered_data_->Resize(data_size));
+ string_view prefix{last_value_};
uint8_t* data_ptr = buffered_data_->mutable_data();
for (int i = 0; i < max_values; ++i) {
- DCHECK_LE(static_cast<const size_t>(prefix_len_ptr[i]), last_value_.length());
- memcpy(data_ptr, last_value_.data(), prefix_len_ptr[i]);
+ if (ARROW_PREDICT_FALSE(static_cast<size_t>(prefix_len_ptr[i]) > prefix.length())) {
+ throw ParquetException("prefix length too large");
+ }
+ memcpy(data_ptr, prefix.data(), prefix_len_ptr[i]);
+ // buffer[i] currently points to the string suffix
memcpy(data_ptr + prefix_len_ptr[i], buffer[i].ptr, buffer[i].len);
buffer[i].ptr = data_ptr;
buffer[i].len += prefix_len_ptr[i];
data_ptr += buffer[i].len;
- last_value_ =
- std::string(reinterpret_cast<const char*>(buffer[i].ptr), buffer[i].len);
+ prefix = string_view{reinterpret_cast<const char*>(buffer[i].ptr), buffer[i].len};
}
prefix_len_offset_ += max_values;
this->num_values_ -= max_values;
num_valid_values_ -= max_values;
+ last_value_ = prefix.to_string();
Review comment:
No particular reason. I'll make the change.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on a change in pull request #11710: ARROW-14721: [C++] Strengthen DELTA_BYTE_ARRAY decoder
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #11710:
URL: https://github.com/apache/arrow/pull/11710#discussion_r751004090
##########
File path: cpp/src/parquet/encoding.cc
##########
@@ -2265,21 +2269,27 @@ class DeltaLengthByteArrayDecoder : public DecoderImpl,
}
int Decode(ByteArray* buffer, int max_values) override {
+ // Decode up to `max_values` strings into an internal buffer
+ // and reference them into `buffer`.
max_values = std::min(max_values, num_valid_values_);
- int64_t data_size = 0;
+ int32_t data_size = 0;
Review comment:
it is not immediately clear to me why this must be less than int32_t? I guess I need to reread the spec on this.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #11710: ARROW-14721: [C++] Strengthen DELTA_BYTE_ARRAY decoder
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11710:
URL: https://github.com/apache/arrow/pull/11710#issuecomment-971421937
Benchmark runs are scheduled for baseline = 6b0248d5b5502a1cf76d5853d15be9844b84522c and contender = 35cc9d1409ed4bab07a40af704a13a6b2011f691. 35cc9d1409ed4bab07a40af704a13a6b2011f691 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/447cab751bb74e22a4f8c9c5ced6f090...da6bfe1d83a54bde9e431f7daf3ae004/)
[Finished :arrow_down:2.56% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/fb965a90766c4205964a4a63f239a6a7...10d3ef86158e44c19851a29f8e3a57fe/)
[Finished :arrow_down:1.06% :arrow_up:0.13%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/7372a2b9b1594ca69938182e34532378...992bc3c783754e5e8ef2fae595ef92a1/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou closed pull request #11710: ARROW-14721: [C++] Strengthen DELTA_BYTE_ARRAY decoder
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #11710:
URL: https://github.com/apache/arrow/pull/11710
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #11710: ARROW-14721: [C++] Strengthen DELTA_BYTE_ARRAY decoder
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11710:
URL: https://github.com/apache/arrow/pull/11710#issuecomment-971421937
Benchmark runs are scheduled for baseline = 6b0248d5b5502a1cf76d5853d15be9844b84522c and contender = 35cc9d1409ed4bab07a40af704a13a6b2011f691. 35cc9d1409ed4bab07a40af704a13a6b2011f691 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/447cab751bb74e22a4f8c9c5ced6f090...da6bfe1d83a54bde9e431f7daf3ae004/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/fb965a90766c4205964a4a63f239a6a7...10d3ef86158e44c19851a29f8e3a57fe/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/7372a2b9b1594ca69938182e34532378...992bc3c783754e5e8ef2fae595ef92a1/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on a change in pull request #11710: ARROW-14721: [C++] Strengthen DELTA_BYTE_ARRAY decoder
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #11710:
URL: https://github.com/apache/arrow/pull/11710#discussion_r750998396
##########
File path: cpp/src/parquet/encoding.cc
##########
@@ -2401,24 +2413,31 @@ class DeltaByteArrayDecoder : public DecoderImpl,
reinterpret_cast<const int32_t*>(buffered_prefix_length_->data()) +
prefix_len_offset_;
for (int i = 0; i < max_values; ++i) {
- data_size += prefix_len_ptr[i] + buffer[i].len;
+ if (AddWithOverflow(data_size, prefix_len_ptr[i], &data_size) ||
+ AddWithOverflow(data_size, buffer[i].len, &data_size)) {
+ throw ParquetException("excess expansion in DELTA_BYTE_ARRAY");
+ }
}
PARQUET_THROW_NOT_OK(buffered_data_->Resize(data_size));
+ string_view prefix{last_value_};
uint8_t* data_ptr = buffered_data_->mutable_data();
for (int i = 0; i < max_values; ++i) {
- DCHECK_LE(static_cast<const size_t>(prefix_len_ptr[i]), last_value_.length());
- memcpy(data_ptr, last_value_.data(), prefix_len_ptr[i]);
+ if (ARROW_PREDICT_FALSE(static_cast<size_t>(prefix_len_ptr[i]) > prefix.length())) {
+ throw ParquetException("prefix length too large");
+ }
+ memcpy(data_ptr, prefix.data(), prefix_len_ptr[i]);
+ // buffer[i] currently points to the string suffix
memcpy(data_ptr + prefix_len_ptr[i], buffer[i].ptr, buffer[i].len);
buffer[i].ptr = data_ptr;
buffer[i].len += prefix_len_ptr[i];
data_ptr += buffer[i].len;
- last_value_ =
- std::string(reinterpret_cast<const char*>(buffer[i].ptr), buffer[i].len);
+ prefix = string_view{reinterpret_cast<const char*>(buffer[i].ptr), buffer[i].len};
}
prefix_len_offset_ += max_values;
this->num_values_ -= max_values;
num_valid_values_ -= max_values;
+ last_value_ = prefix.to_string();
Review comment:
nit: to_string is a non-standard extension of our vendored version. Is there a strong reason to use it here vs std::string(prefix)?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #11710: ARROW-14721: [C++] Strengthen DELTA_BYTE_ARRAY decoder
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #11710:
URL: https://github.com/apache/arrow/pull/11710#discussion_r751028161
##########
File path: cpp/src/parquet/encoding.cc
##########
@@ -2265,21 +2269,27 @@ class DeltaLengthByteArrayDecoder : public DecoderImpl,
}
int Decode(ByteArray* buffer, int max_values) override {
+ // Decode up to `max_values` strings into an internal buffer
+ // and reference them into `buffer`.
max_values = std::min(max_values, num_valid_values_);
- int64_t data_size = 0;
+ int32_t data_size = 0;
Review comment:
Not in the spec, but simply the fact that `GetBatch` takes an `int` for the batch size.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #11710: ARROW-14721: [C++] Strength DELTA_BYTE_ARRAY decoder
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11710:
URL: https://github.com/apache/arrow/pull/11710#issuecomment-970241973
https://issues.apache.org/jira/browse/ARROW-14721
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on a change in pull request #11710: ARROW-14721: [C++] Strengthen DELTA_BYTE_ARRAY decoder
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #11710:
URL: https://github.com/apache/arrow/pull/11710#discussion_r751036656
##########
File path: cpp/src/parquet/encoding.cc
##########
@@ -2265,21 +2269,27 @@ class DeltaLengthByteArrayDecoder : public DecoderImpl,
}
int Decode(ByteArray* buffer, int max_values) override {
+ // Decode up to `max_values` strings into an internal buffer
+ // and reference them into `buffer`.
max_values = std::min(max_values, num_valid_values_);
- int64_t data_size = 0;
+ int32_t data_size = 0;
Review comment:
I think pages can only hold int32max values in them (row groups can hold more and apparently we have a bug someplace when reading). So this makes sense.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #11710: ARROW-14721: [C++] Strengthen DELTA_BYTE_ARRAY decoder
Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #11710:
URL: https://github.com/apache/arrow/pull/11710#issuecomment-971421937
Benchmark runs are scheduled for baseline = 6b0248d5b5502a1cf76d5853d15be9844b84522c and contender = 35cc9d1409ed4bab07a40af704a13a6b2011f691. 35cc9d1409ed4bab07a40af704a13a6b2011f691 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/447cab751bb74e22a4f8c9c5ced6f090...da6bfe1d83a54bde9e431f7daf3ae004/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/fb965a90766c4205964a4a63f239a6a7...10d3ef86158e44c19851a29f8e3a57fe/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/7372a2b9b1594ca69938182e34532378...992bc3c783754e5e8ef2fae595ef92a1/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org