Skip to content

fix(puffin): return DataInvalid instead of panicking on malformed footer length#2867

Open
arpitjain099 wants to merge 1 commit into
apache:mainfrom
arpitjain099:fix/puffin-metadata-length-underflow
Open

fix(puffin): return DataInvalid instead of panicking on malformed footer length#2867
arpitjain099 wants to merge 1 commit into
apache:mainfrom
arpitjain099:fix/puffin-metadata-length-underflow

Conversation

@arpitjain099

Copy link
Copy Markdown

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.

…ter length

Reading the footer of a Puffin file subtracts lengths from the file size
without checking that the file is large enough. Two cases underflow:

- read_footer_payload_length subtracts FOOTER_STRUCT_LENGTH from the file
  size, so any file shorter than 12 bytes underflows.
- read_footer_bytes subtracts footer_length, which is derived from the
  footer_payload_length u32 read out of the file itself, so a declared
  payload length larger than the file underflows.

Both panic with subtract-with-overflow in debug builds and wrap to a bogus
read range in release builds. Add a minimum file length check and use
checked_sub, returning ErrorKind::DataInvalid like the neighbouring magic
and bounds checks already do.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
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.

1 participant