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..c8b4dc17bce2b1e6b033fbcb340a8b1783157685 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,31 +299,55 @@ 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 = true;

[satorux1, 2017/07/27 07:05:31]

start with false and set it to true only when the entire file is read?

[mortonm, 2017/07/27 18:32:00]

Done.

   std::unique_ptr<char[]> buf(new char[internal::kZipBufSize]);
+  uint64_t total_num_bytes_read = 0;
   while (true) {
-    const int num_bytes_read = unzReadCurrentFile(zip_file_, buf.get(),
-                                                  internal::kZipBufSize);
+    uint64_t bytes_to_read = internal::kZipBufSize;
+    // This should never result in a negative value, as |num_bytes_to_extract|
+    // will always be greater than |total_num_bytes_read|.
+    auto unread_bytes = base::CheckedNumeric<uint64_t>(num_bytes_to_extract) -
+                        total_num_bytes_read;
+    if (unread_bytes.ValueOrDie() < internal::kZipBufSize) {
+      bytes_to_read = unread_bytes.ValueOrDie();
+    }
+
+    const int num_bytes_read =
+        unzReadCurrentFile(zip_file_, buf.get(), bytes_to_read);
+
     if (num_bytes_read == 0) {
-      // Reached the end of the file.
+      // Reached the end of the file without reaching |num_bytes_to_extract|.
+      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;
+      entire_file_extracted = false;
       break;
     } else if (num_bytes_read > 0) {
       // Some data is read.
       if (!delegate->WriteBytes(buf.get(), num_bytes_read)) {
-        success = false;
+        entire_file_extracted = false;
         break;
       }
     }
+    // Previous check ensures that num_bytes_read is not negative here.
+    total_num_bytes_read += base::checked_cast<uint64_t>(num_bytes_read);
+    if (total_num_bytes_read == num_bytes_to_extract) {

[satorux1, 2017/07/27 07:05:31]

Special-casing this case seems tricky. Maybe we can get rid of this by something
like below?

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

  if (num_bytes == 0) {
    entire_file_extracted = true;
    break;
  } else 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) {
    // Some data is read.
    auto unread_bytes = base::CheckedNumeric<uint64_t>(num_bytes_to_extract) -
                        total_num_bytes_read;
    uint_64 num_bytes_to_write = std::min(unread_bytes.ValueOrDie(),
                                          num_bytes_read);
    if (!delegate->WriteBytes(buf.get(), num_bytes_to_write))
      break;
  }
}
 

[satorux1, 2017/07/27 07:10:28]

we'd also need

  if (num_bytes_to_write == 0) 
    break;

[mortonm, 2017/07/27 18:32:00]

Done.

[mortonm, 2017/07/27 18:32:00]

Done.

+      // Read as much data as was specified by |num_bytes_to_extract|. Still
+      // checks whether entire file has been read, since |num_bytes_to_extract|
+      // may or may not equal file size.
+      if (unzReadCurrentFile(zip_file_, buf.get(), 1) == 0) {
+        entire_file_extracted = true;
+      } else {
+        entire_file_extracted = false;
+      }
+      break;
+    }
   }
 
   unzCloseCurrentFile(zip_file_);
 
-  return success;
+  return entire_file_extracted;
 }
 
 bool ZipReader::ExtractCurrentEntryToFilePath(
@@ -336,7 +361,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 +442,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 num_bytes,

[satorux1, 2017/07/27 07:05:31]

num_bytes -> max_read_bytes to be consistent with the header file

[mortonm, 2017/07/27 18:32:00]

Done.

                                             std::string* output) const {
   DCHECK(output);
   DCHECK(zip_file_);
-  DCHECK_NE(0U, max_read_bytes);
+
+  if (num_bytes == 0) {
+    output->clear();
+    return true;
+  }
 
   if (current_entry_info()->is_directory()) {
     output->clear();
@@ -436,12 +466,20 @@ 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>(num_bytes),
                                    current_entry_info()->original_size())));
 
-  StringWriterDelegate writer(max_read_bytes, &contents);
-  if (!ExtractCurrentEntry(&writer))
+  StringWriterDelegate writer(num_bytes, &contents);
+  if (!ExtractCurrentEntry(&writer, num_bytes)) {
+    if (contents.length() < num_bytes) {
+      // There was an error in extracting entry.

[satorux1, 2017/07/27 07:05:31]

I was a bit confused at first. Maybe add something like below?

There was an error. If the current entry is smaller than max_read_bytes,
ExtractCurrentEntry() should read the whole content and return true.

[mortonm, 2017/07/27 18:32:00]

Done. The revised comment now explains the situation more clearly.

+      output->clear();
+    } else {
+      // |num_bytes| is less than the length of current entry.
+      output->swap(contents);
+    }
     return false;
+  }
   output->swap(contents);
   return true;
 }