wip: Add variables to InvalidArchive messages to fix #127

src/lib.rs

@@ -32,6 +32,8 @@ pub use crate::read::ZipArchive;
 pub use crate::types::{AesMode, DateTime};
 pub use crate::write::ZipWriter;
 
+extern crate alloc;
+
 #[cfg(feature = "aes-crypto")]
 mod aes;
 #[cfg(feature = "aes-crypto")]

src/read.rs

@@ -7,7 +7,7 @@ use crate::cp437::FromCp437;
 use crate::crc32::Crc32Reader;
 use crate::extra_fields::{ExtendedTimestamp, ExtraField};
 use crate::read::zip_archive::Shared;
-use crate::result::{ZipError, ZipResult};
+use crate::result::{invalid, invalid_archive, ZipError, ZipResult};
 use crate::spec;
 use crate::types::{AesMode, AesVendorVersion, DateTime, System, ZipFileData};
 use crate::zipcrypto::{ZipCryptoReader, ZipCryptoReaderValid, ZipCryptoValidator};
@@ -218,7 +218,7 @@ pub(crate) fn find_content<'a>(
     reader.seek(io::SeekFrom::Start(data.header_start))?;
     let signature = reader.read_u32_le()?;
     if signature != spec::LOCAL_FILE_HEADER_SIGNATURE {
-        return Err(ZipError::InvalidArchive("Invalid local file header"));
+        invalid!("Invalid local file header")
     }
     let data_start = match data.data_start.get() {
         None => {
@@ -410,16 +410,16 @@ impl<R: Read + Seek> ZipArchive<R> {
         new_files.values_mut().try_for_each(|f| {
             /* This is probably the only really important thing to change. */
             f.header_start = f.header_start.checked_add(new_initial_header_start).ok_or(
-                ZipError::InvalidArchive("new header start from merge would have been too large"),
-            )?;
+                invalid!("new header start from merge would have been {} bytes, which is too large",
+                f.header_start as u128 + new_initial_header_start as u128))?;
             /* This is only ever used internally to cache metadata lookups (it's not part of the
              * zip spec), and 0 is the sentinel value. */
             f.central_header_start = 0;
             /* This is an atomic variable so it can be updated from another thread in the
              * implementation (which is good!). */
             if let Some(old_data_start) = f.data_start.take() {
                 let new_data_start = old_data_start.checked_add(new_initial_header_start).ok_or(
-                    ZipError::InvalidArchive("new data start from merge would have been too large"),
+                    invalid!("new data start from merge would have been too large"),
                 )?;
                 f.data_start.get_or_init(|| new_data_start);
             }
@@ -462,9 +462,7 @@ impl<R: Read + Seek> ZipArchive<R> {
         let archive_offset = cde_start_pos
             .checked_sub(footer.central_directory_size as u64)
             .and_then(|x| x.checked_sub(footer.central_directory_offset as u64))
-            .ok_or(ZipError::InvalidArchive(
-                "Invalid central directory size or offset",
-            ))?;
+            .ok_or(invalid!("Invalid central directory size or offset"))?;
 
         let directory_start = footer.central_directory_offset as u64 + archive_offset;
         let number_of_files = footer.number_of_files_on_this_disk as usize;
@@ -477,6 +475,10 @@ impl<R: Read + Seek> ZipArchive<R> {
         })
     }
 
+    fn spec_version_from(raw_version: u16) -> String {
+        (raw_version / 10).to_string() + "." + &*(raw_version % 10).to_string()
+    }
+
     fn get_directory_info_zip64(
         reader: &mut R,
         footer: &spec::CentralDirectoryEnd,
@@ -504,8 +506,8 @@ impl<R: Read + Seek> ZipArchive<R> {
 
         let search_upper_bound = cde_start_pos
             .checked_sub(60) // minimum size of Zip64CentralDirectoryEnd + Zip64CentralDirectoryEndLocator
-            .ok_or(ZipError::InvalidArchive(
-                "File cannot contain ZIP64 central directory end",
+            .ok_or(invalid!(
+                "File cannot contain ZIP64 central directory end"
             ))?;
         let search_results = spec::Zip64CentralDirectoryEnd::find_and_parse(
             reader,
@@ -517,23 +519,21 @@ impl<R: Read + Seek> ZipArchive<R> {
                 let directory_start_result = footer64
                     .central_directory_offset
                     .checked_add(archive_offset)
-                    .ok_or(ZipError::InvalidArchive(
-                        "Invalid central directory size or offset",
-                    ));
+                    .ok_or(invalid!("Central directory would start at address {}",
+                                footer64.central_directory_offset as u128 + archive_offset as u128),
+                    )?;
                 directory_start_result.and_then(|directory_start| {
                     if directory_start > search_upper_bound {
-                        Err(ZipError::InvalidArchive(
-                            "Invalid central directory size or offset",
-                        ))
+                        invalid!("Invalid central directory size or offset {}", directory_start)
                     } else if footer64.number_of_files_on_this_disk > footer64.number_of_files {
-                        Err(ZipError::InvalidArchive(
-                            "ZIP64 footer indicates more files on this disk than in the whole archive",
-                        ))
+                        invalid!(
+                            "ZIP64 footer indicates {} files on this disk but only {} in the whole archive",
+                            footer64.number_of_files_on_this_disk, footer64.number_of_files
+                        )
                     } else if footer64.version_needed_to_extract > footer64.version_made_by {
-                        Err(ZipError::InvalidArchive(
-                            "ZIP64 footer indicates a new version is needed to extract this archive than the \
-    version that wrote it",
-                        ))
+                        invalid!("ZIP64 footer indicates archive was written by version {} but can only be opened by version {} or newer",
+                            footer64.version_made_by, footer64.version_needed_to_extract
+                        )
                     } else {
                         Ok(CentralDirectoryInfo {
                             archive_offset,
@@ -570,25 +570,29 @@ impl<R: Read + Seek> ZipArchive<R> {
                     if central_dir.number_of_files != zip32_central_dir.number_of_files
                         && zip32_central_dir.number_of_files != u16::MAX as usize
                     {
-                        *result = Err(ZipError::InvalidArchive(
-                            "ZIP32 and ZIP64 file counts don't match",
-                        ));
+                        *result = invalid_archive(
+                            format!("ZIP32 file count {} and ZIP64 file count {} don't match",
+                                    zip32_central_dir.number_of_files, central_dir.number_of_files)
+                        );
                         return;
                     }
                     if central_dir.disk_number != zip32_central_dir.disk_number
                         && zip32_central_dir.disk_number != u16::MAX as u32
                     {
-                        *result = Err(ZipError::InvalidArchive(
-                            "ZIP32 and ZIP64 disk numbers don't match",
-                        ));
+                        *result = invalid_archive(
+                            format!("ZIP32 disk number {} and ZIP64 disk number {} don't match",
+                                    zip32_central_dir.disk_number, central_dir.disk_number)
+                            );
                         return;
                     }
                     if central_dir.disk_with_central_directory
                         != zip32_central_dir.disk_with_central_directory
                         && zip32_central_dir.disk_with_central_directory != u16::MAX as u32
                     {
-                        *result = Err(ZipError::InvalidArchive(
-                            "ZIP32 and ZIP64 last-disk numbers don't match",
+                        *result = invalid_archive(
+                            format!("ZIP32 last-disk number {} doesn't match ZIP64 last-disk number {}",
+                                    zip32_central_dir.disk_with_central_directory,
+                                    central_dir.disk_with_central_directory
                         ));
                     }
                 }
@@ -675,8 +679,7 @@ impl<R: Read + Seek> ZipArchive<R> {
         for i in 0..self.len() {
             let mut file = self.by_index(i)?;
             let filepath = file
-                .enclosed_name()
-                .ok_or(ZipError::InvalidArchive("Invalid file path"))?;
+                .try_enclosed_name()?;
 
             let outpath = directory.as_ref().join(filepath);
 
@@ -951,7 +954,7 @@ pub(crate) fn central_header_to_zip_file<R: Read + Seek>(
     // Parse central header
     let signature = reader.read_u32_le()?;
     if signature != spec::CENTRAL_DIRECTORY_HEADER_SIGNATURE {
-        Err(ZipError::InvalidArchive("Invalid Central Directory header"))
+        invalid!("Invalid Central Directory header")
     } else {
         central_header_to_zip_file_inner(reader, archive_offset, central_header_start)
     }
@@ -1036,16 +1039,16 @@ fn central_header_to_zip_file_inner<R: Read>(
 
     let aes_enabled = result.compression_method == CompressionMethod::AES;
     if aes_enabled && result.aes_mode.is_none() {
-        return Err(ZipError::InvalidArchive(
-            "AES encryption without AES extra data field",
-        ));
+        invalid!(
+            "AES encryption without AES extra data field"
+        )
     }
 
     // Account for shifted zip offsets.
     result.header_start = result
         .header_start
         .checked_add(archive_offset)
-        .ok_or(ZipError::InvalidArchive("Archive header is too large"))?;
+        .ok_or(invalid!("Archive header is too large"))?;
 
     Ok(result)
 }
@@ -1094,12 +1097,12 @@ fn parse_extra_field(file: &mut ZipFileData) -> ZipResult<()> {
                 let compression_method = CompressionMethod::from_u16(reader.read_u16_le()?);
 
                 if vendor_id != 0x4541 {
-                    return Err(ZipError::InvalidArchive("Invalid AES vendor"));
+                    invalid!("Invalid AES vendor");
                 }
                 let vendor_version = match vendor_version {
                     0x0001 => AesVendorVersion::Ae1,
                     0x0002 => AesVendorVersion::Ae2,
-                    _ => return Err(ZipError::InvalidArchive("Invalid AES vendor version")),
+                    _ => invalid!("Invalid AES vendor version"),
                 };
                 match aes_mode {
                     0x01 => {
@@ -1111,7 +1114,7 @@ fn parse_extra_field(file: &mut ZipFileData) -> ZipResult<()> {
                     0x03 => {
                         file.aes_mode = Some((AesMode::Aes256, vendor_version, compression_method))
                     }
-                    _ => return Err(ZipError::InvalidArchive("Invalid AES encryption strength")),
+                    _ => invalid!("Invalid AES encryption strength"),
                 };
                 file.compression_method = compression_method;
             }
@@ -1230,6 +1233,11 @@ impl<'a> ZipFile<'a> {
         self.data.enclosed_name()
     }
 
+    pub fn try_enclosed_name(&self) -> ZipResult<PathBuf> {
+        self.data.try_enclosed_name()
+    }
+
+
     /// Get the comment of the file
     pub fn comment(&self) -> &str {
         &self.data.file_comment
@@ -1356,7 +1364,7 @@ pub fn read_zipfile_from_stream<'a, R: Read>(reader: &'a mut R) -> ZipResult<Opt
     match signature {
         spec::LOCAL_FILE_HEADER_SIGNATURE => (),
         spec::CENTRAL_DIRECTORY_HEADER_SIGNATURE => return Ok(None),
-        _ => return Err(ZipError::InvalidArchive("Invalid local file header")),
+        _ => invalid!("Invalid local file header"),
     }
 
     let version_made_by = reader.read_u16_le()?;

src/read/stream.rs

@@ -1,7 +1,8 @@
-use crate::unstable::LittleEndianReadExt;
+use crate::unstable::{bytes_to_rust_literal, LittleEndianReadExt};
 use std::fs;
 use std::io::{self, Read};
 use std::path::{Path, PathBuf};
+use crate::result::invalid;
 
 use super::{
     central_header_to_zip_file_inner, read_zipfile_from_stream, spec, ZipError, ZipFile,
@@ -62,7 +63,8 @@ impl<R: Read> ZipStreamReader<R> {
             fn visit_file(&mut self, file: &mut ZipFile<'_>) -> ZipResult<()> {
                 let filepath = file
                     .enclosed_name()
-                    .ok_or(ZipError::InvalidArchive("Invalid file path"))?;
+                    .ok_or(invalid!("Invalid file path {}",
+                                                   bytes_to_rust_literal(file.name_raw())))?;
 
                 let outpath = self.0.join(filepath);
 
@@ -87,8 +89,7 @@ impl<R: Read> ZipStreamReader<R> {
                 #[cfg(unix)]
                 {
                     let filepath = metadata
-                        .enclosed_name()
-                        .ok_or(ZipError::InvalidArchive("Invalid file path"))?;
+                        .try_enclosed_name()?;
 
                     let outpath = self.0.join(filepath);
 

src/result.rs

@@ -2,6 +2,7 @@
 #![allow(non_local_definitions)]
 //! Error types that can be emitted from this library
 
+use std::borrow::Cow;
 use displaydoc::Display;
 use thiserror::Error;
 
@@ -21,41 +22,47 @@ pub enum ZipError {
     Io(#[from] io::Error),
 
     /// invalid Zip archive: {0}
-    InvalidArchive(&'static str),
+    InvalidArchive(Cow<'static, str>),
 
     /// unsupported Zip archive: {0}
-    UnsupportedArchive(&'static str),
+    UnsupportedArchive(Cow<'static, str>),
 
     /// specified file not found in archive
-    FileNotFound,
+    FileNotFound(Box<str>),
 
     /// The password provided is incorrect
-    InvalidPassword,
+    InvalidPassword {
+        filename: Box<str>,
+        password: Option<Box<str>>
+    },
 }
 
-impl ZipError {
-    /// The text used as an error when a password is required and not supplied
-    ///
-    /// ```rust,no_run
-    /// # use zip::result::ZipError;
-    /// # let mut archive = zip::ZipArchive::new(std::io::Cursor::new(&[])).unwrap();
-    /// match archive.by_index(1) {
-    ///     Err(ZipError::UnsupportedArchive(ZipError::PASSWORD_REQUIRED)) => eprintln!("a password is needed to unzip this file"),
-    ///     _ => (),
-    /// }
-    /// # ()
-    /// ```
-    pub const PASSWORD_REQUIRED: &'static str = "Password required to decrypt file";
+pub(crate) fn invalid_archive<T, M: Into<Cow<'static, str>>>(message: M) -> ZipResult<T> {
+    Err(ZipError::InvalidArchive(message.into()))
 }
 
+macro_rules! invalid {
+    ($fmt_string:literal) => {
+        {
+            return crate::result::invalid_archive($fmt_string).into();
+        }
+    };
+    ($fmt_string:literal, $($param:expr),+) => {
+        {
+            return crate::result::invalid_archive(alloc::format!($fmt_string, ($($param),+))).into();
+        }
+    };
+}
+pub(crate) use invalid;
+
 impl From<ZipError> for io::Error {
     fn from(err: ZipError) -> io::Error {
         let kind = match &err {
             ZipError::Io(err) => err.kind(),
             ZipError::InvalidArchive(_) => io::ErrorKind::InvalidData,
             ZipError::UnsupportedArchive(_) => io::ErrorKind::Unsupported,
-            ZipError::FileNotFound => io::ErrorKind::NotFound,
-            ZipError::InvalidPassword => io::ErrorKind::InvalidInput,
+            ZipError::FileNotFound(_) => io::ErrorKind::NotFound,
+            ZipError::InvalidPassword {..} => io::ErrorKind::InvalidInput,
         };
 
         io::Error::new(kind, err)