GH-50314: [C++][Parquet] Reject invalid DELTA_BINARY_PACKED headers - #51128
GH-50314: [C++][Parquet] Reject invalid DELTA_BINARY_PACKED headers#511281fanwang wants to merge 2 commits into
Conversation
…ders InitHeader() sizes the bit-width buffer from the header's miniblock count without tying it to the page size, so a 10-byte page claiming 2^20 miniblocks allocates 1 MiB before failing. InitBlock() reads one bit-width byte per miniblock, so such a page can never decode. Generated-by: GitHub Copilot CLI (Claude Opus 5) Signed-off-by: 1fanwang <1fannnw@gmail.com>
|
|
github-actions
Bot
added
Component: Parquet
Component: C++
awaiting review
There was a problem hiding this comment.
🟡 Changes recommended
The decoder change still allocates miniblock scratch space unconditionally (including for single-value pages) and the new guard should account for required min_delta_ bytes, leaving a remaining allocation-DoS gap that should be closed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens the C++ Parquet DELTA_BINARY_PACKED decoder against corrupt page headers that can otherwise drive disproportionate memory allocations and produce misleading EOF errors, aligning behavior with the security/robustness goals described in GH-50314.
Changes:
- Add header validation in
DeltaBitPackDecoder::InitHeaderto reject pages whose miniblock count is incompatible with the remaining input bytes and emit a more actionableParquetException. - Add new encoding tests to cover the single-value page path and the corrupt-header rejection case (including allocation behavior via
ProxyMemoryPool).
File summaries
| File | Description |
|---|---|
| cpp/src/parquet/decoder.cc | Adds early validation/error reporting for invalid DELTA_BINARY_PACKED miniblock headers (and aims to prevent oversized allocations). |
| cpp/src/parquet/encoding_test.cc | Adds regression tests for single-value decoding and for rejecting invalid miniblock-width headers without allocating. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // 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 |
There was a problem hiding this 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?
github-actions
Bot
added
awaiting committer review
| ::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); |
There was a problem hiding this comment.
bytes_allocated is the number of bytes currently allocated. Do we want to use total_bytes_allocated instead?
Single-value pages never initialize a block, so leave the bit-width buffer unallocated. Account for the required min-delta byte when validating block metadata, and use cumulative allocation counts in the regression tests. Generated-by: GitHub Copilot CLI (Claude Opus 5) Signed-off-by: 1fanwang <1fannnw@gmail.com>
|
|
There was a problem hiding this comment.
🟢 Approval recommended
The changes directly address the allocation-before-validation issue with clear guards and are covered by focused regression tests.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Rationale for this change
A corrupt
DELTA_BINARY_PACKEDpage can encode a miniblock count far larger than the page itself. The decoder allocated that buffer before discovering the input was incomplete. A single-value page never needs a miniblock buffer but still used the untrusted count.What changes are included in this PR?
The decoder now:
Are these changes tested?
cmake --build cpp/build-review --target parquet-encoding-test -j 8 cpp/build-review/debug/parquet-encoding-test \ --gtest_filter='*SingleValueSkipsMiniblockAllocation*:*RejectsMiniblockWidthsLargerThanInput*:*RejectsMiniblockWidthsWithoutMinDelta*' cpp/build-review/debug/parquet-encoding-testRaw logs
Are there any user-facing changes?
Invalid pages fail before allocation with an error that names the impossible miniblock count and available bytes. Valid single-value pages decode without allocating a miniblock buffer.
GitHub Issue: #50314