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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
