You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "arpitjain099 (via GitHub)" <gi...@apache.org> on 2026/07/21 18:20:53 UTC
[PR] fix(puffin): return DataInvalid instead of panicking on malformed footer length [iceberg-rust]
arpitjain099 opened a new pull request, #2867:
URL: https://github.com/apache/iceberg-rust/pull/2867
## What changes are included in this PR?
`FileMetadata::read` locates the footer by subtracting from the file size, but never checks the file is big enough first. Two subtractions can underflow:
- `read_footer_payload_length` does `input_file_length - FOOTER_STRUCT_LENGTH`, so any file shorter than 12 bytes underflows.
- `read_footer_bytes` does `input_file_length - footer_length`, where `footer_length` is derived from `footer_payload_length`, the u32 read out of the FooterPayloadSize field of the file being parsed. A file that declares a payload larger than itself underflows here.
In debug builds both panic with "attempt to subtract with overflow". In release the subtraction wraps and you get a bogus read range instead.
Worth saying up front: I'm treating this as robustness, not a security issue. Puffin files are table statistics written by the engines that already write the table, so I'm not claiming a meaningful trust boundary. The point is narrower - a truncated or corrupt file should surface as a `DataInvalid` error, the way the magic checks and the bounds checks in `decode_flags` / `extract_footer_payload_as_str` already do in this same file, rather than panicking on the caller.
The fix adds a `MIN_FILE_LENGTH` check in `read` and switches the second subtraction to `checked_sub`, both returning `ErrorKind::DataInvalid`.
I left `read_with_prefetch` alone. Its `prefetch_hint > 16` and `prefetch_hint <= input_file_length` guards already keep its own slicing in range, and when the declared footer is larger than the hint it falls back to `read`, which is now checked.
## Are these changes tested?
Two unit tests, one per case. Both panic before the fix:
```
thread 'puffin::metadata::tests::test_file_shorter_than_minimum_length_returns_error'
panicked at crates/iceberg/src/puffin/metadata.rs:201:21:
attempt to subtract with overflow
thread 'puffin::metadata::tests::test_footer_payload_length_larger_than_file_returns_error'
panicked at crates/iceberg/src/puffin/metadata.rs:218:21:
attempt to subtract with overflow
```
After the fix, `cargo test -p iceberg --lib puffin` gives 39 passed, 0 failed. Full crate `cargo test -p iceberg --lib` is 1444 passed, 0 failed, and `cargo clippy -p iceberg --all-targets` is clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(puffin): return DataInvalid instead of panicking on malformed footer length [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk merged PR #2867:
URL: https://github.com/apache/iceberg-rust/pull/2867
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(puffin): return DataInvalid instead of panicking on malformed footer length [iceberg-rust]
Posted by "arpitjain099 (via GitHub)" <gi...@apache.org>.
arpitjain099 commented on PR #2867:
URL: https://github.com/apache/iceberg-rust/pull/2867#issuecomment-5088188637
Thanks for the thorough review, and for actually reproducing both overflows rather than taking my word for it.
On the ordering point, that was deliberate: a file shorter than `MIN_FILE_LENGTH` can't hold valid magic anyway, so "too short" is the accurate diagnosis, and checking length first is what avoids reading out of range to discover the magic is bad. Happy to flip it if you'd rather short-but-garbage files keep reporting "bad magic", but I think the current order gives the more useful error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org