Merge "Prevent the accidental closure of fd[0] for missing zip files."

2 files changed, 33 insertions, 22 deletions

diff --git a/libziparchive/zip_archive.cc b/libziparchive/zip_archive.cc
index 128bad4..6ec8f0d 100644
--- a/libziparchive/zip_archive.cc
+++ b/libziparchive/zip_archive.cc
@@ -287,7 +287,7 @@ static const char kTempMappingFileName[] = "zip: ExtractFileToFile";
  */
 struct ZipArchive {
   /* open Zip archive */
-  int fd;
+  const int fd;
 
   /* mapped central directory area */
   off64_t directory_offset;
@@ -304,6 +304,25 @@ struct ZipArchive {
    */
   uint32_t hash_table_size;
   ZipEntryName* hash_table;
+
+  ZipArchive(const int fd) :
+      fd(fd),
+      directory_offset(0),
+      directory_map(NULL),
+      num_entries(0),
+      hash_table_size(0),
+      hash_table(NULL) {}
+
+  ~ZipArchive() {
+    if (fd >= 0) {
+      close(fd);
+    }
+
+    if (directory_map != NULL) {
+      directory_map->release();
+    }
+    free(hash_table);
+  }
 };
 
 // Returns 0 on success and negative values on failure.
@@ -661,28 +680,20 @@ static int32_t OpenArchiveInternal(ZipArchive* archive,
 
 int32_t OpenArchiveFd(int fd, const char* debug_file_name,
                       ZipArchiveHandle* handle) {
-  ZipArchive* archive = (ZipArchive*) malloc(sizeof(ZipArchive));
-  memset(archive, 0, sizeof(*archive));
+  ZipArchive* archive = new ZipArchive(fd);
   *handle = archive;
-
-  archive->fd = fd;
-
   return OpenArchiveInternal(archive, debug_file_name);
 }
 
 int32_t OpenArchive(const char* fileName, ZipArchiveHandle* handle) {
-  ZipArchive* archive = (ZipArchive*) malloc(sizeof(ZipArchive));
-  memset(archive, 0, sizeof(*archive));
+  const int fd = open(fileName, O_RDONLY | O_BINARY, 0);
+  ZipArchive* archive = new ZipArchive(fd);
   *handle = archive;
 
-  const int fd = open(fileName, O_RDONLY | O_BINARY, 0);
   if (fd < 0) {
     ALOGW("Unable to open '%s': %s", fileName, strerror(errno));
     return kIoError;
-  } else {
-    archive->fd = fd;
   }
-
   return OpenArchiveInternal(archive, fileName);
 }
 
@@ -692,16 +703,7 @@ int32_t OpenArchive(const char* fileName, ZipArchiveHandle* handle) {
 void CloseArchive(ZipArchiveHandle handle) {
   ZipArchive* archive = (ZipArchive*) handle;
   ALOGV("Closing archive %p", archive);
-
-  if (archive->fd >= 0) {
-    close(archive->fd);
-  }
-
-  if (archive->directory_map != NULL) {
-    archive->directory_map->release();
-  }
-  free(archive->hash_table);
-  free(archive);
+  delete archive;
 }
 
 static int32_t UpdateEntryFromDataDescriptor(int fd,
diff --git a/libziparchive/zip_archive_test.cc b/libziparchive/zip_archive_test.cc
index 875b6de..813a87f 100644
--- a/libziparchive/zip_archive_test.cc
+++ b/libziparchive/zip_archive_test.cc
@@ -26,6 +26,7 @@
 
 static std::string test_data_dir;
 
+static const std::string kMissingZip = "missing.zip";
 static const std::string kValidZip = "valid.zip";
 
 static const uint8_t kATxtContents[] = {
@@ -58,6 +59,14 @@ TEST(ziparchive, Open) {
   CloseArchive(handle);
 }
 
+TEST(ziparchive, OpenMissing) {
+  ZipArchiveHandle handle;
+  ASSERT_NE(0, OpenArchiveWrapper(kMissingZip, &handle));
+
+  // Confirm the file descriptor is not going to be mistaken for a valid one.
+  ASSERT_EQ(-1, GetFileDescriptor(handle));
+}
+
 TEST(ziparchive, Iteration) {
   ZipArchiveHandle handle;
   ASSERT_EQ(0, OpenArchiveWrapper(kValidZip, &handle));