Change zip::ZipFiles() function to take a file descriptor for the dest zip file.

We want the browser process to validate the destination zip file path to make sure
it is not asked to overwrite an existing file or to a directory that could cause
security problems. By replacing the destination file path |dest_file| with a file
descriptor |dest_fd|: this CL allows the dest file to be opened and owned by the
browser process then passed via IPC to utility process.

There is no need to have a duplicate function that takes destination file path. The
new function will be used for CrOS only.

BUG=138359
TEST=builds, passes unit tests.

See also https://codereview.chromium.org/11392005/ for relevant discussion.

Review URL: https://chromiumcodereview.appspot.com/11413100

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@169090 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 39 insertions, 15 deletions

diff --git a/chrome/common/zip.cc b/chrome/common/zip.cc
index 3e2aa81..abd1247 100644
--- a/chrome/common/zip.cc
+++ b/chrome/common/zip.cc
@@ -170,17 +170,15 @@ bool Zip(const FilePath& src_dir, const FilePath& dest_file,
   }
 }
 
+#if defined(OS_POSIX)
 bool ZipFiles(const FilePath& src_dir,
               const std::vector<FilePath>& src_relative_paths,
-              const FilePath& dest_file) {
+              int dest_fd) {
   DCHECK(file_util::DirectoryExists(src_dir));
-
-  // TODO(hshi): check that |src_relative_paths| does not contain |dest_file|.
-  zipFile zip_file = internal::OpenForZipping(dest_file.AsUTF8Unsafe(),
-                                              APPEND_STATUS_CREATE);
+  zipFile zip_file = internal::OpenFdForZipping(dest_fd, APPEND_STATUS_CREATE);
 
   if (!zip_file) {
-    DLOG(WARNING) << "couldn't create file " << dest_file.value();
+    DLOG(ERROR) << "couldn't create file for fd " << dest_fd;
     return false;
   }
 
@@ -196,11 +194,12 @@ bool ZipFiles(const FilePath& src_dir,
   }
 
   if (ZIP_OK != zipClose(zip_file, NULL)) {
-    DLOG(ERROR) << "Error closing zip file " << dest_file.value();
+    DLOG(ERROR) << "Error closing zip file for fd " << dest_fd;
     success = false;
   }
 
   return success;
 }
+#endif  // defined(OS_POSIX)
 
 }  // namespace zip
diff --git a/chrome/common/zip.h b/chrome/common/zip.h
index feda101..c5f34e7 100644
--- a/chrome/common/zip.h
+++ b/chrome/common/zip.h
@@ -25,13 +25,15 @@ bool ZipWithFilterCallback(const FilePath& src_dir, const FilePath& dest_file,
 bool Zip(const FilePath& src_dir, const FilePath& dest_file,
          bool include_hidden_files);
 
-// Zips files listed in |src_relative_paths| to |dest_file|.
-// The paths listed in |src_relative_paths| are relative to |src_dir| and will
-// be used as the file names in the created zip file. All source paths must be
-// under |src_dir| in the file system hierarchy.
+#if defined(OS_POSIX)
+// Zips files listed in |src_relative_paths| to destination specified by file
+// descriptor |dest_fd|. The paths listed in |src_relative_paths| are relative
+// to the |src_dir| and will be used as the file names in the created zip file.
+// All source paths must be under |src_dir| in the file system hierarchy.
 bool ZipFiles(const FilePath& src_dir,
               const std::vector<FilePath>& src_relative_paths,
-              const FilePath& dest_file);
+              int dest_fd);
+#endif  // defined(OS_POSIX)
 
 // Unzip the contents of zip_file into dest_dir.
 bool Unzip(const FilePath& zip_file, const FilePath& dest_dir);
diff --git a/chrome/common/zip_internal.cc b/chrome/common/zip_internal.cc
index fe84696..8d0d423 100644
--- a/chrome/common/zip_internal.cc
+++ b/chrome/common/zip_internal.cc
@@ -97,9 +97,10 @@ void* FdOpenFileFunc(void* opaque, const char* filename, int mode) {
 }
 
 // We don't actually close the file stream since that would close
-// the underlying file descriptor, and we don't own it. We do free
-// |opaque| since we malloc'ed it in FillFdOpenFileFunc.
+// the underlying file descriptor, and we don't own it. However we do need to
+// flush buffers and free |opaque| since we malloc'ed it in FillFdOpenFileFunc.
 int CloseFileFunc(void* opaque, void* stream) {
+  fflush(static_cast<FILE*>(stream));
   free(opaque);
   return 0;
 }
@@ -276,5 +277,14 @@ zipFile OpenForZipping(const std::string& file_name_utf8, int append_flag) {
                   zip_func_ptrs);
 }
 
+#if defined(OS_POSIX)
+zipFile OpenFdForZipping(int zip_fd, int append_flag) {
+  zlib_filefunc_def zip_funcs;
+  FillFdOpenFileFunc(&zip_funcs, zip_fd);
+  // Passing dummy "fd" filename to zlib.
+  return zipOpen2("fd", append_flag, NULL, &zip_funcs);
+}
+#endif
+
 }  // namespace internal
 }  // namespace zip
diff --git a/chrome/common/zip_internal.h b/chrome/common/zip_internal.h
index 3bf8683..cc13a71 100644
--- a/chrome/common/zip_internal.h
+++ b/chrome/common/zip_internal.h
@@ -38,6 +38,12 @@ unzFile PreprareMemoryForUnzipping(const std::string& data);
 // Windows. |append_flag| will be passed to zipOpen2().
 zipFile OpenForZipping(const std::string& file_name_utf8, int append_flag);
 
+#if defined(OS_POSIX)
+// Opens the file referred to by |zip_fd| for zipping. |append_flag| will be
+// passed to zipOpen2().
+zipFile OpenFdForZipping(int zip_fd, int append_flag);
+#endif
+
 const int kZipMaxPath = 256;
 const int kZipBufSize = 8192;
 
diff --git a/chrome/common/zip_unittest.cc b/chrome/common/zip_unittest.cc
index 4d3d5d8..2496170 100644
--- a/chrome/common/zip_unittest.cc
+++ b/chrome/common/zip_unittest.cc
@@ -160,6 +160,7 @@ TEST_F(ZipTest, ZipIgnoreHidden) {
   TestUnzipFile(zip_file, false);
 }
 
+#if defined(OS_POSIX)
 TEST_F(ZipTest, ZipFiles) {
   FilePath src_dir;
   ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &src_dir));
@@ -169,7 +170,12 @@ TEST_F(ZipTest, ZipFiles) {
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
   FilePath zip_file = temp_dir.path().AppendASCII("out.zip");
 
-  EXPECT_TRUE(zip::ZipFiles(src_dir, zip_file_list_, zip_file));
+  const int flags = base::PLATFORM_FILE_CREATE | base::PLATFORM_FILE_WRITE;
+  const base::PlatformFile zip_fd =
+      base::CreatePlatformFile(zip_file, flags, NULL, NULL);
+  ASSERT_LE(0, zip_fd);
+  EXPECT_TRUE(zip::ZipFiles(src_dir, zip_file_list_, zip_fd));
+  base::ClosePlatformFile(zip_fd);
 
   zip::ZipReader reader;
   EXPECT_TRUE(reader.Open(zip_file));
@@ -181,6 +187,7 @@ TEST_F(ZipTest, ZipFiles) {
     EXPECT_EQ(entry_info->file_path(), zip_file_list_[i]);
   }
 }
+#endif  // defined(OS_POSIX)
 
 }  // namespace