Panic `attempt to subtract with overflow` inside `src/extra_fields/extended_timestamp.rs`

[qarmin]

Describe the bug
Code

fn check_file(file_path: &str) {
    let Ok(content) = fs::read(file_path) else {
        return;
    };
    let cursor = std::io::Cursor::new(content);
    let mut zip = match zip::ZipArchive::new(cursor) {
        Ok(t) => t,
        Err(e) => {
            eprintln!("{e}");
            return;
        }
    };

    for i in 0..zip.len() {
        match zip.by_index(i) {
            Ok(mut file) => {
                let mut buf = Vec::new();
                let _ = file.read(&mut buf);
            }
            Err(e) => {
                eprintln!("{e}");
            }
        }
    }
}

when checking file:

problematic_file.zip

will panic with info:

thread 'main' (80946) panicked at /home/runner/.cargo/git/checkouts/zip2-30411767e68ca417/54c634b/src/extra_fields/extended_timestamp.rs:26:9:
attempt to subtract with overflow
stack backtrace:
   0: __rustc::rust_begin_unwind
             at /rustc/07d246fc6dc227903da2955b38a59e060539a485/library/std/src/panicking.rs:698:5
   1: core::panicking::panic_fmt
             at /rustc/07d246fc6dc227903da2955b38a59e060539a485/library/core/src/panicking.rs:75:14
   2: core::panicking::panic_const::panic_const_sub_overflow
             at /rustc/07d246fc6dc227903da2955b38a59e060539a485/library/core/src/panicking.rs:175:17
   3: zip::extra_fields::extended_timestamp::ExtendedTimestamp::try_from_reader
             at /home/runner/.cargo/git/checkouts/zip2-30411767e68ca417/54c634b/src/extra_fields/extended_timestamp.rs:26:9
   4: zip::read::parse_single_extra_field
             at /home/runner/.cargo/git/checkouts/zip2-30411767e68ca417/54c634b/src/read.rs:1468:17
   5: zip::read::parse_extra_field
             at /home/runner/.cargo/git/checkouts/zip2-30411767e68ca417/54c634b/src/read.rs:1377:22
   6: zip::read::central_header_to_zip_file_inner
             at /home/runner/.cargo/git/checkouts/zip2-30411767e68ca417/54c634b/src/read.rs:1342:11
   7: zip::read::central_header_to_zip_file
             at /home/runner/.cargo/git/checkouts/zip2-30411767e68ca417/54c634b/src/read.rs:1249:16
   8: zip::read::<impl zip::read::zip_archive::ZipArchive<R>>::read_central_header
             at /home/runner/.cargo/git/checkouts/zip2-30411767e68ca417/54c634b/src/read.rs:692:24
   9: zip::read::<impl zip::read::zip_archive::ZipArchive<R>>::get_metadata::{{closure}}
             at /home/runner/.cargo/git/checkouts/zip2-30411767e68ca417/54c634b/src/read.rs:654:34
  10: core::result::Result<T,E>::and_then
             at /home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/result.rs:1483:22
  11: zip::read::<impl zip::read::zip_archive::ZipArchive<R>>::get_metadata
             at /home/runner/.cargo/git/checkouts/zip2-30411767e68ca417/54c634b/src/read.rs:654:18
  12: zip::read::<impl zip::read::zip_archive::ZipArchive<R>>::with_config
             at /home/runner/.cargo/git/checkouts/zip2-30411767e68ca417/54c634b/src/read.rs:752:22
  13: zip::read::<impl zip::read::zip_archive::ZipArchive<R>>::new
             at /home/runner/.cargo/git/checkouts/zip2-30411767e68ca417/54c634b/src/read.rs:745:9
  14: zip_rs::check_file
             at ./src/crates/zip_rs/src/main.rs:31:25
  15: zip_rs::main
             at ./src/crates/zip_rs/src/main.rs:23:9
  16: core::ops::function::FnOnce::call_once
             at /home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:253:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Desktop (please complete the following information):

• OS: [e.g. iOS] Ubuntu 25.04(but not really needed)

[amazon-q-developer]

Progress Updates

• I need to explore the repository structure to understand the codebase and locate the problematic file mentioned in the panic trace: src/extra_fields/extended_timestamp.rs at line 26.
• I can see the repository structure. Now I need to examine the problematic file src/extra_fields/extended_timestamp.rs to understand the overflow issue at line 26.
• I found the problematic line 26 where the overflow occurs: bytes_to_read -= flags.len();. The issue is that bytes_to_read is calculated from len as usize and then we subtract flags.len() which is 1. If len is 0, this would cause an underflow. Let me analyze the code more carefully to understand the issue.
• Analyzing the overflow issue in extended_timestamp.rs
• I've identified the issue: when len is 0, the code tries to subtract 1 from 0 causing an underflow. I need to add a check to ensure len is at least 1 before proceeding with the flag reading. Let me implement the fix.
• Good! I've added a check to prevent the overflow by returning an error when len is 0. Now let me also check if there are any other potential underflow issues in the same function, particularly in the sections where we subtract size_of::<u32>() from bytes_to_read.
• I can see there are more potential underflow issues at lines 55, 62, and 69 where size_of::<u32>() (which is 4) is subtracted from bytes_to_read. I need to add checks to ensure bytes_to_read is sufficient before these subtractions to prevent underflows.
• Perfect! I've added overflow protection checks for all the subtraction operations in the function. Now let me check if I need to import size_of since it's being used in the code.
• I need to add the import for size_of since it's being used but not imported. Let me check the current imports and add the necessary one.
• Great! Now let me view the complete modified file to ensure all changes are correct and the code looks good.
• Perfect! Now let me create a simple test to verify that the fix works correctly. I'll look for existing tests to understand the testing pattern used in this codebase.
• Now I'll create a test to verify that our fix handles the overflow case correctly. I'll add a test that simulates the problematic scenario where the extended timestamp field has a length of 0.
• Excellent! I've implemented comprehensive fixes for the overflow issue and added tests to verify the behavior. Let me now create a plan to summarize what I've done and verify that the solution is complete.
• Verify that the overflow fix in extended_timestamp.rs is complete and comprehensive
• Based on the plan analysis, I should verify that my fix is complete by checking other extra field parsers for similar issues and ensuring error message consistency. Let me examine the other extra field implementations and the calling code.
• Great! I can see that the zipinfo_utf8.rs already uses proper bounds checking with checked_sub() method, which is a safer approach than my manual checks. The NTFS parser uses fixed-length validation. Let me check the calling code in read.rs to see how errors are handled.
• Perfect! I can see that the calling code at line 1468 properly handles errors from ExtendedTimestamp::try_from_reader with the ? operator, so my error returns will be properly propagated. I also notice that the ZIP64 parser uses checked_sub() like the Unicode parser. Let me improve my implementation to use the same pattern for consistency.
• Excellent! I've updated the implementation to use checked_sub() which is more idiomatic and consistent with the rest of the codebase. Now let me also improve the initial flags subtraction to use the same pattern for complete consistency.
• Perfect! Now I have a completely consistent implementation using checked_sub() throughout. However, I realize that the initial check for len == 0 is now redundant since the checked_sub() will handle that case. Let me clean this up for better code clarity.
• Now I need to update my test to match the new error message since I changed it from "extended timestamp field cannot be empty" to "extended timestamp field is too small for flags".
• Perfect! Now let me view the final implementation to ensure everything looks good and then create a summary of the changes made.
• Summarizing the comprehensive fix for the overflow issue

---

✅ I finished the proposed code changes, and the pull request is ready for review: #406.

Comment on the pull request to provide feedback and request a new iteration.

To provide feedback on how Amazon Q Developer performed with code generation, create an issue in the Amazon Q Developer public repository.

[Its-Just-Nans]

I think this was fixed in 97c33a6ce

fn main() {
    use std::{fs, io::Read, path::PathBuf};
    let file_path = PathBuf::from("problematic_file.zip");
    let Ok(content) = fs::read(file_path) else {
        return;
    };
    let cursor = std::io::Cursor::new(content);
    let mut zip = match zip::ZipArchive::new(cursor) {
        Ok(t) => t,
        Err(e) => {
            eprintln!("{e}");
            return;
        }
    };

    for i in 0..zip.len() {
        match zip.by_index(i) {
            Ok(mut file) => {
                let mut buf = Vec::new();
                let _ = file.read(&mut buf);
            }
            Err(e) => {
                eprintln!("{e}");
            }
        }
    }
}

does not panic

[Its-Just-Nans]

I'm closing it

Reopen if needed