Merge pull request #212 from a1phyr/improve_unsafe_code

refactor: Improve `FixedSizeBlock`

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, FixedSizeBlock, Zip32CentralDirectoryEnd, ZIP64_ENTRY_THR};
+use crate::spec::{self, FixedSizeBlock, Pod, Zip32CentralDirectoryEnd, ZIP64_ENTRY_THR};
 use crate::types::{
     AesMode, AesVendorVersion, DateTime, System, ZipCentralEntryBlock, ZipFileData,
     ZipLocalEntryBlock,
@@ -1760,19 +1760,17 @@ pub fn read_zipfile_from_stream<'a, R: Read>(reader: &'a mut R) -> ZipResult<Opt
     // "magic" value (since the magic value will be from the central directory header if we've
     // finished iterating over all the actual files).
     /* TODO: smallvec? */
-    let mut block = [0u8; mem::size_of::<ZipLocalEntryBlock>()];
-    reader.read_exact(&mut block)?;
-    let block: Box<[u8]> = block.into();
 
-    let signature = spec::Magic::from_first_le_bytes(&block);
+    let mut block = ZipLocalEntryBlock::zeroed();
+    reader.read_exact(block.as_bytes_mut())?;
 
-    match signature {
+    match block.magic().from_le() {
         spec::Magic::LOCAL_FILE_HEADER_SIGNATURE => (),
         spec::Magic::CENTRAL_DIRECTORY_HEADER_SIGNATURE => return Ok(None),
         _ => return Err(ZipLocalEntryBlock::WRONG_MAGIC_ERROR),
     }
 
-    let block = ZipLocalEntryBlock::interpret(&block)?;
+    let block = block.from_le();
 
     let mut result = ZipFileData::from_local_block(block, reader)?;
 

src/spec.rs

@@ -2,11 +2,11 @@
 
 use crate::result::{ZipError, ZipResult};
 use core::mem;
-use core::mem::align_of;
 use memchr::memmem::FinderRev;
 use std::io;
 use std::io::prelude::*;
 use std::rc::Rc;
+use std::slice;
 
 /// "Magic" header values used in the zip spec to locate metadata records.
 ///
@@ -26,12 +26,6 @@ impl Magic {
         Self(u32::from_le_bytes(bytes))
     }
 
-    #[inline(always)]
-    pub fn from_first_le_bytes(data: &[u8]) -> Self {
-        let first_bytes: [u8; 4] = data[..mem::size_of::<Self>()].try_into().unwrap();
-        Self::from_le_bytes(first_bytes)
-    }
-
     #[inline(always)]
     pub const fn to_le_bytes(self) -> [u8; 4] {
         self.0.to_le_bytes()
@@ -97,64 +91,56 @@ impl ExtraFieldMagic {
 pub const ZIP64_BYTES_THR: u64 = u32::MAX as u64;
 pub const ZIP64_ENTRY_THR: usize = u16::MAX as usize;
 
-pub(crate) trait FixedSizeBlock: Sized + Copy {
+/// # Safety
+///
+/// - No padding/uninit bytes
+/// - All bytes patterns must be valid
+/// - No cell, pointers
+///
+/// See `bytemuck::Pod` for more details.
+pub(crate) unsafe trait Pod: Copy + 'static {
+    #[inline]
+    fn zeroed() -> Self {
+        unsafe { mem::zeroed() }
+    }
+
+    #[inline]
+    fn as_bytes(&self) -> &[u8] {
+        unsafe { slice::from_raw_parts(self as *const Self as *const u8, mem::size_of::<Self>()) }
+    }
+
+    #[inline]
+    fn as_bytes_mut(&mut self) -> &mut [u8] {
+        unsafe { slice::from_raw_parts_mut(self as *mut Self as *mut u8, mem::size_of::<Self>()) }
+    }
+}
+
+pub(crate) trait FixedSizeBlock: Pod {
     const MAGIC: Magic;
 
     fn magic(self) -> Magic;
 
     const WRONG_MAGIC_ERROR: ZipError;
 
-    /* TODO: use smallvec? */
+    #[allow(clippy::wrong_self_convention)]
-    fn interpret(bytes: &[u8]) -> ZipResult<Self> {
+    fn from_le(self) -> Self;
-        if bytes.len() != mem::size_of::<Self>() {
-            return Err(ZipError::InvalidArchive("Block is wrong size"));
-        }
-        let block_ptr: *const Self = bytes.as_ptr().cast();
 
-        // If alignment could be more than 1, we'd have to use read_unaligned() below
+    fn parse<R: Read>(reader: &mut R) -> ZipResult<Self> {
-        debug_assert_eq!(align_of::<Self>(), 1);
+        let mut block = Self::zeroed();
+        reader.read_exact(block.as_bytes_mut())?;
+        let block = Self::from_le(block);
 
-        let block = unsafe { block_ptr.read() }.from_le();
         if block.magic() != Self::MAGIC {
             return Err(Self::WRONG_MAGIC_ERROR);
         }
         Ok(block)
     }
 
-    #[allow(clippy::wrong_self_convention)]
-    fn from_le(self) -> Self;
-
-    fn parse<T: Read>(reader: &mut T) -> ZipResult<Self> {
-        let mut block = vec![0u8; mem::size_of::<Self>()].into_boxed_slice();
-        reader.read_exact(&mut block)?;
-        Self::interpret(&block)
-    }
-
-    fn encode(self) -> Box<[u8]> {
-        self.to_le().serialize()
-    }
-
     fn to_le(self) -> Self;
 
-    /* TODO: use Box<[u8; mem::size_of::<Self>()]> when generic_const_exprs are stabilized! */
-    fn serialize(self) -> Box<[u8]> {
-        /* TODO: use Box::new_zeroed() when stabilized! */
-        /* TODO: also consider using smallvec! */
-
-        // If alignment could be more than 1, we'd have to use write_unaligned() below
-        debug_assert_eq!(align_of::<Self>(), 1);
-
-        let mut out_block = vec![0u8; mem::size_of::<Self>()].into_boxed_slice();
-        let out_ptr: *mut Self = out_block.as_mut_ptr().cast();
-        unsafe {
-            out_ptr.write(self);
-        }
-        out_block
-    }
-
     fn write<T: Write>(self, writer: &mut T) -> ZipResult<()> {
-        let block = self.encode();
+        let block = self.to_le();
-        writer.write_all(&block)?;
+        writer.write_all(block.as_bytes())?;
         Ok(())
     }
 }
@@ -206,7 +192,7 @@ macro_rules! to_and_from_le {
 }
 
 #[derive(Copy, Clone, Debug)]
-#[repr(packed)]
+#[repr(packed, C)]
 pub(crate) struct Zip32CDEBlock {
     magic: Magic,
     pub disk_number: u16,
@@ -218,6 +204,8 @@ pub(crate) struct Zip32CDEBlock {
     pub zip_file_comment_length: u16,
 }
 
+unsafe impl Pod for Zip32CDEBlock {}
+
 impl FixedSizeBlock for Zip32CDEBlock {
     const MAGIC: Magic = Magic::CENTRAL_DIRECTORY_END_SIGNATURE;
 
@@ -395,7 +383,7 @@ impl Zip32CentralDirectoryEnd {
 }
 
 #[derive(Copy, Clone)]
-#[repr(packed)]
+#[repr(packed, C)]
 pub(crate) struct Zip64CDELocatorBlock {
     magic: Magic,
     pub disk_with_central_directory: u32,
@@ -403,6 +391,8 @@ pub(crate) struct Zip64CDELocatorBlock {
     pub number_of_disks: u32,
 }
 
+unsafe impl Pod for Zip64CDELocatorBlock {}
+
 impl FixedSizeBlock for Zip64CDELocatorBlock {
     const MAGIC: Magic = Magic::ZIP64_CENTRAL_DIRECTORY_END_LOCATOR_SIGNATURE;
 
@@ -465,7 +455,7 @@ impl Zip64CentralDirectoryEndLocator {
 }
 
 #[derive(Copy, Clone)]
-#[repr(packed)]
+#[repr(packed, C)]
 pub(crate) struct Zip64CDEBlock {
     magic: Magic,
     pub record_size: u64,
@@ -479,6 +469,8 @@ pub(crate) struct Zip64CDEBlock {
     pub central_directory_offset: u64,
 }
 
+unsafe impl Pod for Zip64CDEBlock {}
+
 impl FixedSizeBlock for Zip64CDEBlock {
     const MAGIC: Magic = Magic::ZIP64_CENTRAL_DIRECTORY_END_SIGNATURE;
 
@@ -668,12 +660,14 @@ mod test {
     use std::io::Cursor;
 
     #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
-    #[repr(packed)]
+    #[repr(packed, C)]
     pub struct TestBlock {
         magic: Magic,
         pub file_name_length: u16,
     }
 
+    unsafe impl Pod for TestBlock {}
+
     impl FixedSizeBlock for TestBlock {
         const MAGIC: Magic = Magic::literal(0x01111);
 

src/types.rs

@@ -12,7 +12,7 @@ use std::sync::{Arc, OnceLock};
 use chrono::{Datelike, NaiveDate, NaiveDateTime, NaiveTime, Timelike};
 
 use crate::result::{ZipError, ZipResult};
-use crate::spec::{self, FixedSizeBlock};
+use crate::spec::{self, FixedSizeBlock, Pod};
 
 pub(crate) mod ffi {
     pub const S_IFDIR: u32 = 0o0040000;
@@ -893,7 +893,7 @@ impl ZipFileData {
 }
 
 #[derive(Copy, Clone, Debug)]
-#[repr(packed)]
+#[repr(packed, C)]
 pub(crate) struct ZipCentralEntryBlock {
     magic: spec::Magic,
     pub version_made_by: u16,
@@ -914,6 +914,8 @@ pub(crate) struct ZipCentralEntryBlock {
     pub offset: u32,
 }
 
+unsafe impl Pod for ZipCentralEntryBlock {}
+
 impl FixedSizeBlock for ZipCentralEntryBlock {
     const MAGIC: spec::Magic = spec::Magic::CENTRAL_DIRECTORY_HEADER_SIGNATURE;
 
@@ -947,7 +949,7 @@ impl FixedSizeBlock for ZipCentralEntryBlock {
 }
 
 #[derive(Copy, Clone, Debug)]
-#[repr(packed)]
+#[repr(packed, C)]
 pub(crate) struct ZipLocalEntryBlock {
     magic: spec::Magic,
     pub version_made_by: u16,
@@ -962,6 +964,8 @@ pub(crate) struct ZipLocalEntryBlock {
     pub extra_field_length: u16,
 }
 
+unsafe impl Pod for ZipLocalEntryBlock {}
+
 impl FixedSizeBlock for ZipLocalEntryBlock {
     const MAGIC: spec::Magic = spec::Magic::LOCAL_FILE_HEADER_SIGNATURE;