Skip to content

fix(parquet/encoding): validate delta binary packed headers - #1012

Merged
zeroshade merged 2 commits into
apache:mainfrom
fallintoplace:fix/parquet-delta-header-validation
Jul 27, 2026
Merged

fix(parquet/encoding): validate delta binary packed headers#1012
zeroshade merged 2 commits into
apache:mainfrom
fallintoplace:fix/parquet-delta-header-validation

Conversation

@fallintoplace

@fallintoplace fallintoplace commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

The delta binary packed decoder accepts header combinations that do not satisfy the Parquet encoding rules. Some combinations narrow into smaller Go fields or reach decoding with invalid miniblock geometry or integer widths.

What changes are included in this PR?

  • Validate block size, miniblock divisibility, values per miniblock, and encoded value count before narrowing them.
  • Enforce INT32 ranges for the first value and minimum delta.
  • Reject used bit widths above 32 bits for INT32 and 64 bits for INT64.
  • Keep bit-width validation at use time so unused padding miniblocks remain compatible.

Are these changes tested?

Yes. Focused tests cover every header invariant, integer range, and per-type width. The full encoding and Parquet file suites pass.

Are there any user-facing changes?

INT32 files written by older arrow-go versions with non-conformant 33-bit miniblocks will now fail to read. This is intentional because those files violate the Parquet encoding rules and are also rejected by other Parquet implementations.

@fallintoplace
fallintoplace requested a review from zeroshade as a code owner July 25, 2026 16:04
@fallintoplace
fallintoplace force-pushed the fix/parquet-delta-header-validation branch from 799a0fa to d178c50 Compare July 25, 2026 16:10
@zeroshade

Copy link
Copy Markdown
Member

This has gone stale — #1027 landed on main a short while ago and implements the same physical-type-width delta wraparound as the encoder half of this PR, so GitHub now reports this as conflicting.

The header-validation half is still valuable and I would like to take it. On main the decoder accepts blockSize == 0 and an unbounded miniBlocksPerBlock, and the deltaBitWidths.Resize(int(d.miniBlocksPerBlock)) on an attacker-controlled count is a real allocation hazard on untrusted files; the block-size/miniblock divisibility and INT32 range checks are all correct and I could not construct a valid input they reject.

Could you rebase onto main and drop whatever the #1027 change already covers, keeping the SetData/initBlock/unpackNextMini validation and the used-bit-width check? One thing worth calling out in the description when you do: rejecting used bit widths above 32 for INT32 means files written by older arrow-go versions that emitted pathological 33-bit miniblocks will now fail to read. I believe that is correct, since those files are non-conformant and other Parquet implementations reject them too, but it should be explicit.

@fallintoplace
fallintoplace force-pushed the fix/parquet-delta-header-validation branch from b9fa1a5 to 6fdfee2 Compare July 27, 2026 17:40

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the rebase — this is exactly the right scope now. I confirmed the encoder Put() wraparound and its encoding_test.go case are gone, since #1027 already covers that on main, leaving just the header validation here.

Re-verified the remaining changes against current main: blockSize == 0 and an unbounded miniBlocksPerBlock were both accepted, and deltaBitWidths.Resize(int(d.miniBlocksPerBlock)) on an attacker-controlled count is a genuine allocation hazard on untrusted files. The block-size/miniblock divisibility rules, the values-per-miniblock multiple-of-32 check, and the INT32 range checks on the first value and min delta all match the Parquet spec, and I could not construct a valid input that any of them reject. Keeping the used-bit-width check at use time is the right call so unused padding miniblocks stay compatible. LGTM.

@zeroshade
zeroshade merged commit c806e6f into apache:main Jul 27, 2026
40 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants