Skip to content

fix(puffin): add missing magic number validation for puffins#2416

Merged
CTTY merged 2 commits into
apache:mainfrom
dentiny:hjiang/add-missing-puffin-validation
May 15, 2026
Merged

fix(puffin): add missing magic number validation for puffins#2416
CTTY merged 2 commits into
apache:mainfrom
dentiny:hjiang/add-missing-puffin-validation

Conversation

@dentiny
Copy link
Copy Markdown
Contributor

@dentiny dentiny commented May 9, 2026

What changes are included in this PR?

We have two functions to read puffin metadata: read and read_with_prefetch. In theory these two functions should be similarly the same apart from IO-related logic, but read_with_prefetch seems to miss magic number validation:

FileMetadata::check_magic(&first_four_bytes)?;

In terms of implementation, I thought about extracting validation+footer parse into a separate function, but the code is very simple here, don't think it necessary.

Are these changes tested?

Yes, the newly added regression test fails for the current codebase

Comment thread crates/iceberg/src/puffin/metadata.rs
Comment thread crates/iceberg/src/puffin/metadata.rs Outdated
@xanderbailey
Copy link
Copy Markdown
Contributor

Thanks for the fix @dentiny. Left a comment on the test, let me know what you think.

@dentiny dentiny requested a review from xanderbailey May 13, 2026 07:35
@xanderbailey
Copy link
Copy Markdown
Contributor

@CTTY this PR makes sense to me, would you be able to take a look when you have a minute?

Copy link
Copy Markdown
Collaborator

@CTTY CTTY left a comment

Choose a reason for hiding this comment

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

LGTM!

Interestingly, java's PuffinReader doesn't validate the MAGIC at the start of file, it only validates the start and the end of puffin footer: https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/puffin/PuffinReader.java#L68

@CTTY CTTY merged commit 3d8ab5e into apache:main May 15, 2026
20 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.

3 participants