Eliminate resource leaks from zip::ZipFiles and

zip::ZipReader::OpenFromPlatformFile.

zip::ZipFiles and zip::ZipReader::OpenFromPlatformFile do not want to
take ownership of the passed-in file. On POSIX, dup the file descriptor
to pass to fopen, so that that we can safely fclose the result. On
Windows, stop closing the file handle. Fix consumers of these functions
that assumed ownership was being passed to instead close the file
themselves.

BUG=430959

Review URL: https://codereview.chromium.org/683913009

Cr-Commit-Position: refs/heads/master@{#304930}

4 files changed, 33 insertions, 14 deletions

diff --git a/third_party/zlib/google/zip.h b/third_party/zlib/google/zip.h
index 0232401..216b09e 100644
--- a/third_party/zlib/google/zip.h
+++ b/third_party/zlib/google/zip.h
@@ -30,9 +30,10 @@ bool Zip(const base::FilePath& src_dir, const base::FilePath& dest_file,
 
 #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.
+// descriptor |dest_fd|, without taking ownership of |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 base::FilePath& src_dir,
               const std::vector<base::FilePath>& src_relative_paths,
               int dest_fd);
diff --git a/third_party/zlib/google/zip_internal.cc b/third_party/zlib/google/zip_internal.cc
index d62e0bb..1f026c9 100644
--- a/third_party/zlib/google/zip_internal.cc
+++ b/third_party/zlib/google/zip_internal.cc
@@ -79,6 +79,8 @@ void* ZipOpenFunc(void *opaque, const char* filename, int mode) {
 
 #if defined(OS_POSIX)
 // Callback function for zlib that opens a file stream from a file descriptor.
+// Since we do not own the file descriptor, dup it so that we can fdopen/fclose
+// a file stream.
 void* FdOpenFileFunc(void* opaque, const char* filename, int mode) {
   FILE* file = NULL;
   const char* mode_fopen = NULL;
@@ -90,18 +92,18 @@ void* FdOpenFileFunc(void* opaque, const char* filename, int mode) {
   else if (mode & ZLIB_FILEFUNC_MODE_CREATE)
     mode_fopen = "wb";
 
-  if ((filename != NULL) && (mode_fopen != NULL))
-    file = fdopen(*static_cast<int*>(opaque), mode_fopen);
+  if ((filename != NULL) && (mode_fopen != NULL)) {
+    int fd = dup(*static_cast<int*>(opaque));
+    if (fd != -1)
+      file = fdopen(fd, mode_fopen);
+  }
 
   return file;
 }
 
-// We don't actually close the file stream since that would close
-// 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);
+int FdCloseFileFunc(void* opaque, void* stream) {
+  fclose(static_cast<FILE*>(stream));
+  free(opaque); // malloc'ed in FillFdOpenFileFunc()
   return 0;
 }
 
@@ -110,7 +112,7 @@ int CloseFileFunc(void* opaque, void* stream) {
 void FillFdOpenFileFunc(zlib_filefunc_def* pzlib_filefunc_def, int fd) {
   fill_fopen_filefunc(pzlib_filefunc_def);
   pzlib_filefunc_def->zopen_file = FdOpenFileFunc;
-  pzlib_filefunc_def->zclose_file = CloseFileFunc;
+  pzlib_filefunc_def->zclose_file = FdCloseFileFunc;
   int* ptr_fd = static_cast<int*>(malloc(sizeof(fd)));
   *ptr_fd = fd;
   pzlib_filefunc_def->opaque = ptr_fd;
@@ -119,6 +121,7 @@ void FillFdOpenFileFunc(zlib_filefunc_def* pzlib_filefunc_def, int fd) {
 
 #if defined(OS_WIN)
 // Callback function for zlib that opens a file stream from a Windows handle.
+// Does not take ownership of the handle.
 void* HandleOpenFileFunc(void* opaque, const char* filename, int mode) {
   WIN32FILE_IOWIN file_ret;
   file_ret.hf = static_cast<HANDLE>(opaque);
@@ -131,6 +134,11 @@ void* HandleOpenFileFunc(void* opaque, const char* filename, int mode) {
     *(static_cast<WIN32FILE_IOWIN*>(ret)) = file_ret;
   return ret;
 }
+
+int HandleCloseFileFunc(void* opaque, void* stream) {
+  free(stream); // malloc'ed in HandleOpenFileFunc()
+  return 0;
+}
 #endif
 
 // A struct that contains data required for zlib functions to extract files from
@@ -282,6 +290,7 @@ unzFile OpenHandleForUnzipping(HANDLE zip_handle) {
   zlib_filefunc_def zip_funcs;
   fill_win32_filefunc(&zip_funcs);
   zip_funcs.zopen_file = HandleOpenFileFunc;
+  zip_funcs.zclose_file = HandleCloseFileFunc;
   zip_funcs.opaque = zip_handle;
   return unzOpen2("fd", &zip_funcs);
 }
diff --git a/third_party/zlib/google/zip_reader.h b/third_party/zlib/google/zip_reader.h
index cda9cf7..9280f23 100644
--- a/third_party/zlib/google/zip_reader.h
+++ b/third_party/zlib/google/zip_reader.h
@@ -102,8 +102,8 @@ class ZipReader {
   // success.
   bool Open(const base::FilePath& zip_file_path);
 
-  // Opens the zip file referred to by the platform file |zip_fd|.
-  // Returns true on success.
+  // Opens the zip file referred to by the platform file |zip_fd|, without
+  // taking ownership of |zip_fd|. Returns true on success.
   bool OpenFromPlatformFile(base::PlatformFile zip_fd);
 
   // Opens the zip data stored in |data|. This class uses a weak reference to
diff --git a/third_party/zlib/google/zip_reader_unittest.cc b/third_party/zlib/google/zip_reader_unittest.cc
index 09fc241..d4bb579 100644
--- a/third_party/zlib/google/zip_reader_unittest.cc
+++ b/third_party/zlib/google/zip_reader_unittest.cc
@@ -573,4 +573,13 @@ TEST_F(ZipReaderTest, ExtractCurrentEntryToString) {
   reader.Close();
 }
 
+// This test exposes http://crbug.com/430959, at least on OS X
+TEST_F(ZipReaderTest, DISABLED_LeakDetectionTest) {
+  for (int i = 0; i < 100000; ++i) {
+    FileWrapper zip_fd_wrapper(test_zip_file_, FileWrapper::READ_ONLY);
+    ZipReader reader;
+    ASSERT_TRUE(reader.OpenFromPlatformFile(zip_fd_wrapper.platform_file()));
+  }
+}
+
 }  // namespace zip