[zip] Add WriterDelegate::OnError()

Added an OnError() method allowing a WriterDelegate to do something in
case of extraction error.

FileWriterDelegate::OnError() empties the output file.
FilePathWriterDelegate::OnError() removes the output file altogether.

BUG=chromium:1304520
TEST=autoninja -C out/Default zlib_unittests && out/Default/zlib_unittests
TEST=autoninja -C out/Default components_unittests && out/Default/components_unittests --gtest_filter='UnzipTest.*'

Change-Id: I3c567773a8da8c8db055db04240e646f0fc94228
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3511757
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: François Degros <fdegros@chromium.org>
Cr-Commit-Position: refs/heads/main@{#980454}
NOKEYCHECK=True
GitOrigin-RevId: 1fb92cbdc817102903c7462066a8235268ede8e2

4 files changed, 68 insertions, 22 deletions

diff --git a/google/zip_reader.cc b/google/zip_reader.cc
index 01d06b0..33bf788 100644
--- a/google/zip_reader.cc
+++ b/google/zip_reader.cc
@@ -309,17 +309,19 @@ bool ZipReader::ExtractCurrentEntry(WriterDelegate* delegate,
     remaining_capacity -= num_bytes_to_write;
   }
 
+  if (const int err = unzCloseCurrentFile(zip_file_); err != UNZ_OK) {
+    LOG(ERROR) << "Cannot extract file " << Redact(entry_.path)
+               << " from ZIP: " << UnzipError(err);
+    entire_file_extracted = false;
+  }
+
   if (entire_file_extracted) {
     delegate->SetPosixFilePermissions(entry_.posix_mode);
     if (entry_.last_modified != base::Time::UnixEpoch()) {
       delegate->SetTimeModified(entry_.last_modified);
     }
-  }
-
-  if (const int err = unzCloseCurrentFile(zip_file_); err != UNZ_OK) {
-    LOG(ERROR) << "Cannot extract file " << Redact(entry_.path)
-               << " from ZIP: " << UnzipError(err);
-    return false;
+  } else {
+    delegate->OnError();
   }
 
   return entire_file_extracted;
@@ -525,6 +527,11 @@ void FileWriterDelegate::SetPosixFilePermissions(int mode) {
 #endif
 }
 
+void FileWriterDelegate::OnError() {
+  file_length_ = 0;
+  file_->SetLength(0);
+}
+
 // FilePathWriterDelegate ------------------------------------------------------
 
 FilePathWriterDelegate::FilePathWriterDelegate(base::FilePath output_file_path)
@@ -544,4 +551,14 @@ bool FilePathWriterDelegate::PrepareOutput() {
   return FileWriterDelegate::PrepareOutput();
 }
 
+void FilePathWriterDelegate::OnError() {
+  FileWriterDelegate::OnError();
+  owned_file_.Close();
+
+  if (!base::DeleteFile(output_file_path_)) {
+    LOG(ERROR) << "Cannot delete partially extracted file "
+               << Redact(output_file_path_);
+  }
+}
+
 }  // namespace zip
diff --git a/google/zip_reader.h b/google/zip_reader.h
index 1289fd3..6ca9cd9 100644
--- a/google/zip_reader.h
+++ b/google/zip_reader.h
@@ -48,6 +48,10 @@ class WriterDelegate {
   // may apply some of the permissions (for example, the executable bit) to the
   // output file.
   virtual void SetPosixFilePermissions(int mode) {}
+
+  // Called if an error occurred while extracting the file. The WriterDelegate
+  // can then remove and clean up the partially extracted data.
+  virtual void OnError() {}
 };
 
 // This class is used for reading ZIP archives. A typical use case of this class
@@ -291,8 +295,6 @@ class FileWriterDelegate : public WriterDelegate {
 
   ~FileWriterDelegate() override;
 
-  // WriterDelegate methods:
-
   // Returns true if the file handle passed to the constructor is valid.
   bool PrepareOutput() override;
 
@@ -307,6 +309,9 @@ class FileWriterDelegate : public WriterDelegate {
   // executable.
   void SetPosixFilePermissions(int mode) override;
 
+  // Empties the file to avoid leaving garbage data in it.
+  void OnError() override;
+
   // Gets the number of bytes written into the file.
   int64_t file_length() { return file_length_; }
 
@@ -331,11 +336,12 @@ class FilePathWriterDelegate : public FileWriterDelegate {
 
   ~FilePathWriterDelegate() override;
 
-  // WriterDelegate methods:
-
   // Creates the output file and any necessary intermediate directories.
   bool PrepareOutput() override;
 
+  // Deletes the file.
+  void OnError() override;
+
  private:
   const base::FilePath output_file_path_;
 };
diff --git a/google/zip_reader_unittest.cc b/google/zip_reader_unittest.cc
index 53155f6..fc80637 100644
--- a/google/zip_reader_unittest.cc
+++ b/google/zip_reader_unittest.cc
@@ -110,6 +110,7 @@ class MockWriterDelegate : public zip::WriterDelegate {
   MOCK_METHOD2(WriteBytes, bool(const char*, int));
   MOCK_METHOD1(SetTimeModified, void(const base::Time&));
   MOCK_METHOD1(SetPosixFilePermissions, void(int));
+  MOCK_METHOD0(OnError, void());
 };
 
 bool ExtractCurrentEntryToFilePath(zip::ZipReader* reader,
@@ -828,13 +829,14 @@ TEST_F(ZipReaderTest, ExtractCurrentEntryPrepareFailure) {
   ASSERT_FALSE(reader.ExtractCurrentEntry(&mock_writer));
 }
 
-// Test that when WriterDelegate::WriteBytes returns false, no other methods on
-// the delegate are called and the extraction fails.
+// Test that when WriterDelegate::WriteBytes returns false, only the OnError
+// method on the delegate is called and the extraction fails.
 TEST_F(ZipReaderTest, ExtractCurrentEntryWriteBytesFailure) {
   testing::StrictMock<MockWriterDelegate> mock_writer;
 
   EXPECT_CALL(mock_writer, PrepareOutput()).WillOnce(Return(true));
   EXPECT_CALL(mock_writer, WriteBytes(_, _)).WillOnce(Return(false));
+  EXPECT_CALL(mock_writer, OnError());
 
   base::FilePath target_path(FILE_PATH_LITERAL("foo/bar/quux.txt"));
   ZipReader reader;
@@ -921,4 +923,21 @@ TEST_F(FileWriterDelegateTest, WriteToEnd) {
   EXPECT_EQ(payload.size(), file_.GetLength());
 }
 
+TEST_F(FileWriterDelegateTest, EmptyOnError) {
+  const std::string payload = "This is the actualy payload data.\n";
+
+  {
+    FileWriterDelegate writer(&file_);
+    EXPECT_EQ(0, writer.file_length());
+    ASSERT_TRUE(writer.PrepareOutput());
+    ASSERT_TRUE(writer.WriteBytes(payload.data(), payload.size()));
+    EXPECT_EQ(payload.size(), writer.file_length());
+    EXPECT_EQ(payload.size(), file_.GetLength());
+    writer.OnError();
+    EXPECT_EQ(0, writer.file_length());
+  }
+
+  EXPECT_EQ(0, file_.GetLength());
+}
+
 }  // namespace zip
diff --git a/google/zip_unittest.cc b/google/zip_unittest.cc
index 433555d..ab86e88 100644
--- a/google/zip_unittest.cc
+++ b/google/zip_unittest.cc
@@ -461,11 +461,9 @@ TEST_F(ZipTest, UnzipEncryptedWithWrongPassword) {
                                      &contents));
   EXPECT_EQ("This is not encrypted.\n", contents);
 
-  // This extracted file contains rubbish data.
-  ASSERT_TRUE(base::ReadFileToString(
-      test_dir_.AppendASCII("Encrypted ZipCrypto.txt"), &contents));
-  EXPECT_NE("", contents);
-  EXPECT_NE("This is encrypted with ZipCrypto.\n", contents);
+  // No rubbish file should be left behind.
+  EXPECT_FALSE(
+      base::PathExists(test_dir_.AppendASCII("Encrypted ZipCrypto.txt")));
 }
 
 TEST_F(ZipTest, UnzipEncryptedWithNoPassword) {
@@ -483,11 +481,17 @@ TEST_F(ZipTest, UnzipEncryptedWithNoPassword) {
                                      &contents));
   EXPECT_EQ("This is not encrypted.\n", contents);
 
-  // This extracted file contains rubbish data.
-  ASSERT_TRUE(base::ReadFileToString(
-      test_dir_.AppendASCII("Encrypted ZipCrypto.txt"), &contents));
-  EXPECT_NE("", contents);
-  EXPECT_NE("This is encrypted with ZipCrypto.\n", contents);
+  // No rubbish file should be left behind.
+  EXPECT_FALSE(
+      base::PathExists(test_dir_.AppendASCII("Encrypted ZipCrypto.txt")));
+}
+
+TEST_F(ZipTest, UnzipWrongCrc) {
+  ASSERT_FALSE(
+      zip::Unzip(GetDataDirectory().AppendASCII("Wrong CRC.zip"), test_dir_));
+
+  // No rubbish file should be left behind.
+  EXPECT_FALSE(base::PathExists(test_dir_.AppendASCII("Corrupted.txt")));
 }
 
 TEST_F(ZipTest, UnzipWithDelegates) {