Skip to content

fix(puffin): surface LZ4 content-checksum error regardless of JSON decoder read-ahead #2069

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nssalian wants to merge 1 commit into from
Closed

fix(puffin): surface LZ4 content-checksum error regardless of JSON decoder read-ahead #2069

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions puffin/puffin_reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,10 @@ func (r *Reader) readFooter() error {
return fmt.Errorf("puffin: read buffered footer JSON: %w", err)
}
if len(bytes.TrimSpace(buffered)) > 0 {
if compressedFooter != nil && compressedFooter.err != nil {

@laskoviymishka laskoviymishka Sep 30, 2026

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.

The checksum error only survives if every trailing-content return remembers to check compressedFooter.err, and we've now had to patch that in after the fact twice. Add a fourth trailing-content return down the line and it silently drops the error again, which is the exact regression this PR is closing. I'd move the check to a single site right after Decode returns nil, before any of the trailing-content logic, so it's structural instead of something each new return has to remember. A small checkCompressedFooterErr(compressedFooter) helper is a fine alternative if you'd rather keep the per-site shape.

Minor and moot if you collapse to the single check: inside this limitedFooter != nil && N == 0 branch compressedFooter is always non-nil (both are only set past the compressed-footer gate), so the != nil arm here is dead; it's only load-bearing at the sites outside that branch.

return fmt.Errorf("puffin: read compressed footer: %w", compressedFooter.err)
}

return errors.New("puffin: unexpected content after footer JSON")
}
}
Expand All @@ -510,6 +514,10 @@ func (r *Reader) readFooter() error {
// content deliberately, even though some other Iceberg implementations
// accept padding or additional values inside the footer payload.
if decoder.More() {
if compressedFooter != nil && compressedFooter.err != nil {

@laskoviymishka laskoviymishka Sep 30, 2026

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.

Neither of these new branches is exercised by the current test. TestReaderPreservesLZ4ChecksumError runs with the default 64MB max footer, so the limitedFooter.N == 0 branch at line 505 is never hit, and the assertion is only on the invalid frame checksum substring, so it can't tell which site actually surfaced the error (Site 3's pre-existing Token() guard would satisfy it just as well). I'd add a case with WithMaxFooterSize(int64(len(payload))) and a corrupted checksum, and pin the full wrapped prefix, so we know these additions are what's covering the error rather than the guard that was already there.

return fmt.Errorf("puffin: read compressed footer: %w", compressedFooter.err)
}

return errors.New("puffin: unexpected content after footer JSON")
}
if _, err := decoder.Token(); !errors.Is(err, io.EOF) {
Expand Down
Loading