Skip to content

fix(parquet/encoding): reject invalid delta byte prefixes - #1014

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

fix(parquet/encoding): reject invalid delta byte prefixes#1014
zeroshade merged 2 commits into
apache:mainfrom
fallintoplace:fix/parquet-delta-prefix-validation

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Rationale for this change

DELTA_BYTE_ARRAY prefix lengths are used directly as slice bounds. Negative prefixes or prefixes longer than the previous value therefore panic. The decoder also retains its previous value when reused for a new page, even though each page starts an independent delta sequence.

What changes are included in this PR?

  • Reset the previous value in SetData.
  • Propagate prefix stream decode errors and short reads.
  • Require the first prefix to be zero and validate later prefixes before slicing.
  • Apply the same validation to decode and discard paths.

Are these changes tested?

Yes. Focused tests cover decoder reuse, negative prefixes, and prefixes longer than the previous value.

@fallintoplace
fallintoplace requested a review from zeroshade as a code owner July 25, 2026 16:07
@fallintoplace
fallintoplace force-pushed the fix/parquet-delta-prefix-validation branch from 7d3de32 to bf7624b Compare July 25, 2026 16:12

@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.

Verified two distinct real bugs here:

  1. `SetData` never reset `lastVal`, so a decoder reused across pages skipped first-value initialization and consumed `prefixLengths[0]` as a continuation, corrupting the first value of the second page. Column readers do reuse decoders across pages, so this is reachable.
  2. `d.lastVal[:prefixLen:prefixLen]` panicked on a negative prefix or one longer than the previous value, and the loop could index an empty `prefixLengths`.

Requiring the first prefix to be zero matches the DELTA_BYTE_ARRAY spec, and the validation is applied consistently to both the decode and discard paths. LGTM.

Note: #1013 modifies the same `SetData` and will need a rebase now that this has landed.

@zeroshade
zeroshade merged commit 03ce2f4 into apache:main Jul 27, 2026
23 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