Improve Zip File Scanning on Mac

third_party/zlib/google/zip_reader.cc

Index: third_party/zlib/google/zip_reader.cc
diff --git a/third_party/zlib/google/zip_reader.cc b/third_party/zlib/google/zip_reader.cc
index ff5a39514af113d28f5266d2c119d51a4d386328..3cd308db26b6a5767e83efaf8c3b079c0da574ba 100644
--- a/third_party/zlib/google/zip_reader.cc
+++ b/third_party/zlib/google/zip_reader.cc
@@ -289,7 +289,8 @@ bool ZipReader::LocateAndOpenEntry(const base::FilePath& path_in_zip) {
   return OpenCurrentEntryInZip();
 }
 
-bool ZipReader::ExtractCurrentEntry(WriterDelegate* delegate) const {
+bool ZipReader::ExtractCurrentEntry(WriterDelegate* delegate,
+                                    uint64_t num_bytes_to_extract) const {
   DCHECK(zip_file_);
 
   const int open_result = unzOpenCurrentFile(zip_file_);
@@ -298,23 +299,36 @@ bool ZipReader::ExtractCurrentEntry(WriterDelegate* delegate) const {
 
   if (!delegate->PrepareOutput())
     return false;
-
-  bool success = true;  // This becomes false when something bad happens.
+  bool entire_file_extracted = false;
   std::unique_ptr<char[]> buf(new char[internal::kZipBufSize]);
+  uint64_t total_num_bytes_extracted = 0;
+
   while (true) {
-    const int num_bytes_read = unzReadCurrentFile(zip_file_, buf.get(),
-                                                  internal::kZipBufSize);
+    const int num_bytes_read =
+        unzReadCurrentFile(zip_file_, buf.get(), internal::kZipBufSize);
+
     if (num_bytes_read == 0) {
-      // Reached the end of the file.
+      entire_file_extracted = true;
       break;
     } else if (num_bytes_read < 0) {
       // If num_bytes_read < 0, then it's a specific UNZ_* error code.
-      success = false;
       break;
     } else if (num_bytes_read > 0) {
       // Some data is read.
-      if (!delegate->WriteBytes(buf.get(), num_bytes_read)) {
-        success = false;
+      auto unread_bytes = base::CheckedNumeric<uint64_t>(num_bytes_to_extract) -
+                          total_num_bytes_extracted;
+      uint64_t num_bytes_to_write =
+          std::min<uint64_t>(unread_bytes.ValueOrDie(),
+                             base::checked_cast<uint64_t>(num_bytes_read));
+      total_num_bytes_extracted += num_bytes_to_write;
+      if (!delegate->WriteBytes(buf.get(), num_bytes_to_write) ||
+          num_bytes_to_extract == total_num_bytes_extracted) {
+        // Ensures function returns true if the entire file has been read.
+        if (num_bytes_to_write ==
+                base::checked_cast<uint64_t>(num_bytes_read) &&
+            unzReadCurrentFile(zip_file_, buf.get(), 1) == 0) {

[satorux1, 2017/07/27 23:18:10]

I thought we could get rid of the call to unzReadCurrentFile() here, since the
while loop will end at the first |if| conditional in the loop, if the entire
file is read:

310   if (num_bytes_read == 0) {
311     entire_file_extracted = true;
312     break;
313   }

Here, can we go with something like below?

  auto unread_bytes = ...;
  if (unread_bytes.ValueOrDie() == 0)
    break;

  uint64_t uint64_t num_bytes_to_write = ...;
  if (!delegate->WriteBytes(buf.get(), num_bytes_to_write)
    break;

[mortonm, 2017/07/31 18:13:04]

Yeah, I think I originally wrote the function this way as well. The problem with
this approach is that it will cause the function to return false if the
specified |num_bytes_to_extract| parameter equals the size of the entry being
extracted. The check on line 325 checks to see if the |num_bytes_to_extract| cap
has been hit while performing extraction. If so, we usually want to return
false, except when |num_bytes_to_extract| equals the size of the entry, in which
case we want to return true. However, in line with your comment, I've re-written
the code to avoid doing the second call to unzReadCurrentFile().

[satorux1, 2017/08/01 08:01:08]

Ah you are right. Sorry for missing that point.
I think the new version with two additional booleans is more complicated than
the previous version. :)

I think your previous version could be a bit simplified if we keep track of the
remaining capacity of the delegate than the total bytes extracted:

  uint64_t remaining_capacity = num_bytes_to_extract;
  bool entire_file_extracted = false;

  while (remaining_capacity > 0) {
    const int num_bytes_read =
        unzReadCurrentFile(zip_file_, buf.get(), internal::kZipBufSize);

    if (num_bytes_read == 0) {
      entire_file_extracted = true;
      break;
    }
    if (num_bytes_read < 0) {
      // If num_bytes_read < 0, then it's a specific UNZ_* error code.          
           
      break;
    } else if (num_bytes_read > 0) {
      uint64_t num_bytes_to_write =
          std::min<uint64_t>(remaining_capacity,
                             base::checked_cast<uint64_t>(num_bytes_read));
      if (!delegate->WriteBytes(buf.get(), num_bytes_to_write))
         break;
      if (remaining_capacity == base::checked_cast<uint64_t>(num_bytes_read)) {
        // Ensures function returns true if the entire file has been read.
        entire_file_extracted =
            (unzReadCurrentFile(zip_file_, buf.get(), 1) == 0);
      }
      CHECK_GE(remaining_capacity, num_bytes_to_write);
      remaining_capacity -= num_bytes_to_write;
    }
  }

[mortonm, 2017/08/01 16:16:46]

Done.

+          entire_file_extracted = true;
+        }
         break;
       }
     }
@@ -322,7 +336,7 @@ bool ZipReader::ExtractCurrentEntry(WriterDelegate* delegate) const {
 
   unzCloseCurrentFile(zip_file_);
 
-  return success;
+  return entire_file_extracted;
 }
 
 bool ZipReader::ExtractCurrentEntryToFilePath(
@@ -336,7 +350,8 @@ bool ZipReader::ExtractCurrentEntryToFilePath(
   bool success = false;
   {
     FilePathWriterDelegate writer(output_file_path);
-    success = ExtractCurrentEntry(&writer);
+    success =
+        ExtractCurrentEntry(&writer, std::numeric_limits<uint64_t>::max());
   }
 
   if (success &&
@@ -416,14 +431,18 @@ bool ZipReader::ExtractCurrentEntryToFile(base::File* file) const {
     return false;
 
   FileWriterDelegate writer(file);
-  return ExtractCurrentEntry(&writer);
+  return ExtractCurrentEntry(&writer, std::numeric_limits<uint64_t>::max());
 }
 
-bool ZipReader::ExtractCurrentEntryToString(size_t max_read_bytes,
+bool ZipReader::ExtractCurrentEntryToString(uint64_t max_read_bytes,
                                             std::string* output) const {
   DCHECK(output);
   DCHECK(zip_file_);
-  DCHECK_NE(0U, max_read_bytes);
+
+  if (max_read_bytes == 0) {
+    output->clear();
+    return true;
+  }
 
   if (current_entry_info()->is_directory()) {
     output->clear();
@@ -436,12 +455,23 @@ bool ZipReader::ExtractCurrentEntryToString(size_t max_read_bytes,
   // incorrect therefore this function needs to read as much data as possible.
   std::string contents;
   contents.reserve(
-      static_cast<size_t>(std::min(static_cast<int64_t>(max_read_bytes),
+      static_cast<size_t>(std::min(base::checked_cast<int64_t>(max_read_bytes),
                                    current_entry_info()->original_size())));
 
   StringWriterDelegate writer(max_read_bytes, &contents);
-  if (!ExtractCurrentEntry(&writer))
+  if (!ExtractCurrentEntry(&writer, max_read_bytes)) {
+    if (contents.length() < max_read_bytes) {
+      // There was an error in extracting entry. If ExtractCurrentEntry()
+      // returns false, the entire file was not read - in which case
+      // contents.length() should equal |max_read_bytes| unless an error
+      // occurred which caused extraction to be aborted.
+      output->clear();
+    } else {
+      // |num_bytes| is less than the length of current entry.
+      output->swap(contents);
+    }
     return false;
+  }
   output->swap(contents);
   return true;
 }