fix(puffin): return DataInvalid instead of panicking on malformed footer length#2867
Open
arpitjain099 wants to merge 1 commit into
Open
fix(puffin): return DataInvalid instead of panicking on malformed footer length#2867arpitjain099 wants to merge 1 commit into
arpitjain099 wants to merge 1 commit into
Conversation
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes are included in this PR?
FileMetadata::readlocates the footer by subtracting from the file size, but never checks the file is big enough first. Two subtractions can underflow:read_footer_payload_lengthdoesinput_file_length - FOOTER_STRUCT_LENGTH, so any file shorter than 12 bytes underflows.read_footer_bytesdoesinput_file_length - footer_length, wherefooter_lengthis derived fromfooter_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
DataInvaliderror, the way the magic checks and the bounds checks indecode_flags/extract_footer_payload_as_stralready do in this same file, rather than panicking on the caller.The fix adds a
MIN_FILE_LENGTHcheck inreadand switches the second subtraction tochecked_sub, both returningErrorKind::DataInvalid.I left
read_with_prefetchalone. Itsprefetch_hint > 16andprefetch_hint <= input_file_lengthguards already keep its own slicing in range, and when the declared footer is larger than the hint it falls back toread, which is now checked.Are these changes tested?
Two unit tests, one per case. Both panic before the fix:
After the fix,
cargo test -p iceberg --lib puffingives 39 passed, 0 failed. Full cratecargo test -p iceberg --libis 1444 passed, 0 failed, andcargo clippy -p iceberg --all-targetsis clean.