From 18d52557f2a901a82c977aa5c89c9bdec418bde9 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Wed, 8 May 2024 19:49:38 -0400 Subject: [PATCH] respond to review comments --- src/read.rs | 4 ++-- src/spec.rs | 51 ++++++++++++++++++++++++++++++++++++--------------- src/types.rs | 4 ++-- 3 files changed, 40 insertions(+), 19 deletions(-) diff --git a/src/read.rs b/src/read.rs index 4a2a2e33e..05ab98edd 100644 --- a/src/read.rs +++ b/src/read.rs @@ -1285,9 +1285,9 @@ impl<'a> Drop for ZipFile<'a> { /// * `data_start`: set to 0 /// * `external_attributes`: `unix_mode()`: will return None pub fn read_zipfile_from_stream<'a, R: Read>(reader: &'a mut R) -> ZipResult>> { - let mut block = vec![0u8; mem::size_of::()]; + let mut block = [0u8; mem::size_of::()]; reader.read_exact(&mut block)?; - let block = block.into_boxed_slice(); + let block: Box<[u8]> = block.into(); let signature = spec::Magic::from_le_bytes( block[..mem::size_of_val(&spec::CENTRAL_DIRECTORY_HEADER_SIGNATURE)] diff --git a/src/spec.rs b/src/spec.rs index d0dae62e1..c498c2618 100755 --- a/src/spec.rs +++ b/src/spec.rs @@ -1,4 +1,3 @@ -#![allow(clippy::wrong_self_convention)] #![macro_use] use crate::result::{ZipError, ZipResult}; @@ -86,7 +85,7 @@ macro_rules! to_le { #[derive(Copy, Clone, Debug)] #[repr(packed)] pub struct CDEBlock { - pub magic: Magic, + magic: Magic, pub disk_number: u16, pub disk_with_central_directory: u16, pub number_of_files_on_this_disk: u16, @@ -97,6 +96,7 @@ pub struct CDEBlock { } impl CDEBlock { + #[allow(clippy::wrong_self_convention)] #[inline(always)] fn from_le(mut self) -> Self { from_le![ @@ -252,17 +252,26 @@ impl CentralDirectoryEnd { } } + /* We always want to make sure we go allllll the way back to the start of the file if + * we can't find it elsewhere. However, our `while` condition doesn't check that. So we + * avoid infinite looping by checking at the end of the loop. */ if window_start == search_upper_bound { break; } debug_assert!(END_WINDOW_SIZE > mem::size_of_val(&CENTRAL_DIRECTORY_END_SIGNATURE)); + /* Shift the window by END_WINDOW_SIZE bytes, but make sure to cover matches that + * overlap our nice neat window boundaries! */ + window_start = (window_start + /* NB: To catch matches across window boundaries, we need to make our blocks overlap + * by the width of the pattern to match. */ + + mem::size_of_val(&CENTRAL_DIRECTORY_END_SIGNATURE) as u64) + /* This should never happen, but make sure we don't go past the end of the file. */ + .min(file_length); window_start = window_start .saturating_sub( - /* Shift the window upon each iteration. */ - END_WINDOW_SIZE as u64 - /* But don't shift all the way -- make sure to catch matches across window - * boundaries! */ - - mem::size_of_val(&CENTRAL_DIRECTORY_END_SIGNATURE) as u64, + /* Shift the window upon each iteration so we search END_WINDOW_SIZE bytes at + * once (unless limited by file_length). */ + END_WINDOW_SIZE as u64, ) /* This will never go below the value of `search_upper_bound`, so we have a special * `if window_start == search_upper_bound` check above. */ @@ -285,13 +294,14 @@ impl CentralDirectoryEnd { #[derive(Copy, Clone)] #[repr(packed)] pub struct Zip64CDELocatorBlock { - pub magic: Magic, + magic: Magic, pub disk_with_central_directory: u32, pub end_of_central_directory_offset: u64, pub number_of_disks: u32, } impl Zip64CDELocatorBlock { + #[allow(clippy::wrong_self_convention)] #[inline(always)] fn from_le(mut self) -> Self { from_le![ @@ -384,7 +394,7 @@ impl Zip64CentralDirectoryEndLocator { #[derive(Copy, Clone)] #[repr(packed)] pub struct Zip64CDEBlock { - pub magic: Magic, + magic: Magic, pub record_size: u64, pub version_made_by: u16, pub version_needed_to_extract: u16, @@ -397,6 +407,7 @@ pub struct Zip64CDEBlock { } impl Zip64CDEBlock { + #[allow(clippy::wrong_self_convention)] #[inline(always)] fn from_le(mut self) -> Self { from_le![ @@ -537,19 +548,29 @@ impl Zip64CentralDirectoryEnd { results.push((cde, archive_offset)); } + /* We always want to make sure we go allllll the way back to the start of the file if + * we can't find it elsewhere. However, our `while` condition doesn't check that. So we + * avoid infinite looping by checking at the end of the loop. */ if window_start == nominal_offset { break; } debug_assert!( END_WINDOW_SIZE > mem::size_of_val(&ZIP64_CENTRAL_DIRECTORY_END_SIGNATURE) ); + /* Shift the window by END_WINDOW_SIZE bytes, but make sure to cover matches that + * overlap our nice neat window boundaries! */ + window_start = (window_start + /* NB: To catch matches across window boundaries, we need to make our blocks overlap + * by the width of the pattern to match. */ + + mem::size_of_val(&ZIP64_CENTRAL_DIRECTORY_END_SIGNATURE) as u64) + /* This may never happen, but make sure we don't go past the end of the specified + * range. */ + .min(search_upper_bound); window_start = window_start .saturating_sub( - /* Shift the window upon each iteration. */ - END_WINDOW_SIZE as u64 - /* But don't shift all the way -- make sure to catch matches across window - * boundaries! */ - - mem::size_of_val(&ZIP64_CENTRAL_DIRECTORY_END_SIGNATURE) as u64, + /* Shift the window upon each iteration so we search END_WINDOW_SIZE bytes at + * once (unless limited by search_upper_bound). */ + END_WINDOW_SIZE as u64, ) /* This will never go below the value of `nominal_offset`, so we have a special * `if window_start == nominal_offset` check above. */ @@ -651,7 +672,7 @@ mod test { #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] #[repr(packed)] pub struct TestBlock { - pub magic: Magic, + magic: Magic, pub file_name_length: u16, } diff --git a/src/types.rs b/src/types.rs index 7653c63bc..7bc3d387f 100644 --- a/src/types.rs +++ b/src/types.rs @@ -1,5 +1,3 @@ -#![allow(clippy::wrong_self_convention)] - //! Types that specify what is contained in a ZIP. use crate::cp437::FromCp437; use crate::write::{FileOptionExtension, FileOptions}; @@ -707,6 +705,7 @@ pub(crate) struct ZipEntryBlock { } impl ZipEntryBlock { + #[allow(clippy::wrong_self_convention)] #[inline(always)] fn from_le(mut self) -> Self { from_le![ @@ -795,6 +794,7 @@ pub(crate) struct ZipLocalEntryBlock { } impl ZipLocalEntryBlock { + #[allow(clippy::wrong_self_convention)] #[inline(always)] fn from_le(mut self) -> Self { from_le![