fix: ignore a ZIP64 extra field that no sentinel asked for

src/extra_fields/extra_field.rs

@@ -117,20 +117,13 @@ impl ExtraField {
         let parsed_extra_field = match decoded_extra_field {
             // Zip64 extended information extra field
             Ok(UsedExtraField::Zip64ExtendedInfo) => {
-                let (new_uncomp, new_comp, new_head) = Zip64ExtendedInformation::parse(
+                ExtraField::Zip64ExtendedInformation(Zip64ExtendedInformation::parse(
                     reader,
                     len,
                     file.get_uncompressed_size(),
                     file.get_compressed_size(),
                     file.get_header_start(),
-                )?;
-                ExtraField::Zip64ExtendedInformation(Zip64ExtendedInformation {
-                    sizes: Some(Zip64Sizes {
-                        uncompressed_size: new_uncomp,
-                        compressed_size: new_comp,
-                    }),
-                    header_start: Some(new_head),
-                })
+                )?)
             }
             Ok(UsedExtraField::Ntfs) => {
                 // NTFS extra field

src/extra_fields/zip64_extended_information.rs

@@ -140,72 +140,87 @@ impl Zip64ExtendedInformation {
         Ok(())
     }
 
+    /// Reads the value for one field, and keeps it only if the entry asked for it.
+    ///
+    /// Whether the value is *read* is decided by the block's length, so that the reader stays in
+    /// step with writers that emit more fields than they need to. Whether it is *kept* is decided
+    /// by `is_zip64`, which says whether the matching field in the entry held the sentinel.
     #[inline]
-    pub(crate) fn parse<R: Read>(
+    fn read_field<R: Read>(
         reader: &mut R,
         len: u16,
-        uncompressed_size: u32,
-        compressed_size: u32,
-        header_start: Option<u32>,
-    ) -> ZipResult<(u64, u64, u64)> {
-        let mut consumed_len = 0;
-        let new_uncompressed_size = if len >= 24 || u64::from(uncompressed_size) == ZIP64_BYTES_THR
-        {
-            let new_uncompressed_size = match reader.read_u64_le() {
-                Ok(v) => v,
-                Err(e) if e.kind() == ErrorKind::UnexpectedEof => {
-                    return Err(invalid!("ZIP64 extra field truncated"));
-                }
-                Err(e) => return Err(e.into()),
-            };
-            consumed_len += mem::size_of::<u64>();
-            new_uncompressed_size
-        } else {
-            uncompressed_size.into()
+        consumed_len: &mut usize,
+        is_zip64: bool,
+    ) -> ZipResult<Option<u64>> {
+        if len < 24 && !is_zip64 {
+            return Ok(None);
+        }
+        let value = match reader.read_u64_le() {
+            Ok(value) => value,
+            Err(e) if e.kind() == ErrorKind::UnexpectedEof => {
+                return Err(invalid!("ZIP64 extra field truncated"));
+            }
+            Err(e) => return Err(e.into()),
         };
+        *consumed_len += mem::size_of::<u64>();
+        Ok(is_zip64.then_some(value))
+    }
 
-        let new_compressed_size = if len >= 24 || u64::from(compressed_size) == ZIP64_BYTES_THR {
-            let new_compressed_size = match reader.read_u64_le() {
-                Ok(v) => v,
-                Err(e) if e.kind() == ErrorKind::UnexpectedEof => {
-                    return Err(invalid!("ZIP64 extra field truncated"));
-                }
-                Err(e) => return Err(e.into()),
-            };
-            consumed_len += mem::size_of::<u64>();
-            new_compressed_size
-        } else {
-            compressed_size.into()
-        };
+    /// Reads the block, keeping only the values the entry actually asked for.
+    ///
+    /// Per APPNOTE 4.5.3 a value belongs in this block only when the matching field in the entry
+    /// itself holds the 0xFFFFFFFF sentinel, which means "too large to store here, the real value
+    /// is in the ZIP64 block". Writers exist that emit a full length block anyway, and at least
+    /// one emits a malformed one, so a value that no sentinel asked for is dropped rather than
+    /// read over the entry's own perfectly good field. Such a value can only repeat what the entry
+    /// already said, so dropping it costs nothing when the block is well formed, and it is the
+    /// only way a malformed block can be told apart from a meaningful one.
+    ///
+    /// The `None` fields this leaves behind are the same `None` the writer uses for "this entry
+    /// has nothing to record here", so a block that was ignored on the way in is not written back
+    /// out on the way through.
+    ///
+    /// `entry_header_start` is `None` for a local header, which has no relative offset field for
+    /// the block to override in the first place.
+    #[inline]
+    pub(crate) fn parse<R: Read>(
+        reader: &mut R,
+        len: u16,
+        entry_uncompressed_size: u32,
+        entry_compressed_size: u32,
+        entry_header_start: Option<u32>,
+    ) -> ZipResult<Self> {
+        let mut consumed_len = 0;
 
-        let new_header_start = if len >= 24 {
-            let new_header_start = match reader.read_u64_le() {
-                Ok(v) => v,
-                Err(e) if e.kind() == ErrorKind::UnexpectedEof => {
-                    return Err(invalid!("ZIP64 extra field truncated"));
-                }
-                Err(e) => return Err(e.into()),
-            };
-            consumed_len += mem::size_of::<u64>();
-            new_header_start
+        let uncompressed_size = Self::read_field(
+            reader,
+            len,
+            &mut consumed_len,
+            u64::from(entry_uncompressed_size) == ZIP64_BYTES_THR,
+        )?;
+        let compressed_size = Self::read_field(
+            reader,
+            len,
+            &mut consumed_len,
+            u64::from(entry_compressed_size) == ZIP64_BYTES_THR,
+        )?;
+        let header_start = Self::read_field(
+            reader,
+            len,
+            &mut consumed_len,
+            entry_header_start.is_some_and(|start| u64::from(start) == ZIP64_BYTES_THR),
+        )?;
+
+        // The two sizes travel together, so one sentinel brings both along. The field that had no
+        // sentinel keeps the entry's own value, which is what it already held.
+        let sizes = if uncompressed_size.is_some() || compressed_size.is_some() {
+            Some(Zip64Sizes {
+                uncompressed_size: uncompressed_size
+                    .unwrap_or_else(|| entry_uncompressed_size.into()),
+                compressed_size: compressed_size.unwrap_or_else(|| entry_compressed_size.into()),
+            })
         } else {
-            if let Some(header_start) = header_start {
-                if u64::from(header_start) == ZIP64_BYTES_THR {
-                    let new_header_start = match reader.read_u64_le() {
-                        Ok(v) => v,
-                        Err(e) if e.kind() == ErrorKind::UnexpectedEof => {
-                            return Err(invalid!("ZIP64 extra field truncated"));
-                        }
-                        Err(e) => return Err(e.into()),
-                    };
-                    consumed_len += mem::size_of::<u64>();
-                    new_header_start
-                } else {
-                    header_start.into()
-                }
-            } else {
-                0
-            }
+            None
         };
 
         let Some(leftover_len) = (len as usize).checked_sub(consumed_len) else {
@@ -219,6 +234,9 @@ impl Zip64ExtendedInformation {
             return Err(e.into());
         }
 
-        Ok((new_uncompressed_size, new_compressed_size, new_header_start))
+        Ok(Self {
+            sizes,
+            header_start,
+        })
     }
 }