From e6141ace581c2be83a30b01097edc273ebdda8b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Iv=C3=A1nek?= Date: Sun, 20 Oct 2024 11:33:17 +0200 Subject: [PATCH] fix: first pass on maintainer comments --- src/read.rs | 16 ++++++++-------- src/read/magic_finder.rs | 18 ++++++++++-------- src/spec.rs | 37 ++++++++++++++++++++++++++----------- 3 files changed, 44 insertions(+), 27 deletions(-) diff --git a/src/read.rs b/src/read.rs index e5493ca1..573b1fdb 100644 --- a/src/read.rs +++ b/src/read.rs @@ -8,7 +8,7 @@ use crate::crc32::Crc32Reader; use crate::extra_fields::{ExtendedTimestamp, ExtraField}; use crate::read::zip_archive::{Shared, SharedBuilder}; use crate::result::{ZipError, ZipResult}; -use crate::spec::{self, CentralDirectoryEndInfo, FixedSizeBlock, Pod}; +use crate::spec::{self, CentralDirectoryEndInfo, DataAndPosition, FixedSizeBlock, Pod}; use crate::types::{ AesMode, AesVendorVersion, DateTime, System, ZipCentralEntryBlock, ZipFileData, ZipLocalEntryBlock, @@ -450,7 +450,7 @@ impl<'a> TryFrom<&'a CentralDirectoryEndInfo> for CentralDirectoryInfo { let (relative_cd_offset, number_of_files, disk_number, disk_with_central_directory) = match &value.eocd64 { - Some((eocd64, _)) => { + Some(DataAndPosition { data: eocd64, .. }) => { if eocd64.number_of_files_on_this_disk > eocd64.number_of_files { return Err(InvalidArchive( "ZIP64 footer indicates more files on this disk than in the whole archive", @@ -469,10 +469,10 @@ impl<'a> TryFrom<&'a CentralDirectoryEndInfo> for CentralDirectoryInfo { ) } _ => ( - value.eocd.0.central_directory_offset as u64, - value.eocd.0.number_of_files_on_this_disk as usize, - value.eocd.0.disk_number as u32, - value.eocd.0.disk_with_central_directory as u32, + value.eocd.data.central_directory_offset as u64, + value.eocd.data.number_of_files_on_this_disk as usize, + value.eocd.data.disk_number as u32, + value.eocd.data.disk_with_central_directory as u32, ), }; @@ -608,8 +608,8 @@ impl ZipArchive { let shared = Self::read_central_header(info, config, reader)?; Ok(shared.build( - cde.eocd.0.zip_file_comment, - cde.eocd64.map(|(v, _)| v.extensible_data_sector), + cde.eocd.data.zip_file_comment, + cde.eocd64.map(|v| v.data.extensible_data_sector), )) } diff --git a/src/read/magic_finder.rs b/src/read/magic_finder.rs index c719defc..d405b5b2 100644 --- a/src/read/magic_finder.rs +++ b/src/read/magic_finder.rs @@ -17,22 +17,21 @@ pub struct MagicFinder<'a> { impl<'a> MagicFinder<'a> { /// Create a new magic bytes finder to look within specific bounds. - pub fn new(magic_bytes: &'a [u8], bounds: (u64, u64)) -> Self { + pub fn new(magic_bytes: &'a [u8], start_inclusive: u64, end_exclusive: u64) -> Self { const BUFFER_SIZE: usize = 2048; // Smaller buffer size would be unable to locate bytes. // Equal buffer size would stall (the window could not be moved). - debug_assert!(BUFFER_SIZE > magic_bytes.len()); + debug_assert!(BUFFER_SIZE >= magic_bytes.len()); Self { buffer: vec![0; BUFFER_SIZE].into_boxed_slice(), finder: FinderRev::new(magic_bytes), - cursor: bounds - .1 + cursor: end_exclusive .saturating_sub(BUFFER_SIZE as u64) - .clamp(bounds.0, bounds.1), + .clamp(start_inclusive, end_exclusive), mid_buffer_offset: None, - bounds, + bounds: (start_inclusive, end_exclusive), } } @@ -109,7 +108,7 @@ impl<'a> MagicFinder<'a> { /* Move cursor to the next chunk, cover magic at boundary by shifting by needle length. */ self.cursor = self .cursor - .saturating_add(self.finder.needle().len() as u64) + .saturating_add(self.finder.needle().len() as u64 - 1) .saturating_sub(self.buffer.len() as u64) .clamp(self.bounds.0, self.bounds.1); } @@ -139,7 +138,7 @@ impl<'a> OptimisticMagicFinder<'a> { /// Create a new empty optimistic magic bytes finder. pub fn new_empty() -> Self { Self { - inner: MagicFinder::new(&[], (0, 0)), + inner: MagicFinder::new(&[], 0, 0), initial_guess: None, } } @@ -183,6 +182,9 @@ impl<'a> OptimisticMagicFinder<'a> { // If a match is not found, but the initial guess was mandatory, return an error. if mandatory { return Ok(None); + } else { + // If the initial guess was not mandatory, remove it, as it was not found. + self.initial_guess.take(); } } diff --git a/src/spec.rs b/src/spec.rs index 64266907..5f2a69be 100755 --- a/src/spec.rs +++ b/src/spec.rs @@ -290,7 +290,7 @@ pub(crate) struct Zip32CentralDirectoryEnd { } impl Zip32CentralDirectoryEnd { - fn block_and_comment(self) -> ZipResult<(Zip32CDEBlock, Box<[u8]>)> { + fn try_into_block_and_comment(self) -> ZipResult<(Zip32CDEBlock, Box<[u8]>)> { let Self { disk_number, disk_with_central_directory, @@ -344,7 +344,7 @@ impl Zip32CentralDirectoryEnd { } pub fn write(self, writer: &mut T) -> ZipResult<()> { - let (block, comment) = self.block_and_comment()?; + let (block, comment) = self.try_into_block_and_comment()?; block.write(writer)?; writer.write_all(&comment)?; Ok(()) @@ -513,7 +513,7 @@ impl Zip64CentralDirectoryEnd { }) } - pub fn block_and_comment(self) -> (Zip64CDEBlock, Box<[u8]>) { + pub fn into_block_and_comment(self) -> (Zip64CDEBlock, Box<[u8]>) { let Self { record_size, version_made_by, @@ -545,16 +545,31 @@ impl Zip64CentralDirectoryEnd { } pub fn write(self, writer: &mut T) -> ZipResult<()> { - let (block, comment) = self.block_and_comment(); + let (block, comment) = self.into_block_and_comment(); block.write(writer)?; writer.write_all(&comment)?; Ok(()) } } +pub(crate) struct DataAndPosition { + pub data: T, + #[allow(dead_code)] + pub position: u64, +} + +impl From<(T, u64)> for DataAndPosition { + fn from(value: (T, u64)) -> Self { + Self { + data: value.0, + position: value.1, + } + } +} + pub(crate) struct CentralDirectoryEndInfo { - pub eocd: (Zip32CentralDirectoryEnd, u64), - pub eocd64: Option<(Zip64CentralDirectoryEnd, u64)>, + pub eocd: DataAndPosition, + pub eocd64: Option>, pub archive_offset: u64, } @@ -580,7 +595,7 @@ pub fn find_central_directory( let file_length = reader.seek(io::SeekFrom::End(0))?; // Instantiate the mandatory finder - let mut eocd_finder = MagicFinder::new(&EOCD_SIG_BYTES, (0, file_length)); + let mut eocd_finder = MagicFinder::new(&EOCD_SIG_BYTES, 0, file_length); let mut subfinder: Option> = None; // Keep the last errors for cases of improper EOCD instances. @@ -612,7 +627,7 @@ pub fn find_central_directory( // If the archive is empty, there is nothing more to be checked, the archive is correct. if eocd.number_of_files == 0 { return Ok(CentralDirectoryEndInfo { - eocd: (eocd, eocd_offset), + eocd: (eocd, eocd_offset).into(), eocd64: None, archive_offset: eocd_offset - relative_cd_offset, }); @@ -646,7 +661,7 @@ pub fn find_central_directory( let archive_offset = cd_offset - relative_cd_offset; return Ok(CentralDirectoryEndInfo { - eocd: (eocd, eocd_offset), + eocd: (eocd, eocd_offset).into(), eocd64: None, archive_offset, }); @@ -737,8 +752,8 @@ pub fn find_central_directory( ) { Ok(eocd64) => { return Ok(CentralDirectoryEndInfo { - eocd: (eocd, eocd_offset), - eocd64: Some((eocd64, eocd64_offset)), + eocd: (eocd, eocd_offset).into(), + eocd64: Some((eocd64, eocd64_offset).into()), archive_offset, }) }