[zip] Unzip() does not overwrite files anymore

Changed the way FilePathWriterDelegate creates a new file, by using
FLAG_CREATE instead of FLAG_CREATE_ALWAYS.

This prevents FilePathWriterDelegate and zip::Unzip() from overwriting
any existing file. This allows the detection of duplicated or
conflicting file names when extracting a ZIP archive. See
ZipTest.UnzipRepeatedFileName.

This fixes a confusing corner case on case-insensitive file systems,
which could result in file contents not matching file names as stored in
the ZIP archive. See ZipTest.UnzipDifferentCases.

BUG=chromium:953256
TEST=autoninja -C out/Default zlib_unittests && out/Default/zlib_unittests

Change-Id: I6cf80e6d8b9e39ffc76739c00cb6f2ecf57da88f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3529591


Reviewed-by: Alex Danilo <adanilo@chromium.org>
Commit-Queue: François Degros <fdegros@chromium.org>
Cr-Commit-Position: refs/heads/main@{#982048}
NOKEYCHECK=True
GitOrigin-RevId: 7ba6004ba3727e9f5339841cfe0f54e7e51dfbf2

google/zip.cc

@@ -10,6 +10,7 @@
 #include "base/bind.h"
 #include "base/files/file.h"
 #include "base/files/file_enumerator.h"
+#include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/string_util.h"
@@ -180,6 +181,9 @@ bool Unzip(const base::FilePath& src_file,
     return false;
   }
 
+  DLOG_IF(WARNING, !base::IsDirectoryEmpty(dest_dir))
+      << "ZIP extraction directory is not empty: " << dest_dir;
+
   return Unzip(file.GetPlatformFile(),
                base::BindRepeating(&CreateFilePathWriterDelegate, dest_dir),
                base::BindRepeating(&CreateDirectory, dest_dir),

google/zip.h

@@ -198,7 +198,9 @@ bool Unzip(const base::PlatformFile& zip_file,
            UnzipOptions options = {});
 
 // Unzips the contents of |zip_file| into |dest_dir|.
-// WARNING: This can overwrite existing files in |dest_dir|.
+// This function does not overwrite any existing file.
+// A filename collision will result in an error.
+// Therefore, |dest_dir| should initially be an empty directory.
 bool Unzip(const base::FilePath& zip_file,
            const base::FilePath& dest_dir,
            UnzipOptions options = {});

google/zip_reader.cc

@@ -550,8 +550,8 @@ bool FilePathWriterDelegate::PrepareOutput() {
     return false;
   }
 
-  owned_file_.Initialize(output_file_path_, base::File::FLAG_CREATE_ALWAYS |
-                                                base::File::FLAG_WRITE);
+  owned_file_.Initialize(output_file_path_,
+                         base::File::FLAG_CREATE | base::File::FLAG_WRITE);
   PLOG_IF(ERROR, !owned_file_.IsValid())
       << "Cannot create file " << Redact(output_file_path_) << ": "
       << base::File::ErrorToString(owned_file_.error_details());

google/zip_reader.h

@@ -326,7 +326,8 @@ class FileWriterDelegate : public WriterDelegate {
   int64_t file_length_ = 0;
 };
 
-// A writer delegate that writes a file at a given path.
+// A writer delegate that creates and writes a file at a given path. This does
+// not overwrite any existing file.
 class FilePathWriterDelegate : public FileWriterDelegate {
  public:
   explicit FilePathWriterDelegate(base::FilePath output_file_path);
@@ -336,10 +337,12 @@ class FilePathWriterDelegate : public FileWriterDelegate {
 
   ~FilePathWriterDelegate() override;
 
-  // Creates the output file and any necessary intermediate directories.
+  // Creates the output file and any necessary intermediate directories. Does
+  // not overwrite any existing file, and returns false if the output file
+  // cannot be created because another file conflicts with it.
   bool PrepareOutput() override;
 
-  // Deletes the file.
+  // Deletes the output file.
   void OnError() override;
 
  private:

google/zip_unittest.cc

@@ -517,7 +517,7 @@ TEST_F(ZipTest, UnzipRepeatedDirName) {
 }
 
 TEST_F(ZipTest, UnzipRepeatedFileName) {
-  EXPECT_TRUE(zip::Unzip(
+  EXPECT_FALSE(zip::Unzip(
       GetDataDirectory().AppendASCII("Repeated File Name.zip"), test_dir_));
 
   EXPECT_THAT(
@@ -527,7 +527,7 @@ TEST_F(ZipTest, UnzipRepeatedFileName) {
   std::string contents;
   EXPECT_TRUE(
       base::ReadFileToString(test_dir_.AppendASCII("repeated"), &contents));
-  EXPECT_EQ("Second file", contents);
+  EXPECT_EQ("First file", contents);
 }
 
 TEST_F(ZipTest, UnzipCannotCreateEmptyDir) {
@@ -602,22 +602,25 @@ TEST_F(ZipTest, UnzipWindowsSpecialNames) {
 }
 
 TEST_F(ZipTest, UnzipDifferentCases) {
-  EXPECT_TRUE(zip::Unzip(GetDataDirectory().AppendASCII(
-                             "Repeated File Name With Different Cases.zip"),
-                         test_dir_));
-
 #if defined(OS_WIN) || defined(OS_MAC)
-  // There is only one file, with the name of the first file but the contents of
-  // the last file.
+  // Only the first file (with mixed case) is extracted.
+  EXPECT_FALSE(zip::Unzip(GetDataDirectory().AppendASCII(
+                              "Repeated File Name With Different Cases.zip"),
+                          test_dir_));
+
   EXPECT_THAT(
       GetRelativePaths(test_dir_, base::FileEnumerator::FileType::FILES),
       UnorderedElementsAre("Case"));
 
   std::string contents;
   EXPECT_TRUE(base::ReadFileToString(test_dir_.AppendASCII("Case"), &contents));
-  EXPECT_EQ("Upper case 3", contents);
+  EXPECT_EQ("Mixed case 111", contents);
 #else
-  // All the files are correctly extracted.
+  // All the files are extracted.
+  EXPECT_TRUE(zip::Unzip(GetDataDirectory().AppendASCII(
+                             "Repeated File Name With Different Cases.zip"),
+                         test_dir_));
+
   EXPECT_THAT(
       GetRelativePaths(test_dir_, base::FileEnumerator::FileType::FILES),
       UnorderedElementsAre("Case", "case", "CASE"));