Avoid warnings on corrupt zip files

When a corrupt zip file is read, an exception should be thrown without
any previous notices and warnings leaking.

This also introduces constants for a few magic numbers in the zip
format.

Author: splitbrain

src/Zip.php

@@ -17,6 +17,10 @@ class Zip extends Archive
 {
     const LOCAL_FILE_HEADER_CRC_OFFSET = 14;
 
+    const SIG_LOCAL_FILE_HEADER    = "\x50\x4b\x03\x04";
+    const SIG_CENTRAL_FILE_HEADER  = "\x50\x4b\x01\x02";
+    const SIG_END_OF_CENTRAL_DIR   = "\x50\x4b\x05\x06";
+
     protected $file = '';
     protected $fh;
     protected $memory = '';
@@ -89,6 +93,7 @@ public function contents()
      *
      * @see contents()
      * @throws ArchiveIOException
+     * @throws ArchiveCorruptedException
      * @return FileInfo[]
      */
     public function yieldContents()
@@ -129,6 +134,7 @@ public function yieldContents()
      * @param string     $exclude a regular expression of files to exclude
      * @param string     $include a regular expression of files to include
      * @throws ArchiveIOException
+     * @throws ArchiveCorruptedException
      * @return FileInfo[]
      */
     public function extract($outdir, $strip = '', $exclude = '', $include = '')
@@ -478,7 +484,7 @@ public function close()
             $this->writebytes($ctrldir);
 
             // write end of central directory record
-            $this->writebytes("\x50\x4b\x05\x06"); // end of central dir signature
+            $this->writebytes(self::SIG_END_OF_CENTRAL_DIR);
             $this->writebytes(pack('v', 0)); // number of this disk
             $this->writebytes(pack('v', 0)); // number of the disk with the start of the central directory
             $this->writebytes(pack('v',
@@ -538,6 +544,7 @@ public function save($file)
      * This key-value list contains general information about the ZIP file
      *
      * @return array
+     * @throws ArchiveCorruptedException when the file is not a valid ZIP archive
      */
     protected function readCentralDir()
     {
@@ -550,21 +557,31 @@ protected function readCentralDir()
 
         @fseek($this->fh, $size - $maximum_size);
         $pos   = ftell($this->fh);
-        $bytes = 0x00000000;
+        $bytes = '';
 
         while ($pos < $size) {
-            $byte  = @fread($this->fh, 1);
-            $bytes = (($bytes << 8) & 0xFFFFFFFF) | ord($byte);
-            if ($bytes == 0x504b0506) {
+            $bytes = substr($bytes . (string)@fread($this->fh, 1), -4);
+            if ($bytes === self::SIG_END_OF_CENTRAL_DIR) {
                 break;
             }
             $pos++;
         }
 
+        if ($bytes !== self::SIG_END_OF_CENTRAL_DIR) {
+            throw new ArchiveCorruptedException(
+                'End of central directory signature not found - not a valid ZIP file'
+            );
+        }
+
         $data = unpack(
             'vdisk/vdisk_start/vdisk_entries/ventries/Vsize/Voffset/vcomment_size',
             fread($this->fh, 18)
         );
+        if ($data === false) {
+            throw new ArchiveCorruptedException(
+                'Could not read end of central directory record'
+            );
+        }
 
         if ($data['comment_size'] != 0) {
             $centd['comment'] = fread($this->fh, $data['comment_size']);
@@ -912,7 +929,7 @@ protected function makeCentralFileRecord($offset, $ts, $crc, $len, $clen, $name,
 
         list($name, $extra) = $this->encodeFilename($name);
 
-        $header = "\x50\x4b\x01\x02"; // central file header signature
+        $header = self::SIG_CENTRAL_FILE_HEADER;
         $header .= pack('v', 14); // version made by - VFAT
         $header .= pack('v', 20); // version needed to extract - 2.0
         $header .= pack('v', 0); // general purpose flag - no flags set
@@ -959,7 +976,7 @@ protected function makeLocalFileHeader($ts, $crc, $len, $clen, $name, $comp = nu
 
         list($name, $extra) = $this->encodeFilename($name);
 
-        $header = "\x50\x4b\x03\x04"; //  local file header signature
+        $header = self::SIG_LOCAL_FILE_HEADER;
         $header .= pack('v', 20); // version needed to extract - 2.0
         $header .= pack('v', 0); // general purpose flag - no flags set
         $header .= pack('v', $comp); // compression method - deflate|none

tests/ZipTestCase.php

@@ -52,6 +52,51 @@ public function testMissing()
         $tar->open('nope.zip');
     }
 
+    /**
+     * Feeding a file without an End-of-Central-Directory signature must raise
+     * ArchiveCorruptedException, not bubble up PHP warnings from unpack().
+     */
+    public function testNotAZip()
+    {
+        $tmp = sys_get_temp_dir() . '/phpa-bad-' . md5(uniqid('', true)) . '.bin';
+        file_put_contents($tmp, str_repeat('this is not a zip file ', 50));
+        $zip = new Zip();
+        $zip->open($tmp);
+        try {
+            $this->expectException(ArchiveCorruptedException::class);
+            iterator_to_array($zip->yieldContents());
+        } finally {
+            try {
+                $zip->close();
+            } catch(\Exception $e) {
+                /* already errored */
+            }
+            @unlink($tmp);
+        }
+    }
+
+    /**
+     * A truncated file (smaller than the EOCD record) must also fail cleanly.
+     */
+    public function testTruncatedFile()
+    {
+        $tmp = sys_get_temp_dir() . '/phpa-trunc-' . md5(uniqid('', true)) . '.bin';
+        file_put_contents($tmp, "PK\x03\x04");
+        $zip = new Zip();
+        $zip->open($tmp);
+        try {
+            $this->expectException(ArchiveCorruptedException::class);
+            iterator_to_array($zip->yieldContents());
+        } finally {
+            try {
+                $zip->close();
+            } catch(\Exception $e) {
+                /* already errored */
+            }
+            @unlink($tmp);
+        }
+    }
+
     /**
      * simple test that checks that the given filenames and contents can be grepped from
      * the uncompressed zip stream