Skip to content

fix(parquet): validate DataPageV2 decoded length#911

Merged
zeroshade merged 3 commits into
apache:mainfrom
fallintoplace:fix-parquet-v2-decoded-length
Jul 9, 2026
Merged

fix(parquet): validate DataPageV2 decoded length#911
zeroshade merged 3 commits into
apache:mainfrom
fallintoplace:fix-parquet-v2-decoded-length

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

What changed

  • Make DataPageV2 V2 readers verify decoded byte counts instead of only checking buffer size.
  • Update readV2Encrypted/readV2Unencrypted to return the actual number of bytes written/read and validate lengths.
  • Enforce decoded-length validation in �6 uncompressed path and encrypted/unencrypted paths.
  • Add regression tests for:
    • compressed DataPageV2 with mismatched codec decode length
    • encrypted uncompressed DataPageV2 with short decrypted output

Validation

  • Added focused regression coverage in .
  • No functional changes to non-V2 paths.

@fallintoplace fallintoplace requested a review from zeroshade as a code owner July 5, 2026 18:50

@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 adding the decoded-length validation — the goal is good, and the encrypted path plus the caller-level got != lenUncompressed check are correct. But readV2Unencrypted has an accumulation bug that regresses the happy path for compressed V2 pages (details inline). Requesting changes on that one check, plus a happy-path test to lock it in. Everything else looks fine.

Comment thread parquet/file/page_reader.go Outdated
}
return nil
decodedLen += len(values)
if len(values) != lenUncompressed-decodedLen {

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.

This is the blocking issue. decodedLen += len(values) on the line above already makes decodedLen == levelsBytelen + len(values), so lenUncompressed - decodedLen here reduces to lenUncompressed - levelsBytelen - len(values) — which is 0 for any valid page. That turns this into len(values) != 0, i.e. it rejects every non-empty compressed DataPageV2. (The encrypted sibling gets it right: len(decoded) != lenUncompressed-levelsBytelen.)

Suggested fix:

decodedLen += len(values)
if decodedLen != lenUncompressed {
    return decodedLen, fmt.Errorf("parquet: metadata said %d bytes uncompressed data page, got %d bytes", lenUncompressed, decodedLen)
}

I verified on this branch with a valid compressed V2 page (Snappy, no levels): Next() returns false with the self-contradictory parquet: metadata said 128 bytes uncompressed data page, got 128 bytes. The new tests here don't catch it because they only exercise the mismatched and encrypted paths — please also add a happy-path test for a valid compressed DataPageV2, since the existing TestDataPageV2 is uncompressed and TestCompression is V1-only.

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

The compressed-V2 regression is fixed — readV2Unencrypted now checks decodedLen != lenUncompressed, and the new TestDataPageV2Compressed guards the happy path. Verified locally: that test passes (valid compressed V2 now reads, Data() round-trips) while TestDataPageV2CorruptDecodedLength and the encrypted-mismatch test still error correctly, and the full parquet/file package is green. Thanks for the quick turnaround — LGTM.

@zeroshade zeroshade merged commit cafb660 into apache:main Jul 9, 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