Fix subtraction overflow in ExtendedTimestamp::try_from_reader#697
Fix subtraction overflow in ExtendedTimestamp::try_from_reader#697ozzieba wants to merge 1 commit intozip-rs:masterfrom
Conversation
Replace unchecked `bytes_to_read -= size_of::<u32>()` with `checked_sub` that returns `ZipError::InvalidArchive` instead of panicking on malformed ZIP files where the extended timestamp extra field length is inconsistent with the flags byte. While the existing validation check (`len != 5 && len != 1 + 4 * flags.count_ones()`) catches most inconsistent inputs before reaching the subtraction, using checked arithmetic provides defense-in-depth against future refactors that might weaken the validation, and is the idiomatic Rust approach for arithmetic on untrusted input. Fixes zip-rs#696
Summary of ChangesHello @ozzieba, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness of the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Could you provide a file example Thanks |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a potential panic due to subtraction overflow by using checked_sub. The change is robust and includes a new unit test to verify the fix. I have provided one suggestion to refactor the timestamp parsing logic to improve code clarity and maintainability by reducing duplication.
| let mod_time = if (flags & 0b0000_0001_u8 == 0b0000_0001_u8) || len == 5 { | ||
| bytes_to_read -= size_of::<u32>(); | ||
| bytes_to_read = bytes_to_read | ||
| .checked_sub(size_of::<u32>()) | ||
| .ok_or(invalid!("Extended timestamp field too short for mod_time"))?; | ||
| Some(reader.read_u32_le()?) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let ac_time = if flags & 0b0000_0010_u8 == 0b0000_0010_u8 && len > 5 { | ||
| bytes_to_read -= size_of::<u32>(); | ||
| bytes_to_read = bytes_to_read | ||
| .checked_sub(size_of::<u32>()) | ||
| .ok_or(invalid!("Extended timestamp field too short for ac_time"))?; | ||
| Some(reader.read_u32_le()?) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let cr_time = if flags & 0b0000_0100_u8 == 0b0000_0100_u8 && len > 5 { | ||
| bytes_to_read -= size_of::<u32>(); | ||
| bytes_to_read = bytes_to_read | ||
| .checked_sub(size_of::<u32>()) | ||
| .ok_or(invalid!("Extended timestamp field too short for cr_time"))?; | ||
| Some(reader.read_u32_le()?) | ||
| } else { | ||
| None |
There was a problem hiding this comment.
While the change is correct, there is significant code duplication in parsing the mod_time, ac_time, and cr_time. This can be refactored to improve readability and maintainability by defining constants for the flags and using a helper closure to read the timestamp values.
const MOD_TIME_FLAG: u8 = 1;
const AC_TIME_FLAG: u8 = 2;
const CR_TIME_FLAG: u8 = 4;
let mut read_timestamp = |field_name: &'static str| -> ZipResult<u32> {
bytes_to_read = bytes_to_read
.checked_sub(std::mem::size_of::<u32>())
.ok_or(invalid!(
"Extended timestamp field too short for {}",
field_name
))?;
reader.read_u32_le()
};
let mod_time = if (flags & MOD_TIME_FLAG != 0) || len == 5 {
Some(read_timestamp("mod_time")?)
} else {
None
};
let ac_time = if (flags & AC_TIME_FLAG != 0) && len > 5 {
Some(read_timestamp("ac_time")?)
} else {
None
};
let cr_time = if (flags & CR_TIME_FLAG != 0) && len > 5 {
Some(read_timestamp("cr_time")?)
} else {
None
}There was a problem hiding this comment.
Pull request overview
Hardens parsing of the ZIP “extended timestamp” extra field to avoid panics on malformed input by replacing unchecked arithmetic with checked underflow handling, and adds a regression test around truncated inputs.
Changes:
- Replace three
bytes_to_read -= ...subtractions withchecked_sub(...)that returns an error on underflow. - Add a unit test covering a truncated/invalid extended timestamp extra field.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mod_time = if (flags & 0b0000_0001_u8 == 0b0000_0001_u8) || len == 5 { | ||
| bytes_to_read -= size_of::<u32>(); | ||
| bytes_to_read = bytes_to_read | ||
| .checked_sub(size_of::<u32>()) | ||
| .ok_or(invalid!("Extended timestamp field too short for mod_time"))?; |
There was a problem hiding this comment.
size_of::<u32>() is used here but size_of isn’t in scope in this module, which will fail to compile. Import it (e.g., use core::mem::size_of;) or fully-qualify the calls (e.g., core::mem::size_of::<u32>()).
| // but len = 2 only provides 1 byte after the flags byte. | ||
| // The validation check catches this before the subtraction, but even | ||
| // if validation were removed, checked_sub would catch it. | ||
| let data: &[u8] = &[0x01, 0x00, 0x00, 0x00]; |
There was a problem hiding this comment.
This test’s comment says len = 2 “only provides 1 byte after the flags byte”, but data currently contains 4 bytes total. Consider making the buffer length consistent with the declared len (e.g., only flags + 1 byte) so the test actually represents a truncated extra field and won’t accidentally pass if code starts reading past len.
| let data: &[u8] = &[0x01, 0x00, 0x00, 0x00]; | |
| let data: &[u8] = &[0x01, 0x00]; |
Summary
Fixes #696 --
ExtendedTimestamp::try_from_readercan panic with "attempt to subtract with overflow" when processing malformed ZIP files where the extended timestamp extra field length is inconsistent with the flags byte.Changes
bytes_to_read -= size_of::<u32>()subtractions withchecked_subthat returnsZipError::InvalidArchiveon underflowtest_extended_timestamp_overflowthat verifies truncated extended timestamp fields return an error instead of panickingDetails
The existing validation check (
len != 5 && len != 1 + 4 * flags.count_ones()) catches most inconsistent inputs before reaching the subtraction. However, using checked arithmetic provides defense-in-depth against:len == 5bypass and flag-based branchingThis is the idiomatic Rust approach for arithmetic on untrusted input.
Test plan
cargo test --lib extra_fields::extended_timestamp-- both existing and new test passcargo test-- full test suite passes (189 tests, 0 failures)