Bug fix for permissions on deep-copied files

src/types.rs

@@ -1,4 +1,5 @@
 //! Types that specify what is contained in a ZIP.
+use path::{Component, Path, PathBuf};
 use std::path;
 
 #[cfg(not(any(
@@ -12,7 +13,7 @@ use std::time::SystemTime;
 #[cfg(doc)]
 use {crate::read::ZipFile, crate::write::FileOptions};
 
-mod ffi {
+pub(crate) mod ffi {
     pub const S_IFDIR: u32 = 0o0040000;
     pub const S_IFREG: u32 = 0o0100000;
 }
@@ -100,7 +101,7 @@ pub struct DateTime {
     second: u8,
 }
 
-impl ::std::default::Default for DateTime {
+impl Default for DateTime {
     /// Constructs an 'default' datetime of 1980-01-01 00:00:00
     fn default() -> DateTime {
         DateTime {
@@ -353,7 +354,7 @@ pub struct ZipFileData {
 }
 
 impl ZipFileData {
-    pub fn file_name_sanitized(&self) -> ::std::path::PathBuf {
+    pub fn file_name_sanitized(&self) -> PathBuf {
         let no_null_filename = match self.file_name.find('\0') {
             Some(index) => &self.file_name[0..index],
             None => &self.file_name,
@@ -363,7 +364,7 @@ impl ZipFileData {
         // zip files can contain both / and \ as separators regardless of the OS
         // and as we want to return a sanitized PathBuf that only supports the
         // OS separator let's convert incompatible separators to compatible ones
-        let separator = ::std::path::MAIN_SEPARATOR;
+        let separator = path::MAIN_SEPARATOR;
         let opposite_separator = match separator {
             '/' => '\\',
             _ => '/',
@@ -371,27 +372,27 @@ impl ZipFileData {
         let filename =
             no_null_filename.replace(&opposite_separator.to_string(), &separator.to_string());
 
-        ::std::path::Path::new(&filename)
+        Path::new(&filename)
             .components()
-            .filter(|component| matches!(*component, ::std::path::Component::Normal(..)))
+            .filter(|component| matches!(*component, path::Component::Normal(..)))
-            .fold(::std::path::PathBuf::new(), |mut path, ref cur| {
+            .fold(PathBuf::new(), |mut path, ref cur| {
                 path.push(cur.as_os_str());
                 path
             })
     }
 
-    pub(crate) fn enclosed_name(&self) -> Option<&path::Path> {
+    pub(crate) fn enclosed_name(&self) -> Option<&Path> {
         if self.file_name.contains('\0') {
             return None;
         }
-        let path = path::Path::new(&self.file_name);
+        let path = Path::new(&self.file_name);
         let mut depth = 0usize;
         for component in path.components() {
             match component {
-                path::Component::Prefix(_) | path::Component::RootDir => return None,
+                Component::Prefix(_) | Component::RootDir => return None,
-                path::Component::ParentDir => depth = depth.checked_sub(1)?,
+                Component::ParentDir => depth = depth.checked_sub(1)?,
-                path::Component::Normal(_) => depth += 1,
+                Component::Normal(_) => depth += 1,
-                path::Component::CurDir => (),
+                Component::CurDir => (),
             }
         }
         Some(path)
@@ -509,10 +510,7 @@ mod test {
             large_file: false,
             aes_mode: None,
         };
-        assert_eq!(
+        assert_eq!(data.file_name_sanitized(), PathBuf::from("path/etc/passwd"));
-            data.file_name_sanitized(),
-            ::std::path::PathBuf::from("path/etc/passwd")
-        );
     }
 
     #[test]

src/write.rs

@@ -4,7 +4,7 @@ use crate::compression::CompressionMethod;
 use crate::read::{central_header_to_zip_file, find_content, ZipArchive, ZipFile, ZipFileReader};
 use crate::result::{ZipError, ZipResult};
 use crate::spec;
-use crate::types::{AtomicU64, DateTime, System, ZipFileData, DEFAULT_VERSION};
+use crate::types::{ffi, AtomicU64, DateTime, System, ZipFileData, DEFAULT_VERSION};
 use byteorder::{LittleEndian, ReadBytesExt, WriteBytesExt};
 use crc32fast::Hasher;
 use std::convert::TryInto;
@@ -307,32 +307,33 @@ impl<A: Read + Write + Seek> ZipWriter<A> {
         let write_position = self.inner.get_plain().stream_position()?;
         let src_data = self.data_by_name(src_name)?;
         let data_start = src_data.data_start.load();
-        let real_size = src_data.compressed_size.max(write_position - data_start);
+        let compressed_size = src_data.compressed_size;
+        if compressed_size > write_position - data_start {
+            return Err(ZipError::InvalidArchive("Source file size too large"));
+        }
+        let uncompressed_size = src_data.uncompressed_size;
         let mut options = FileOptions::default()
-            .large_file(real_size > spec::ZIP64_BYTES_THR)
+            .large_file(compressed_size.max(uncompressed_size) > spec::ZIP64_BYTES_THR)
             .last_modified_time(src_data.last_modified_time)
             .compression_method(src_data.compression_method);
         if let Some(perms) = src_data.unix_mode() {
             options = options.unix_permissions(perms);
         }
+        Self::normalize_options(&mut options);
         let raw_values = ZipRawValues {
             crc32: src_data.crc32,
-            compressed_size: real_size,
+            compressed_size,
-            uncompressed_size: src_data.uncompressed_size,
+            uncompressed_size,
         };
         let mut reader = BufReader::new(ZipFileReader::Raw(self.reader_by_name(src_name)?));
-        let mut copy = Vec::with_capacity(real_size as usize);
+        let mut copy = Vec::with_capacity(compressed_size as usize);
         reader.read_to_end(&mut copy)?;
         drop(reader);
-        self.inner
-            .get_plain()
-            .seek(SeekFrom::Start(write_position))?;
         self.start_entry(dest_name, options, Some(raw_values))?;
-        self.writing_raw = true;
+        self.inner.switch_to(CompressionMethod::Stored, None)?;
         self.writing_to_file = true;
-        self.write_all(&copy)?;
+        self.writing_raw = true;
-
+        Ok(self.write_all(&copy)?)
-        Ok(())
     }
 }
 
@@ -464,10 +465,7 @@ impl<W: Write + Seek> ZipWriter<W> {
     where
         S: Into<String>,
     {
-        if options.permissions.is_none() {
+        Self::normalize_options(&mut options);
-            options.permissions = Some(0o644);
-        }
-        *options.permissions.as_mut().unwrap() |= 0o100000;
         self.start_entry(name, options, None)?;
         self.inner
             .switch_to(options.compression_method, options.compression_level)?;
@@ -475,6 +473,13 @@ impl<W: Write + Seek> ZipWriter<W> {
         Ok(())
     }
 
+    fn normalize_options(options: &mut FileOptions) {
+        if options.permissions.is_none() {
+            options.permissions = Some(0o644);
+        }
+        *options.permissions.as_mut().unwrap() |= ffi::S_IFREG;
+    }
+
     /// Starts a file, taking a Path as argument.
     ///
     /// This function ensures that the '/' path separator is used. It also ignores all non 'Normal'
@@ -591,10 +596,7 @@ impl<W: Write + Seek> ZipWriter<W> {
     where
         S: Into<String>,
     {
-        if options.permissions.is_none() {
+        Self::normalize_options(&mut options);
-            options.permissions = Some(0o644);
-        }
-        *options.permissions.as_mut().unwrap() |= 0o100000;
         self.start_entry(name, options, None)?;
         self.writing_to_file = true;
         self.writing_to_extra_field = true;
@@ -693,6 +695,7 @@ impl<W: Write + Seek> ZipWriter<W> {
         if let Some(perms) = file.unix_mode() {
             options = options.unix_permissions(perms);
         }
+        Self::normalize_options(&mut options);
 
         let raw_values = ZipRawValues {
             crc32: file.crc32(),

tests/end_to_end.rs

@@ -15,7 +15,7 @@ fn end_to_end() {
         let file = &mut Cursor::new(Vec::new());
 
         println!("Writing file with {method} compression");
-        write_test_archive(file, method, true).expect("Couldn't write test zip archive");
+        write_test_archive(file, method, true);
 
         println!("Checking file contents");
         check_archive_file(file, ENTRY_NAME, Some(method), LOREM_IPSUM);
@@ -29,7 +29,7 @@ fn end_to_end() {
 fn copy() {
     for &method in SUPPORTED_COMPRESSION_METHODS {
         let src_file = &mut Cursor::new(Vec::new());
-        write_test_archive(src_file, method, false).expect("Couldn't write to test file");
+        write_test_archive(src_file, method, false);
 
         let mut tgt_file = &mut Cursor::new(Vec::new());
 
@@ -68,14 +68,17 @@ fn copy() {
 fn append() {
     for &method in SUPPORTED_COMPRESSION_METHODS {
         for shallow_copy in &[false, true] {
+            println!("Writing file with {method} compression, shallow_copy {shallow_copy}");
             let mut file = &mut Cursor::new(Vec::new());
-            write_test_archive(file, method, *shallow_copy).expect("Couldn't write to test file");
+            write_test_archive(file, method, *shallow_copy);
 
             {
                 let mut zip = ZipWriter::new_append(&mut file).unwrap();
                 zip.start_file(
                     COPY_ENTRY_NAME,
-                    FileOptions::default().compression_method(method),
+                    FileOptions::default()
+                        .compression_method(method)
+                        .unix_permissions(0o755),
                 )
                 .unwrap();
                 zip.write_all(LOREM_IPSUM).unwrap();
@@ -91,40 +94,39 @@ fn append() {
 }
 
 // Write a test zip archive to buffer.
-fn write_test_archive(
+fn write_test_archive(file: &mut Cursor<Vec<u8>>, method: CompressionMethod, shallow_copy: bool) {
-    file: &mut Cursor<Vec<u8>>,
-    method: CompressionMethod,
-    shallow_copy: bool,
-) -> ZipResult<()> {
     let mut zip = ZipWriter::new(file);
 
-    zip.add_directory("test/", Default::default())?;
+    zip.add_directory("test/", Default::default()).unwrap();
 
     let options = FileOptions::default()
         .compression_method(method)
         .unix_permissions(0o755);
 
-    zip.start_file("test/☃.txt", options)?;
+    zip.start_file("test/☃.txt", options).unwrap();
-    zip.write_all(b"Hello, World!\n")?;
+    zip.write_all(b"Hello, World!\n").unwrap();
 
-    zip.start_file_with_extra_data("test_with_extra_data/🐢.txt", options)?;
+    zip.start_file_with_extra_data("test_with_extra_data/🐢.txt", options)
-    zip.write_u16::<LittleEndian>(0xbeef)?;
+        .unwrap();
-    zip.write_u16::<LittleEndian>(EXTRA_DATA.len() as u16)?;
+    zip.write_u16::<LittleEndian>(0xbeef).unwrap();
-    zip.write_all(EXTRA_DATA)?;
+    zip.write_u16::<LittleEndian>(EXTRA_DATA.len() as u16)
-    zip.end_extra_data()?;
+        .unwrap();
-    zip.write_all(b"Hello, World! Again.\n")?;
+    zip.write_all(EXTRA_DATA).unwrap();
+    zip.end_extra_data().unwrap();
+    zip.write_all(b"Hello, World! Again.\n").unwrap();
 
-    zip.start_file(ENTRY_NAME, options)?;
+    zip.start_file(ENTRY_NAME, options).unwrap();
-    zip.write_all(LOREM_IPSUM)?;
+    zip.write_all(LOREM_IPSUM).unwrap();
 
     if shallow_copy {
-        zip.shallow_copy_file(ENTRY_NAME, INTERNAL_COPY_ENTRY_NAME)?;
+        zip.shallow_copy_file(ENTRY_NAME, INTERNAL_COPY_ENTRY_NAME)
+            .unwrap();
     } else {
-        zip.deep_copy_file(ENTRY_NAME, INTERNAL_COPY_ENTRY_NAME)?;
+        zip.deep_copy_file(ENTRY_NAME, INTERNAL_COPY_ENTRY_NAME)
+            .unwrap();
     }
 
-    zip.finish()?;
+    zip.finish().unwrap();
-    Ok(())
 }
 
 // Load an archive from buffer and check for test data.
@@ -200,6 +202,9 @@ fn check_archive_file_contents<R: Read + Seek>(
     name: &str,
     expected: &[u8],
 ) {
+    let file_permissions: u32 = archive.by_name(name).unwrap().unix_mode().unwrap();
+    assert_eq!(file_permissions, 0o100755);
+
     let file_contents: String = read_archive_file(archive, name).unwrap();
     assert_eq!(file_contents.as_bytes(), expected);
 }