You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "1fanwang (via GitHub)" <gi...@apache.org> on 2026/09/01 22:32:43 UTC
[PR] GH-50314: [C++][Parquet] Reject invalid DELTA_BINARY_PACKED headers [arrow]
1fanwang opened a new pull request, #51128:
URL: https://github.com/apache/arrow/pull/51128
### Rationale for this change
Reading a Parquet file that contains a corrupt `DELTA_BINARY_PACKED` page makes the decoder allocate memory far larger than the page it is reading. A 10-byte page allocates 1 MiB, and because the miniblock count in the page header is a VLQ `uint32`, a header can ask for close to 4 GiB. The read then fails with `Decode bit-width EOF`, which does not say which header field was wrong.
Before this change, a reader hitting such a page allocates whatever the header asked for and then reports an unrelated error. After it, the reader allocates nothing and the error names both the miniblock count and the bytes actually available.
### What changes are included in this PR?
`InitHeader` now rejects a page whose miniblock count is larger than the bytes remaining in the page. `InitBlock` reads one bit-width byte per miniblock, so a page that trips this check could never have decoded, and no page that read correctly before is affected. A page holding a single value keeps that value in the header and never initializes a block, so the check skips it.
### Are these changes tested?
Yes, by two new cases in `parquet-encoding-test`. `RejectsMiniblockWidthsLargerThanInput` decodes the 10-byte page and asserts both the reported error and, via a `ProxyMemoryPool`, that nothing was allocated. `SingleValueRoundTrip` covers the single-value page that the check has to leave alone.
<details>
<summary>Red, with the decoder change reverted to the parent commit</summary>
```console
$ git checkout HEAD~1 cpp/src/parquet/decoder.cc
$ ninja parquet-encoding-test
$ ./debug/parquet-encoding-test --gtest_filter='*SingleValueRoundTrip*:*RejectsMiniblockWidthsLargerThanInput*'
[==========] Running 4 tests from 2 test suites.
[ RUN ] TestDeltaBitPackEncoding/0.SingleValueRoundTrip
[ OK ] TestDeltaBitPackEncoding/0.SingleValueRoundTrip (8 ms)
[ RUN ] TestDeltaBitPackEncoding/0.RejectsMiniblockWidthsLargerThanInput
cpp/src/parquet/encoding_test.cc:1933: Failure
Value of: err
Expected: is an object whose given property has substring "the number of miniblocks per block (1048576) is larger than the number of bytes remaining in the page (1)"
Actual: Unexpected end of stream: Decode bit-width EOF (of type parquet::ParquetException), whose given property is 0x46e0228b0 pointing to "Unexpected end of stream: Decode bit-width EOF" (of type char const*)
cpp/src/parquet/encoding_test.cc:1934: Failure
Expected equality of these values:
pool.bytes_allocated()
Which is: 1048576
0
[ FAILED ] TestDeltaBitPackEncoding/0.RejectsMiniblockWidthsLargerThanInput, where TypeParam = parquet::PhysicalType<(parquet::Type::type)1> (2 ms)
[ RUN ] TestDeltaBitPackEncoding/1.SingleValueRoundTrip
[ OK ] TestDeltaBitPackEncoding/1.SingleValueRoundTrip (0 ms)
[ RUN ] TestDeltaBitPackEncoding/1.RejectsMiniblockWidthsLargerThanInput
cpp/src/parquet/encoding_test.cc:1933: Failure
Value of: err
Expected: is an object whose given property has substring "the number of miniblocks per block (1048576) is larger than the number of bytes remaining in the page (1)"
Actual: Unexpected end of stream: Decode bit-width EOF (of type parquet::ParquetException), whose given property is 0x46e0227f0 pointing to "Unexpected end of stream: Decode bit-width EOF" (of type char const*)
cpp/src/parquet/encoding_test.cc:1934: Failure
Expected equality of these values:
pool.bytes_allocated()
Which is: 1048576
0
[ FAILED ] TestDeltaBitPackEncoding/1.RejectsMiniblockWidthsLargerThanInput, where TypeParam = parquet::PhysicalType<(parquet::Type::type)2> (0 ms)
[==========] 4 tests from 2 test suites ran. (12 ms total)
[ PASSED ] 2 tests.
[ FAILED ] 2 tests, listed below:
[ FAILED ] TestDeltaBitPackEncoding/0.RejectsMiniblockWidthsLargerThanInput, where TypeParam = parquet::PhysicalType<(parquet::Type::type)1>
[ FAILED ] TestDeltaBitPackEncoding/1.RejectsMiniblockWidthsLargerThanInput, where TypeParam = parquet::PhysicalType<(parquet::Type::type)2>
2 FAILED TESTS
```
</details>
<details>
<summary>Green, with the fix applied</summary>
```console
$ git checkout HEAD cpp/src/parquet/decoder.cc
$ ninja parquet-encoding-test
$ ./debug/parquet-encoding-test --gtest_filter='*SingleValueRoundTrip*:*RejectsMiniblockWidthsLargerThanInput*'
[==========] Running 4 tests from 2 test suites.
[ RUN ] TestDeltaBitPackEncoding/0.SingleValueRoundTrip
[ OK ] TestDeltaBitPackEncoding/0.SingleValueRoundTrip (4 ms)
[ RUN ] TestDeltaBitPackEncoding/0.RejectsMiniblockWidthsLargerThanInput
[ OK ] TestDeltaBitPackEncoding/0.RejectsMiniblockWidthsLargerThanInput (0 ms)
[ RUN ] TestDeltaBitPackEncoding/1.SingleValueRoundTrip
[ OK ] TestDeltaBitPackEncoding/1.SingleValueRoundTrip (0 ms)
[ RUN ] TestDeltaBitPackEncoding/1.RejectsMiniblockWidthsLargerThanInput
[ OK ] TestDeltaBitPackEncoding/1.RejectsMiniblockWidthsLargerThanInput (0 ms)
[==========] 4 tests from 2 test suites ran. (5 ms total)
[ PASSED ] 4 tests.
```
</details>
The full encoding suite and the neighbouring Parquet suites (`parquet-internals-test`, `parquet-reader-test`, `parquet-arrow-reader-writer-test`) report identical results before and after the change, their pre-existing failures all caused by test data files absent from this checkout.
### Are there any user-facing changes?
A `DELTA_BINARY_PACKED` page whose header asks for more miniblocks than the page has bytes now raises a `ParquetException` naming both numbers, instead of allocating first and failing with `Decode bit-width EOF`.
--
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
Re: [PR] GH-50314: [C++][Parquet] Reject invalid DELTA_BINARY_PACKED headers [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #51128:
URL: https://github.com/apache/arrow/pull/51128#discussion_r3912026321
##########
cpp/src/parquet/encoding_test.cc:
##########
@@ -1902,6 +1904,36 @@ TYPED_TEST(TestDeltaBitPackEncoding, BasicRoundTrip) {
}
}
+TYPED_TEST(TestDeltaBitPackEncoding, SingleValueRoundTrip) {
+ ASSERT_NO_FATAL_FAILURE(this->Execute(1, 1));
+}
+
+TYPED_TEST(TestDeltaBitPackEncoding, RejectsMiniblockWidthsLargerThanInput) {
+ using T = typename TypeParam::c_type;
+
+ // Header: 2^25 values per block, 2^20 miniblocks, 2 values, and first value 0,
+ // followed by min delta 0 and no miniblock bit widths.
+ const std::vector<uint8_t> encoded = {0x80, 0x80, 0x80, 0x10, 0x80,
+ 0x80, 0x40, 0x02, 0x00, 0x00};
+ ::arrow::ProxyMemoryPool pool(default_memory_pool());
+ auto decoder = MakeTypedDecoder<TypeParam>(Encoding::DELTA_BINARY_PACKED,
+ this->descr_.get(), &pool);
+ std::vector<T> decoded(2);
+
+ EXPECT_THROW_THAT(
+ [&] {
+ decoder->SetData(2, encoded.data(), static_cast<int>(encoded.size()));
+ decoder->Decode(decoded.data(), static_cast<int>(decoded.size()));
+ },
+ ParquetException,
+ ::testing::Property(
+ &ParquetException::what,
+ ::testing::HasSubstr(
+ "the number of miniblocks per block (1048576) is larger than the "
+ "number of bytes remaining in the page (1)")));
+ EXPECT_EQ(pool.bytes_allocated(), 0);
Review Comment:
`bytes_allocated` is the number of bytes currently allocated. Do we want to use `total_bytes_allocated` instead?
--
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
Re: [PR] GH-50314: [C++][Parquet] Reject invalid DELTA_BINARY_PACKED headers [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #51128:
URL: https://github.com/apache/arrow/pull/51128#issuecomment-6022853390
:warning: GitHub issue #50314 **has been automatically assigned in GitHub** to PR creator.
--
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
Re: [PR] GH-50314: [C++][Parquet] Reject invalid DELTA_BINARY_PACKED headers [arrow]
Posted by "1fanwang (via GitHub)" <gi...@apache.org>.
1fanwang commented on PR #51128:
URL: https://github.com/apache/arrow/pull/51128#issuecomment-6107847953
> We know that each miniblock requires at least one byte for its bitwidth.
@pitrou thanks, that bound is simpler than decoding a block. InitHeader now throws when the required bit-width bytes exceed the bytes left, before any allocation. Done in https://github.com/apache/arrow/commit/025e74816fb6725c8f46a87a60aedfff5d14527a, with tests for truncated headers.
--
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
Re: [PR] GH-50314: [C++][Parquet] Reject invalid DELTA_BINARY_PACKED headers [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #51128:
URL: https://github.com/apache/arrow/pull/51128#issuecomment-6012815079
Sorry for the delay here. I don't like the proposed approach because it starts decoding a block in `InitHeader`. This makes the code more complicated and brittle.
I think we can do simpler. We know that each miniblock requires at least one byte for its bitwidth.
So the check can look like:
```c++
const int64_t required_blocks = CeilDiv(total_value_count_ - 1, values_per_block_);
if (required_blocks * mini_blocks_per_block > decoder_->bytes_left()) { /* throw exception */ }
```
--
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
Re: [PR] GH-50314: [C++][Parquet] Reject invalid DELTA_BINARY_PACKED headers [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #51128:
URL: https://github.com/apache/arrow/pull/51128#issuecomment-5501326339
:warning: GitHub issue #50314 **has been automatically assigned in GitHub** to PR creator.
--
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
Re: [PR] GH-50314: [C++][Parquet] Reject invalid DELTA_BINARY_PACKED headers [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #51128:
URL: https://github.com/apache/arrow/pull/51128#issuecomment-5839455613
:warning: GitHub issue #50314 **has been automatically assigned in GitHub** to PR creator.
--
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
Re: [PR] GH-50314: [C++][Parquet] Reject invalid DELTA_BINARY_PACKED headers [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #51128:
URL: https://github.com/apache/arrow/pull/51128#issuecomment-5531130294
:warning: GitHub issue #50314 **has been automatically assigned in GitHub** to PR creator.
--
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
Re: [PR] GH-50314: [C++][Parquet] Reject invalid DELTA_BINARY_PACKED headers [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #51128:
URL: https://github.com/apache/arrow/pull/51128#discussion_r3912012359
##########
cpp/src/parquet/decoder.cc:
##########
@@ -1561,6 +1561,20 @@ class DeltaBitPackDecoder : public TypedDecoderImpl<DType> {
}
total_values_remaining_ = total_value_count_;
+ // GH-50314: mini_blocks_per_block_ comes from the page header and sizes the
+ // allocation below, while InitBlock() reads one bit-width byte per miniblock.
+ // A count larger than the bytes left can never decode, so we reject it here
+ // instead of allowing it to drive a large allocation. A page holding a single
+ // value keeps that value in the header and never calls InitBlock(), so this
Review Comment:
I think Copilot is right that we should also handle `total_value_count_ == 1` somehow @1fanwang . Perhaps in that case we should just skip the allocation?
--
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
Re: [PR] GH-50314: [C++][Parquet] Reject invalid DELTA_BINARY_PACKED headers [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #51128:
URL: https://github.com/apache/arrow/pull/51128#discussion_r3909024588
##########
cpp/src/parquet/decoder.cc:
##########
@@ -1561,6 +1561,20 @@ class DeltaBitPackDecoder : public TypedDecoderImpl<DType> {
}
total_values_remaining_ = total_value_count_;
+ // GH-50314: mini_blocks_per_block_ comes from the page header and sizes the
+ // allocation below, while InitBlock() reads one bit-width byte per miniblock.
+ // A count larger than the bytes left can never decode, so we reject it here
+ // instead of allowing it to drive a large allocation. A page holding a single
+ // value keeps that value in the header and never calls InitBlock(), so this
Review Comment:
InitHeader still allocates delta_bit_widths_ unconditionally, even when total_value_count_ == 1 (the “single value in header” case). A corrupt page can therefore still force a large allocation via mini_blocks_per_block_ without ever calling InitBlock(). Also, the new bytes_left guard should account for the mandatory min_delta field (at least 1 byte) and should use a single captured bytes_left value to avoid mismatches between the comparison and the error message.
--
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
Re: [PR] GH-50314: [C++][Parquet] Reject invalid DELTA_BINARY_PACKED headers [arrow]
Posted by "1fanwang (via GitHub)" <gi...@apache.org>.
1fanwang commented on code in PR #51128:
URL: https://github.com/apache/arrow/pull/51128#discussion_r3928069379
##########
cpp/src/parquet/decoder.cc:
##########
@@ -1561,6 +1561,20 @@ class DeltaBitPackDecoder : public TypedDecoderImpl<DType> {
}
total_values_remaining_ = total_value_count_;
+ // GH-50314: mini_blocks_per_block_ comes from the page header and sizes the
+ // allocation below, while InitBlock() reads one bit-width byte per miniblock.
+ // A count larger than the bytes left can never decode, so we reject it here
+ // instead of allowing it to drive a large allocation. A page holding a single
+ // value keeps that value in the header and never calls InitBlock(), so this
Review Comment:
Done in 21133d68.
##########
cpp/src/parquet/encoding_test.cc:
##########
@@ -1902,6 +1904,36 @@ TYPED_TEST(TestDeltaBitPackEncoding, BasicRoundTrip) {
}
}
+TYPED_TEST(TestDeltaBitPackEncoding, SingleValueRoundTrip) {
+ ASSERT_NO_FATAL_FAILURE(this->Execute(1, 1));
+}
+
+TYPED_TEST(TestDeltaBitPackEncoding, RejectsMiniblockWidthsLargerThanInput) {
+ using T = typename TypeParam::c_type;
+
+ // Header: 2^25 values per block, 2^20 miniblocks, 2 values, and first value 0,
+ // followed by min delta 0 and no miniblock bit widths.
+ const std::vector<uint8_t> encoded = {0x80, 0x80, 0x80, 0x10, 0x80,
+ 0x80, 0x40, 0x02, 0x00, 0x00};
+ ::arrow::ProxyMemoryPool pool(default_memory_pool());
+ auto decoder = MakeTypedDecoder<TypeParam>(Encoding::DELTA_BINARY_PACKED,
+ this->descr_.get(), &pool);
+ std::vector<T> decoded(2);
+
+ EXPECT_THROW_THAT(
+ [&] {
+ decoder->SetData(2, encoded.data(), static_cast<int>(encoded.size()));
+ decoder->Decode(decoded.data(), static_cast<int>(decoded.size()));
+ },
+ ParquetException,
+ ::testing::Property(
+ &ParquetException::what,
+ ::testing::HasSubstr(
+ "the number of miniblocks per block (1048576) is larger than the "
+ "number of bytes remaining in the page (1)")));
+ EXPECT_EQ(pool.bytes_allocated(), 0);
Review Comment:
Done in 21133d68.
--
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
Re: [PR] GH-50314: [C++][Parquet] Reject invalid DELTA_BINARY_PACKED headers [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #51128:
URL: https://github.com/apache/arrow/pull/51128#issuecomment-5770315398
:warning: GitHub issue #50314 **has been automatically assigned in GitHub** to PR creator.
--
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