From 5d16fa250c524c0a373a4aac75cc2782f47e42c1 Mon Sep 17 00:00:00 2001 From: Chris Hennick Date: Mon, 15 May 2023 21:25:34 -0700 Subject: [PATCH] Revert "Wrap extra-data field in an Option, to save Arc overhead when empty" This reverts commit 1df9186527ec6f6964f0b671e791472a477a7c9f. --- CHANGELOG.md | 8 +--- src/read.rs | 130 ++++++++++++++++++++++++--------------------------- src/types.rs | 8 ++-- src/write.rs | 86 ++++++++++++---------------------- 4 files changed, 96 insertions(+), 136 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6979583a..3cb979d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -158,10 +158,4 @@ ### Added - - New method `with_alignment` on `FileOptions`. - -## [0.8.3] - -### Changed - - - Improved performance for files with no extra data (should mainly affect multithreaded Mutex-guarded access). \ No newline at end of file + - New method `with_alignment` on `FileOptions`. \ No newline at end of file diff --git a/src/read.rs b/src/read.rs index 074b0af5..cc95558c 100644 --- a/src/read.rs +++ b/src/read.rs @@ -737,8 +737,8 @@ fn central_header_to_zip_file_inner( uncompressed_size: uncompressed_size as u64, file_name, file_name_raw, - extra_field: Some(Arc::new(extra_field)), - central_extra_field: None, + extra_field: Arc::new(extra_field), + central_extra_field: Arc::new(vec![]), file_comment, header_start: offset, central_header_start, @@ -770,73 +770,69 @@ fn central_header_to_zip_file_inner( } fn parse_extra_field(file: &mut ZipFileData) -> ZipResult<()> { - if let Some(extra_data) = &file.extra_field { - let mut reader = io::Cursor::new(extra_data.as_ref()); + let mut reader = io::Cursor::new(file.extra_field.as_ref()); - while (reader.position() as usize) < extra_data.len() { - let kind = reader.read_u16::()?; - let len = reader.read_u16::()?; - let mut len_left = len as i64; - match kind { - // Zip64 extended information extra field - 0x0001 => { - if file.uncompressed_size == spec::ZIP64_BYTES_THR { - file.large_file = true; - file.uncompressed_size = reader.read_u64::()?; - len_left -= 8; - } - if file.compressed_size == spec::ZIP64_BYTES_THR { - file.large_file = true; - file.compressed_size = reader.read_u64::()?; - len_left -= 8; - } - if file.header_start == spec::ZIP64_BYTES_THR { - file.header_start = reader.read_u64::()?; - len_left -= 8; - } + while (reader.position() as usize) < file.extra_field.len() { + let kind = reader.read_u16::()?; + let len = reader.read_u16::()?; + let mut len_left = len as i64; + match kind { + // Zip64 extended information extra field + 0x0001 => { + if file.uncompressed_size == spec::ZIP64_BYTES_THR { + file.large_file = true; + file.uncompressed_size = reader.read_u64::()?; + len_left -= 8; } - 0x9901 => { - // AES - if len != 7 { - return Err(ZipError::UnsupportedArchive( - "AES extra data field has an unsupported length", - )); - } - let vendor_version = reader.read_u16::()?; - let vendor_id = reader.read_u16::()?; - let aes_mode = reader.read_u8()?; - let compression_method = reader.read_u16::()?; - - if vendor_id != 0x4541 { - return Err(ZipError::InvalidArchive("Invalid AES vendor")); - } - let vendor_version = match vendor_version { - 0x0001 => AesVendorVersion::Ae1, - 0x0002 => AesVendorVersion::Ae2, - _ => return Err(ZipError::InvalidArchive("Invalid AES vendor version")), - }; - match aes_mode { - 0x01 => file.aes_mode = Some((AesMode::Aes128, vendor_version)), - 0x02 => file.aes_mode = Some((AesMode::Aes192, vendor_version)), - 0x03 => file.aes_mode = Some((AesMode::Aes256, vendor_version)), - _ => { - return Err(ZipError::InvalidArchive("Invalid AES encryption strength")) - } - }; - file.compression_method = { - #[allow(deprecated)] - CompressionMethod::from_u16(compression_method) - }; + if file.compressed_size == spec::ZIP64_BYTES_THR { + file.large_file = true; + file.compressed_size = reader.read_u64::()?; + len_left -= 8; } - _ => { - // Other fields are ignored + if file.header_start == spec::ZIP64_BYTES_THR { + file.header_start = reader.read_u64::()?; + len_left -= 8; } } + 0x9901 => { + // AES + if len != 7 { + return Err(ZipError::UnsupportedArchive( + "AES extra data field has an unsupported length", + )); + } + let vendor_version = reader.read_u16::()?; + let vendor_id = reader.read_u16::()?; + let aes_mode = reader.read_u8()?; + let compression_method = reader.read_u16::()?; - // We could also check for < 0 to check for errors - if len_left > 0 { - reader.seek(io::SeekFrom::Current(len_left))?; + if vendor_id != 0x4541 { + return Err(ZipError::InvalidArchive("Invalid AES vendor")); + } + let vendor_version = match vendor_version { + 0x0001 => AesVendorVersion::Ae1, + 0x0002 => AesVendorVersion::Ae2, + _ => return Err(ZipError::InvalidArchive("Invalid AES vendor version")), + }; + match aes_mode { + 0x01 => file.aes_mode = Some((AesMode::Aes128, vendor_version)), + 0x02 => file.aes_mode = Some((AesMode::Aes192, vendor_version)), + 0x03 => file.aes_mode = Some((AesMode::Aes256, vendor_version)), + _ => return Err(ZipError::InvalidArchive("Invalid AES encryption strength")), + }; + file.compression_method = { + #[allow(deprecated)] + CompressionMethod::from_u16(compression_method) + }; } + _ => { + // Other fields are ignored + } + } + + // We could also check for < 0 to check for errors + if len_left > 0 { + reader.seek(io::SeekFrom::Current(len_left))?; } } Ok(()) @@ -983,11 +979,7 @@ impl<'a> ZipFile<'a> { /// Get the extra data of the zip header for this file pub fn extra_data(&self) -> &[u8] { - if let Some(extra_data) = &self.data.extra_field { - extra_data - } else { - &[] - } + &self.data.extra_field } /// Get the starting offset of the data of the compressed file @@ -1106,8 +1098,8 @@ pub fn read_zipfile_from_stream<'a, R: Read>(reader: &'a mut R) -> ZipResult, /// Extra field usually used for storage expansion - pub extra_field: Option>>, + pub extra_field: Arc>, /// Extra field only written to central directory - pub central_extra_field: Option>>, + pub central_extra_field: Arc>, /// File comment pub file_comment: String, /// Specifies where the local header of the file starts @@ -531,8 +531,8 @@ mod test { uncompressed_size: 0, file_name: file_name.clone(), file_name_raw: file_name.into_bytes(), - extra_field: None, - central_extra_field: None, + extra_field: Arc::new(vec![]), + central_extra_field: Arc::new(vec![]), file_comment: String::new(), header_start: 0, data_start: AtomicU64::new(0), diff --git a/src/write.rs b/src/write.rs index ca6b89ec..f7db7441 100644 --- a/src/write.rs +++ b/src/write.rs @@ -134,8 +134,8 @@ pub struct FileOptions { pub(crate) permissions: Option, pub(crate) large_file: bool, encrypt_with: Option, - extra_data: Option>>, - central_extra_data: Option>>, + extra_data: Arc>, + central_extra_data: Arc>, alignment: u16, } @@ -149,8 +149,8 @@ impl arbitrary::Arbitrary<'_> for FileOptions { permissions: Option::::arbitrary(u)?, large_file: bool::arbitrary(u)?, encrypt_with: Option::::arbitrary(u)?, - extra_data: None, - central_extra_data: None, + extra_data: Arc::new(vec![]), + central_extra_data: Arc::new(vec![]), alignment: u16::arbitrary(u)?, }; u.arbitrary_loop(Some(0), Some((u16::MAX / 4) as u32), |u| { @@ -242,14 +242,7 @@ impl FileOptions { ) -> ZipResult<()> { validate_extra_data(header_id, data)?; let len = data.len() + 4; - if self.extra_data.as_ref().map_or(0, |data| data.len()) - + self - .central_extra_data - .as_ref() - .map_or(0, |data| data.len()) - + len - > u16::MAX as usize - { + if self.extra_data.len() + self.central_extra_data.len() + len > u16::MAX as usize { Err(InvalidArchive( "Extra data field would be longer than allowed", )) @@ -259,17 +252,12 @@ impl FileOptions { } else { &mut self.extra_data }; - let vec = match field { - Some(arc) => match Arc::get_mut(arc) { - Some(exclusive) => exclusive, - None => { - *field = Some(Arc::new(arc.to_vec())); - Arc::get_mut(field.as_mut().unwrap()).unwrap() - } - }, + let vec = Arc::get_mut(field); + let vec = match vec { + Some(exclusive) => exclusive, None => { - *field = Some(Arc::new(Vec::new())); - Arc::get_mut(field.as_mut().unwrap()).unwrap() + *field = Arc::new(field.to_vec()); + Arc::get_mut(field).unwrap() } }; vec.reserve_exact(data.len() + 4); @@ -283,11 +271,11 @@ impl FileOptions { /// Removes the extra data fields. #[must_use] pub fn clear_extra_data(mut self) -> FileOptions { - if self.extra_data.is_some() { - self.extra_data = None; + if self.extra_data.len() > 0 { + self.extra_data = Arc::new(vec![]); } - if self.central_extra_data.is_some() { - self.central_extra_data = None; + if self.central_extra_data.len() > 0 { + self.central_extra_data = Arc::new(vec![]); } self } @@ -324,8 +312,8 @@ impl Default for FileOptions { permissions: None, large_file: false, encrypt_with: None, - extra_data: None, - central_extra_data: None, + extra_data: Arc::new(vec![]), + central_extra_data: Arc::new(vec![]), alignment: 1, } } @@ -594,17 +582,11 @@ impl ZipWriter { // file name length writer.write_u16::(file.file_name.as_bytes().len() as u16)?; // extra field length - let mut extra_field_length = file.extra_field.as_ref().map_or(0, |data| data.len()); + let mut extra_field_length = file.extra_field.len(); if file.large_file { extra_field_length += 20; } - if extra_field_length - + file - .central_extra_field - .as_ref() - .map_or(0, |data| data.len()) - > u16::MAX as usize - { + if extra_field_length + file.central_extra_field.len() > u16::MAX as usize { let _ = self.abort_file(); return Err(InvalidArchive("Extra data field is too large")); } @@ -616,9 +598,7 @@ impl ZipWriter { if file.large_file { write_local_zip64_extra_field(writer, file)?; } - if let Some(extra_data) = &file.extra_field { - writer.write_all(extra_data)?; - } + writer.write_all(&file.extra_field)?; let mut header_end = writer.stream_position()?; if options.alignment > 1 { let align = options.alignment as u64; @@ -1282,11 +1262,8 @@ fn write_central_directory_header(writer: &mut T, file: &ZipFileData) // extra field length writer.write_u16::( zip64_extra_field_length - + file.extra_field.as_ref().map_or(0, |data| data.len()) as u16 - + file - .central_extra_field - .as_ref() - .map_or(0, |data| data.len()) as u16, + + file.extra_field.len() as u16 + + file.central_extra_field.len() as u16, )?; // file comment length writer.write_u16::(0)?; @@ -1303,12 +1280,8 @@ fn write_central_directory_header(writer: &mut T, file: &ZipFileData) // zip64 extra field writer.write_all(&zip64_extra_field[..zip64_extra_field_length as usize])?; // extra field - if let Some(extra_data) = &file.extra_field { - writer.write_all(extra_data)?; - } - if let Some(extra_data) = &file.central_extra_field { - writer.write_all(extra_data)?; - } + writer.write_all(&file.extra_field)?; + writer.write_all(&file.central_extra_field)?; // file comment // @@ -1433,6 +1406,7 @@ mod test { use crate::ZipArchive; use std::io; use std::io::{Read, Write}; + use std::sync::Arc; #[test] fn write_empty_zip() { @@ -1552,8 +1526,8 @@ mod test { permissions: Some(33188), large_file: false, encrypt_with: None, - extra_data: None, - central_extra_data: None, + extra_data: Arc::new(vec![]), + central_extra_data: Arc::new(vec![]), alignment: 1, }; writer.start_file("mimetype", options).unwrap(); @@ -1592,8 +1566,8 @@ mod test { permissions: Some(33188), large_file: false, encrypt_with: None, - extra_data: None, - central_extra_data: None, + extra_data: Arc::new(vec![]), + central_extra_data: Arc::new(vec![]), alignment: 0, }; writer.start_file(RT_TEST_FILENAME, options).unwrap(); @@ -1642,8 +1616,8 @@ mod test { permissions: Some(33188), large_file: false, encrypt_with: None, - extra_data: None, - central_extra_data: None, + extra_data: Arc::new(vec![]), + central_extra_data: Arc::new(vec![]), alignment: 0, }; writer.start_file(RT_TEST_FILENAME, options).unwrap();