Skip to content

Fix subtraction overflow in ExtendedTimestamp::try_from_reader #697

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
ozzieba wants to merge 1 commit into from
Closed

Fix subtraction overflow in ExtendedTimestamp::try_from_reader #697

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
29 changes: 26 additions & 3 deletions src/extra_fields/extended_timestamp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,21 +53,27 @@ impl ExtendedTimestamp {
// allow unsupported/undocumented flags

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"))?;
Comment on lines 55 to +58
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>()).

Copilot uses AI. Check for mistakes.
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
Comment on lines 55 to 79
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
        }

Expand Down Expand Up @@ -120,4 +126,21 @@ mod test {
.is_err()
);
}

#[test]
/// Ensure that a truncated extended timestamp (len too short for flags)
/// returns an error instead of panicking from a subtraction overflow.
fn test_extended_timestamp_overflow() {
use super::ExtendedTimestamp;
use std::io::Cursor;

// flags = 0x01 indicates mod_time is present, which requires 4 bytes,
// 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];
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
let data: &[u8] = &[0x01, 0x00, 0x00, 0x00];
let data: &[u8] = &[0x01, 0x00];

Copilot uses AI. Check for mistakes.
let mut cursor = Cursor::new(data);
let result = ExtendedTimestamp::try_from_reader(&mut cursor, 2);
assert!(result.is_err(), "expected error for truncated extended timestamp, got {result:?}");
}
}
Loading