Skip to content

GH-50314: [C++][Parquet] Reject invalid DELTA_BINARY_PACKED headers - #51128

Open
1fanwang wants to merge 3 commits into
apache:mainfrom
1fanwang:1fannnw/reject-invalid-delta-headers
Open

GH-50314: [C++][Parquet] Reject invalid DELTA_BINARY_PACKED headers#51128
1fanwang wants to merge 3 commits into
apache:mainfrom
1fanwang:1fannnw/reject-invalid-delta-headers

Conversation

@1fanwang

@1fanwang 1fanwang commented Sep 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Rationale for this change

A corrupt DELTA_BINARY_PACKED page can ask for a miniblock buffer much larger than the page. A single-value page never needs that buffer. A truncated page whose min delta spans more than one byte could still allocate, then fail with Decode bit-width EOF.

Fixes #50314.

What changes are included in this PR?

Skip the bit-width buffer for single-value pages. For every other page, read min delta first, then reject an impossible miniblock count before allocating.

Are these changes tested?

The decoder tests cover a single-value page with an oversized miniblock count, a one-byte leftover after the header, and a two-byte min delta with no bit-width byte.

./build/release/parquet-encoding-test --gtest_filter='TestDeltaBitPackEncoding*'

Without the min-delta check, the two-byte case failed with:

Actual: Unexpected end of stream: Decode bit-width EOF
pool.total_bytes_allocated() Which is: 64

With it:

[==========] 18 tests from 2 test suites ran. (111 ms total)
[  PASSED  ] 18 tests.

The rest of parquet-encoding-test also passed. In-memory column reader tests passed. File-backed reader tests need PARQUET_TEST_DATA and were not run here.

Are there any user-facing changes?

Invalid headers fail before allocation. Valid single-value pages decode without a miniblock buffer.

New Contributor's Guide |
Contributing Overview |
AI-generated Code Guidance

Was AI used for this PR?

AI assisted with the code, regression tests, and description.

PR code and description written by:

  • Human
  • AI

Reviewed before submission by:

  • Human
  • AI
  • Not reviewed

Copilot AI lite review requested due to automatic review settings September 1, 2026 22:32
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

⚠️ GitHub issue #50314 has been automatically assigned in GitHub to PR creator.

Copilot AI reviewed Sep 1, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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::InitHeader to reject pages whose miniblock count is incompatible with the remaining input bytes and emit a more actionable ParquetException.
  • 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.

Comment thread cpp/src/parquet/decoder.cc Outdated
@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 2, 2026
pitrou
pitrou reviewed Sep 2, 2026
View reviewed changes
Comment thread cpp/src/parquet/encoding_test.cc Outdated
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

⚠️ GitHub issue #50314 has been automatically assigned in GitHub to PR creator.

Copilot AI review requested due to automatic review settings September 3, 2026 19:40
Copilot AI reviewed Sep 3, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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

@github-actions

github-actions Bot commented Sep 22, 2026

Copy link
Copy Markdown

⚠️ GitHub issue #50314 has been automatically assigned in GitHub to PR creator.

Copilot AI review requested due to automatic review settings September 22, 2026 02:24
@1fanwang
1fanwang requested a review from HuaHuaY as a code owner September 22, 2026 02:24
Copilot AI reviewed Sep 22, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Header validation can still allow allocation when min_delta_ uses multiple bytes and width data is truncated.

Review effort: Lite
Findings: None

Resolved since last review (1)

Copilot AI review requested due to automatic review settings September 24, 2026 06:31
@1fanwang
1fanwang force-pushed the 1fannnw/reject-invalid-delta-headers branch from bb561d0 to 18ae360 Compare September 24, 2026 06:31
Copilot AI reviewed Sep 24, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

Remaining feedback is limited to non-blocking test-name nits.

Review effort: Lite
Findings: None

@1fanwang
apacheGH-50314: [C++][Parquet] Reject invalid DELTA_BINARY_PACKED hea…
0b01abb 
…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.

Signed-off-by: 1fanwang <1fannnw@gmail.com>
@1fanwang
apacheGH-50314: [C++][Parquet] Avoid single-value miniblock allocation
5e626ba 
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.

Signed-off-by: 1fanwang <1fannnw@gmail.com>
@1fanwang
apacheGH-50314: [C++][Parquet] Size miniblock buffer after min delta
fca97c6 
InitHeader subtracted one byte for min delta, so a two-byte min delta
with no bit widths still allocated the aligned scratch buffer and then
failed with Decode bit-width EOF. Consume min delta first, then reject
when remaining bytes cannot hold the declared miniblock widths.

Signed-off-by: 1fanwang <1fannnw@gmail.com>
Copilot AI review requested due to automatic review settings September 25, 2026 20:52
@1fanwang
1fanwang force-pushed the 1fannnw/reject-invalid-delta-headers branch from 18ae360 to fca97c6 Compare September 25, 2026 20:52
@github-actions

github-actions Bot commented Sep 25, 2026

Copy link
Copy Markdown

⚠️ GitHub issue #50314 has been automatically assigned in GitHub to PR creator.

Copilot AI reviewed Sep 25, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The changes are covered by regression tests, with only a minor diagnostic wording nit remaining.

Review effort: Lite
Findings: None

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pitrou pitrou pitrou left review comments
Copilot code review Copilot
Copilot review effort, defaults to Lite
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@wgtmac wgtmac Awaiting requested review from wgtmac wgtmac is a code owner
@HuaHuaY HuaHuaY Awaiting requested review from HuaHuaY HuaHuaY is a code owner

Assignees

No one assigned

Labels

awaiting committer review Awaiting committer review Component: C++ Component: Parquet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[C++][Parquet] Reject outlandish values in DELTA_BINARY_PACKED decoder

3 participants

@1fanwang @pitrou