Clean up: Pass File* to DexFileLoader instead of fd.

The DexFileLoader used to always close the fd, which led to
need to duplicate the fds using DupCloexec, or it was easy
to accidentally close the fd while it was still needed.

Make the DexFileLoader take File pointer, which moves the
lifetime management responsibilities to the caller.
The DexFileLoader never closes a file that it didn't open.

Test: ./art/test.py -b --host --64 --optimizing
Change-Id: Ie428edcae6c9062845c57c53528baee249046d70

12 files changed, 65 insertions, 45 deletions

diff --git a/dex2oat/linker/oat_writer.cc b/dex2oat/linker/oat_writer.cc
index cb97a1a515..205b5efde6 100644
--- a/dex2oat/linker/oat_writer.cc
+++ b/dex2oat/linker/oat_writer.cc
@@ -398,7 +398,7 @@ bool OatWriter::AddDexFileSource(const char* filename, const char* location) {
 bool OatWriter::AddDexFileSource(File&& dex_file_fd, const char* location) {
   DCHECK(write_state_ == WriteState::kAddingDexFileSources);
   std::string error_msg;
-  ArtDexFileLoader loader(dex_file_fd.Release(), location);
+  ArtDexFileLoader loader(&dex_file_fd, location);
   std::vector<std::unique_ptr<const DexFile>> dex_files;
   if (!loader.Open(/*verify=*/false,
                    /*verify_checksum=*/false,
diff --git a/libdexfile/dex/art_dex_file_loader_test.cc b/libdexfile/dex/art_dex_file_loader_test.cc
index 4663a02ff0..23ffad6e78 100644
--- a/libdexfile/dex/art_dex_file_loader_test.cc
+++ b/libdexfile/dex/art_dex_file_loader_test.cc
@@ -65,7 +65,7 @@ TEST_F(ArtDexFileLoaderTest, OpenZipMultiDex) {
   ASSERT_GE(file.Fd(), 0);
   std::vector<std::unique_ptr<const DexFile>> dex_files;
   std::string error_msg;
-  ArtDexFileLoader dex_file_loader(file.Release(), zip_file);
+  ArtDexFileLoader dex_file_loader(&file, zip_file);
   ASSERT_TRUE(dex_file_loader.Open(/*verify=*/false,
                                    /*verify_checksum=*/true,
                                    /*allow_no_dex_files=*/true,
@@ -81,7 +81,7 @@ TEST_F(ArtDexFileLoaderTest, OpenZipEmpty) {
   ASSERT_GE(file.Fd(), 0);
   std::vector<std::unique_ptr<const DexFile>> dex_files;
   std::string error_msg;
-  ArtDexFileLoader dex_file_loader(file.Release(), zip_file);
+  ArtDexFileLoader dex_file_loader(&file, zip_file);
   ASSERT_TRUE(dex_file_loader.Open(/*verify=*/false,
                                    /*verify_checksum=*/true,
                                    /*allow_no_dex_files=*/true,
diff --git a/libdexfile/dex/dex_file_loader.cc b/libdexfile/dex/dex_file_loader.cc
index 9b54c15d9e..e0d4c77eea 100644
--- a/libdexfile/dex/dex_file_loader.cc
+++ b/libdexfile/dex/dex_file_loader.cc
@@ -120,6 +120,8 @@ class MemMapContainer : public DexFileContainer {
 
 }  // namespace
 
+const File DexFileLoader::kInvalidFile;
+
 bool DexFileLoader::IsMagicValid(uint32_t magic) {
   return IsMagicValid(reinterpret_cast<uint8_t*>(&magic));
 }
@@ -168,7 +170,7 @@ bool DexFileLoader::GetMultiDexChecksum(std::optional<uint32_t>* checksum,
 
   if (IsZipMagic(magic)) {
     std::unique_ptr<ZipArchive> zip_archive(
-        file_.has_value() ?
+        file_->IsValid() ?
             ZipArchive::OpenFromOwnedFd(file_->Fd(), location_.c_str(), error_msg) :
             ZipArchive::OpenFromMemory(
                 root_container_->Begin(), root_container_->Size(), location_.c_str(), error_msg));
@@ -285,13 +287,14 @@ bool DexFileLoader::InitAndReadMagic(uint32_t* magic, std::string* error_msg) {
     *magic = *reinterpret_cast<const uint32_t*>(root_container_->Begin());
   } else {
     // Open the file if we have not been given the file-descriptor directly before.
-    if (!file_.has_value()) {
+    if (!file_->IsValid()) {
       CHECK(!filename_.empty());
-      file_.emplace(filename_, O_RDONLY, /* check_usage= */ false);
-      if (file_->Fd() == -1) {
+      owned_file_ = File(filename_, O_RDONLY, /* check_usage= */ false);
+      if (!owned_file_->IsValid()) {
         *error_msg = StringPrintf("Unable to open '%s' : %s", filename_.c_str(), strerror(errno));
         return false;
       }
+      file_ = &owned_file_.value();
     }
     if (!ReadMagicAndReset(file_->Fd(), magic, error_msg)) {
       return false;
@@ -306,7 +309,7 @@ bool DexFileLoader::MapRootContainer(std::string* error_msg) {
   }
 
   CHECK(MemMap::IsInitialized());
-  CHECK(file_.has_value());
+  CHECK(file_->IsValid());
   struct stat sbuf;
   memset(&sbuf, 0, sizeof(sbuf));
   if (fstat(file_->Fd(), &sbuf) == -1) {
@@ -350,7 +353,7 @@ bool DexFileLoader::Open(bool verify,
 
   if (IsZipMagic(magic)) {
     std::unique_ptr<ZipArchive> zip_archive(
-        file_.has_value() ?
+        file_->IsValid() ?
             ZipArchive::OpenFromOwnedFd(file_->Fd(), location_.c_str(), error_msg) :
             ZipArchive::OpenFromMemory(
                 root_container_->Begin(), root_container_->Size(), location_.c_str(), error_msg));
@@ -487,7 +490,7 @@ bool DexFileLoader::OpenFromZipEntry(const ZipArchive& zip_archive,
   CHECK(MemMap::IsInitialized());
   MemMap map;
   bool is_file_map = false;
-  if (file_.has_value() && zip_entry->IsUncompressed()) {
+  if (file_->IsValid() && zip_entry->IsUncompressed()) {
     if (!zip_entry->IsAlignedTo(alignof(DexFile::Header))) {
       // Do not mmap unaligned ZIP entries because
       // doing so would fail dex verification which requires 4 byte alignment.
diff --git a/libdexfile/dex/dex_file_loader.h b/libdexfile/dex/dex_file_loader.h
index 14cbdadaad..ec5f64a9d6 100644
--- a/libdexfile/dex/dex_file_loader.h
+++ b/libdexfile/dex/dex_file_loader.h
@@ -153,14 +153,14 @@ class DexFileLoader {
     return (pos == std::string::npos) ? std::string() : location.substr(pos);
   }
 
-  DexFileLoader(const char* filename, int fd, const std::string& location)
-      : filename_(filename),
-        file_(fd == -1 ? std::optional<File>() : File(fd, /*check_usage=*/false)),
-        location_(location) {}
+  DexFileLoader(const char* filename, const File* file, const std::string& location)
+      : filename_(filename), file_(file), location_(location) {
+    CHECK(file != nullptr);  // Must be non-null, but may be invalid.
+  }
 
   DexFileLoader(std::shared_ptr<DexFileContainer> container, const std::string& location)
       : root_container_(std::move(container)), location_(location) {
-    DCHECK(root_container_ != nullptr);
+    CHECK(root_container_ != nullptr);
   }
 
   DexFileLoader(const uint8_t* base, size_t size, const std::string& location);
@@ -169,14 +169,14 @@ class DexFileLoader {
 
   DexFileLoader(MemMap&& mem_map, const std::string& location);
 
-  DexFileLoader(int fd, const std::string& location)
-      : DexFileLoader(/*filename=*/location.c_str(), fd, location) {}
+  DexFileLoader(File* file, const std::string& location)
+      : DexFileLoader(/*filename=*/location.c_str(), file, location) {}
 
   DexFileLoader(const char* filename, const std::string& location)
-      : DexFileLoader(filename, /*fd=*/-1, location) {}
+      : DexFileLoader(filename, /*file=*/&kInvalidFile, location) {}
 
   explicit DexFileLoader(const std::string& location)
-      : DexFileLoader(location.c_str(), /*fd=*/-1, location) {}
+      : DexFileLoader(location.c_str(), /*file=*/&kInvalidFile, location) {}
 
   virtual ~DexFileLoader() {}
 
@@ -241,6 +241,8 @@ class DexFileLoader {
   }
 
  protected:
+  static const File kInvalidFile;  // Used for "no file descriptor" (-1).
+
   bool InitAndReadMagic(uint32_t* magic, std::string* error_msg);
 
   // Ensure we have root container.  If we are backed by a file, memory-map it.
@@ -297,7 +299,8 @@ class DexFileLoader {
   // The DexFileLoader can be backed either by file or by memory (i.e. DexFileContainer).
   // We can not just mmap the file since APKs might be unreasonably large for 32-bit system.
   std::string filename_;
-  std::optional<File> file_;
+  const File* file_ = &kInvalidFile;
+  std::optional<File> owned_file_;  // May be used as backing storage for 'file_'.
   std::shared_ptr<DexFileContainer> root_container_;
   const std::string location_;
 };
diff --git a/profman/profman.cc b/profman/profman.cc
index 63bc861436..25f03feacf 100644
--- a/profman/profman.cc
+++ b/profman/profman.cc
@@ -610,7 +610,8 @@ class ProfMan final {
       std::vector<std::unique_ptr<const DexFile>> dex_files_for_location;
       // We do not need to verify the apk for processing profiles.
       if (use_apk_fd_list) {
-          ArtDexFileLoader dex_file_loader(apks_fd_[i], dex_locations_[i]);
+          File file(apks_fd_[i], /*check_usage=*/false);
+          ArtDexFileLoader dex_file_loader(&file, dex_locations_[i]);
           if (dex_file_loader.Open(/*verify=*/false,
                                    kVerifyChecksum,
                                    /*allow_no_dex_files=*/true,
@@ -626,7 +627,7 @@ class ProfMan final {
           PLOG(ERROR) << "Unable to open '" << apk_files_[i] << "'";
           return false;
         }
-        ArtDexFileLoader dex_file_loader(file.Release(), dex_locations_[i]);
+        ArtDexFileLoader dex_file_loader(&file, dex_locations_[i]);
         if (dex_file_loader.Open(/*verify=*/false,
                                  kVerifyChecksum,
                                  /*allow_no_dex_files=*/true,
diff --git a/runtime/class_loader_context.cc b/runtime/class_loader_context.cc
index 73546ed0a9..6dc94fbd25 100644
--- a/runtime/class_loader_context.cc
+++ b/runtime/class_loader_context.cc
@@ -462,7 +462,7 @@ bool ClassLoaderContext::OpenDexFiles(const std::string& classpath_dir,
       // If file descriptors were provided for the class loader context dex paths,
       // get the descriptor which corresponds to this dex path. We assume the `fds`
       // vector follows the same order as a flattened class loader context.
-      int fd = -1;
+      File file;
       if (!fds.empty()) {
         if (dex_file_index >= fds.size()) {
           LOG(WARNING) << "Number of FDs is smaller than number of dex files in the context";
@@ -470,32 +470,35 @@ bool ClassLoaderContext::OpenDexFiles(const std::string& classpath_dir,
           return false;
         }
 
-        fd = fds[dex_file_index++];
-        DCHECK_GE(fd, 0);
+        file = File(fds[dex_file_index++], /*check_usage=*/false);
+        DCHECK(file.IsValid());
       }
 
       std::string error_msg;
       std::optional<uint32_t> dex_checksum;
       if (only_read_checksums) {
         bool zip_file_only_contains_uncompress_dex;
-        ArtDexFileLoader dex_file_loader(DupCloexec(fd), location);
+        ArtDexFileLoader dex_file_loader(&file, location);
         if (!dex_file_loader.GetMultiDexChecksum(
                 &dex_checksum, &error_msg, &zip_file_only_contains_uncompress_dex)) {
-          LOG(WARNING) << "Could not get dex checksums for location " << location << ", fd=" << fd;
+          LOG(WARNING) << "Could not get dex checksums for location " << location
+                       << ", fd=" << file.Fd();
           dex_files_state_ = kDexFilesOpenFailed;
         }
+        file.Release();  // Don't close the file yet (we have only read the checksum).
       } else {
         // When opening the dex files from the context we expect their checksum to match their
         // contents. So pass true to verify_checksum.
         // We don't need to do structural dex file verification, we only need to
         // check the checksum, so pass false to verify.
         size_t i = info->opened_dex_files.size();
-        ArtDexFileLoader dex_file_loader(fd, location);
+        ArtDexFileLoader dex_file_loader(&file, location);
         if (!dex_file_loader.Open(/*verify=*/false,
                                   /*verify_checksum=*/true,
                                   &error_msg,
                                   &info->opened_dex_files)) {
-          LOG(WARNING) << "Could not open dex files for location " << location << ", fd=" << fd;
+          LOG(WARNING) << "Could not open dex files for location " << location
+                       << ", fd=" << file.Fd();
           dex_files_state_ = kDexFilesOpenFailed;
         } else {
           dex_checksum = DexFileLoader::GetMultiDexChecksum(info->opened_dex_files, &i);
diff --git a/runtime/gc/space/image_space.cc b/runtime/gc/space/image_space.cc
index 124fed7ac2..98bf3223c7 100644
--- a/runtime/gc/space/image_space.cc
+++ b/runtime/gc/space/image_space.cc
@@ -3474,8 +3474,11 @@ bool ImageSpace::ValidateOatFile(const OatFile& oat_file,
 
     // Original checksum.
     std::optional<uint32_t> dex_checksum;
-    ArtDexFileLoader dex_loader(DupCloexec(dex_fd), dex_file_location);
-    if (!dex_loader.GetMultiDexChecksum(&dex_checksum, error_msg)) {
+    File file(dex_fd, /*check_usage=*/false);
+    ArtDexFileLoader dex_loader(&file, dex_file_location);
+    bool ok = dex_loader.GetMultiDexChecksum(&dex_checksum, error_msg);
+    file.Release();  // Don't close the file yet (we have only read the checksum).
+    if (!ok) {
       *error_msg = StringPrintf(
           "ValidateOatFile failed to get checksum of dex file '%s' "
           "referenced by oat file %s: %s",
diff --git a/runtime/oat_file.cc b/runtime/oat_file.cc
index afb5e2df68..ccfcc895dc 100644
--- a/runtime/oat_file.cc
+++ b/runtime/oat_file.cc
@@ -811,18 +811,21 @@ bool OatFileBase::Setup(int zip_fd,
         bool loaded = false;
         CHECK(zip_fd == -1 || dex_fds.empty());  // Allow only the supported combinations.
         if (zip_fd != -1) {
-          ArtDexFileLoader dex_file_loader(zip_fd, dex_file_location);
+          File file(zip_fd, /*check_usage=*/false);
+          ArtDexFileLoader dex_file_loader(&file, dex_file_location);
           loaded = dex_file_loader.Open(/*verify=*/false,
                                         /*verify_checksum=*/false,
                                         error_msg,
                                         &new_dex_files);
         } else if (dex_fd != -1) {
           // Note that we assume dex_fds are backing by jars.
-          ArtDexFileLoader dex_file_loader(DupCloexec(dex_fd), dex_file_location);
+          File file(dex_fd, /*check_usage=*/false);
+          ArtDexFileLoader dex_file_loader(&file, dex_file_location);
           loaded = dex_file_loader.Open(/*verify=*/false,
                                         /*verify_checksum=*/false,
                                         error_msg,
                                         &new_dex_files);
+          file.Release();  // Don't close the file.
         } else {
           ArtDexFileLoader dex_file_loader(dex_file_name.c_str(), dex_file_location);
           loaded = dex_file_loader.Open(/*verify=*/false,
@@ -837,9 +840,8 @@ bool OatFileBase::Setup(int zip_fd,
             LOG(WARNING) << "Could not find associated dex files of oat file. "
                          << "Oatdump will only dump the header.";
             return true;
-          } else {
-            return false;
           }
+          return false;
         }
         // The oat file may be out of date wrt/ the dex-file location. We need to be defensive
         // here and ensure that at least the number of dex files still matches.
@@ -1828,7 +1830,8 @@ class OatFileBackedByVdex final : public OatFileBase {
       // a vdex file.
       bool loaded = false;
       if (zip_fd != -1) {
-        ArtDexFileLoader dex_file_loader(zip_fd, dex_location);
+        File file(zip_fd, /*check_usage=*/false);
+        ArtDexFileLoader dex_file_loader(&file, dex_location);
         loaded = dex_file_loader.Open(/*verify=*/false,
                                       /*verify_checksum=*/false,
                                       error_msg,
diff --git a/runtime/oat_file_assistant.cc b/runtime/oat_file_assistant.cc
index 5d035f554f..9848a78cbd 100644
--- a/runtime/oat_file_assistant.cc
+++ b/runtime/oat_file_assistant.cc
@@ -720,7 +720,8 @@ bool OatFileAssistant::GetRequiredDexChecksum(std::optional<uint32_t>* checksum,
   if (!required_dex_checksums_attempted_) {
     required_dex_checksums_attempted_ = true;
 
-    ArtDexFileLoader dex_loader(DupCloexec(zip_fd_), dex_location_);
+    File file(zip_fd_, /*check_usage=*/false);
+    ArtDexFileLoader dex_loader(&file, dex_location_);
     std::optional<uint32_t> checksum2;
     std::string error2;
     if (dex_loader.GetMultiDexChecksum(
@@ -731,6 +732,7 @@ bool OatFileAssistant::GetRequiredDexChecksum(std::optional<uint32_t>* checksum,
       cached_required_dex_checksums_ = std::nullopt;
       cached_required_dex_checksums_error_ = error2;
     }
+    file.Release();  // Don't close the file yet (we have only read the checksum).
   }
 
   if (cached_required_dex_checksums_error_.has_value()) {
diff --git a/runtime/oat_file_assistant_context.cc b/runtime/oat_file_assistant_context.cc
index 0539e1c0a5..e0e8a3d808 100644
--- a/runtime/oat_file_assistant_context.cc
+++ b/runtime/oat_file_assistant_context.cc
@@ -159,10 +159,12 @@ const std::vector<std::string>* OatFileAssistantContext::GetBcpChecksums(size_t
   }
 
   const std::vector<int>* fds = runtime_options_->boot_class_path_fds;
-  ArtDexFileLoader dex_loader(fds != nullptr ? DupCloexec((*fds)[bcp_index]) : -1,
-                              runtime_options_->boot_class_path[bcp_index]);
+  File file(fds != nullptr ? (*fds)[bcp_index] : -1, /*check_usage=*/false);
+  ArtDexFileLoader dex_loader(&file, runtime_options_->boot_class_path[bcp_index]);
   std::optional<uint32_t> checksum;
-  if (!dex_loader.GetMultiDexChecksum(&checksum, error_msg)) {
+  bool ok = dex_loader.GetMultiDexChecksum(&checksum, error_msg);
+  file.Release();  // Don't close the file yet (we have only read the checksum).
+  if (!ok) {
     return nullptr;
   }
 
diff --git a/runtime/runtime.cc b/runtime/runtime.cc
index d266c2f6d7..e04d6ec387 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -1283,17 +1283,17 @@ static size_t OpenBootDexFiles(ArrayRef<const std::string> dex_filenames,
   for (size_t i = 0; i < dex_filenames.size(); i++) {
     const char* dex_filename = dex_filenames[i].c_str();
     const char* dex_location = dex_locations[i].c_str();
-    const int dex_fd = i < dex_fds.size() ? dex_fds[i] : -1;
+    File file = File(i < dex_fds.size() ? dex_fds[i] : -1, /*check_usage=*/false);
     static constexpr bool kVerifyChecksum = true;
     std::string error_msg;
-    if (!OS::FileExists(dex_filename) && dex_fd < 0) {
+    if (!OS::FileExists(dex_filename) && file.IsValid()) {
       LOG(WARNING) << "Skipping non-existent dex file '" << dex_filename << "'";
       continue;
     }
     bool verify = Runtime::Current()->IsVerificationEnabled();
-    ArtDexFileLoader dex_file_loader(dex_filename, dex_fd, dex_location);
+    ArtDexFileLoader dex_file_loader(dex_filename, &file, dex_location);
     if (!dex_file_loader.Open(verify, kVerifyChecksum, &error_msg, dex_files)) {
-      LOG(WARNING) << "Failed to open .dex from file '" << dex_filename << "' / fd " << dex_fd
+      LOG(WARNING) << "Failed to open .dex from file '" << dex_filename << "' / fd " << file.Fd()
                    << ": " << error_msg;
       ++failure_count;
     }
diff --git a/tools/hiddenapi/hiddenapi_test.cc b/tools/hiddenapi/hiddenapi_test.cc
index 7eb7480b54..fe7677efff 100644
--- a/tools/hiddenapi/hiddenapi_test.cc
+++ b/tools/hiddenapi/hiddenapi_test.cc
@@ -114,7 +114,7 @@ class HiddenApiTest : public CommonRuntimeTest {
       UNREACHABLE();
     }
 
-    ArtDexFileLoader dex_loader(fd.Release(), file.GetFilename());
+    ArtDexFileLoader dex_loader(&fd, file.GetFilename());
     std::unique_ptr<const DexFile> dex_file(dex_loader.Open(
         /*location_checksum=*/0,
         /*verify=*/true,