commit 14d7b3e37b6129d845d6c8da8ee8d612937c63d4
author Mathieu Chartier <mathieuc@google.com> Thu Jun 09 16:18:04 2016 -0700
committer Mathieu Chartier <mathieuc@google.com> Fri Jun 10 09:41:36 2016 -0700
tree 78fbff24020bbb21a57fa647523bd32970669c2c
parent 531e86c88872f7d28fb751a57efccdeed7ec960a

Fix dex file leak in oat file manager

Simplified ownership by having a vector of unique pointers own
all newly opened dex files.

Bug: 29246280

(cherry picked from commit fed316715faeec7bf34e3c4a878288c1342cb0e8)

Change-Id: I97db09ced76db8ffdbae371ff72977c4276a0494

runtime/oat_file_manager.cc

diff --git a/runtime/oat_file_manager.cc b/runtime/oat_file_manager.cc
index 3eeccd2..fbae1da 100644
--- a/runtime/oat_file_manager.cc
+++ b/runtime/oat_file_manager.cc
@@ -183,11 +183,6 @@
     return dex_file_;
   }
 
-  void DeleteDexFile() {
-    delete dex_file_;
-    dex_file_ = nullptr;
-  }
-
  private:
   static const char* GetClassDescriptor(const DexFile* dex_file, size_t index) {
     DCHECK(IsUint<16>(index));
@@ -206,36 +201,25 @@
 
 static void AddDexFilesFromOat(const OatFile* oat_file,
                                bool already_loaded,
-                               /*out*/std::priority_queue<DexFileAndClassPair>* heap) {
+                               /*out*/std::priority_queue<DexFileAndClassPair>* heap,
+                               std::vector<std::unique_ptr<const DexFile>>* opened_dex_files) {
   for (const OatDexFile* oat_dex_file : oat_file->GetOatDexFiles()) {
     std::string error;
     std::unique_ptr<const DexFile> dex_file = oat_dex_file->OpenDexFile(&error);
     if (dex_file == nullptr) {
       LOG(WARNING) << "Could not create dex file from oat file: " << error;
     } else if (dex_file->NumClassDefs() > 0U) {
-      heap->emplace(dex_file.release(), /*current_class_index*/0U, already_loaded);
+      heap->emplace(dex_file.get(), /*current_class_index*/0U, already_loaded);
+      opened_dex_files->push_back(std::move(dex_file));
     }
   }
 }
 
 static void AddNext(/*inout*/DexFileAndClassPair* original,
-                    /*inout*/std::priority_queue<DexFileAndClassPair>* heap,
-                    bool owning_dex_files) {
+                    /*inout*/std::priority_queue<DexFileAndClassPair>* heap) {
   if (original->DexFileHasMoreClasses()) {
     original->Next();
     heap->push(std::move(*original));
-  } else if (owning_dex_files) {
-    original->DeleteDexFile();
-  }
-}
-
-static void FreeDexFilesInHeap(std::priority_queue<DexFileAndClassPair>* heap,
-                               bool owning_dex_files) {
-  if (owning_dex_files) {
-    while (!heap->empty()) {
-      delete heap->top().GetDexFile();
-      heap->pop();
-    }
   }
 }
 
@@ -449,7 +433,6 @@
   DCHECK(error_msg != nullptr);
 
   std::priority_queue<DexFileAndClassPair> queue;
-  bool owning_dex_files = false;
 
   // Try to get dex files from the given class loader. If the class loader is null, or we do
   // not support one of the class loaders in the chain, conservatively compare against all
@@ -479,6 +462,9 @@
   // against the open oat files. Take the oat_file_manager_lock_ that protects oat_files_ accesses.
   ReaderMutexLock mu(Thread::Current(), *Locks::oat_file_manager_lock_);
 
+  // Vector that holds the newly opened dex files live, this is done to prevent leaks.
+  std::vector<std::unique_ptr<const DexFile>> opened_dex_files;
+
   if (!class_loader_ok) {
     // Add dex files from already loaded oat files, but skip boot.
 
@@ -487,9 +473,6 @@
       queue.pop();
     }
 
-    // Anything we load now is something we own and must be released later.
-    owning_dex_files = true;
-
     std::vector<const OatFile*> boot_oat_files = GetBootOatFiles();
     // The same OatFile can be loaded multiple times at different addresses. In this case, we don't
     // need to check both against each other since they would have resolved the same way at compile
@@ -502,7 +485,10 @@
           boot_oat_files.end() && location != oat_file->GetLocation() &&
           unique_locations.find(location) == unique_locations.end()) {
         unique_locations.insert(location);
-        AddDexFilesFromOat(loaded_oat_file.get(), /*already_loaded*/true, &queue);
+        AddDexFilesFromOat(loaded_oat_file.get(),
+                           /*already_loaded*/true,
+                           &queue,
+                           /*out*/&opened_dex_files);
       }
     }
   }
@@ -511,14 +497,13 @@
   const std::string
       shared_libraries(oat_file->GetOatHeader().GetStoreValueByKey(OatHeader::kClassPathKey));
   if (AreSharedLibrariesOk(shared_libraries, queue)) {
-    FreeDexFilesInHeap(&queue, owning_dex_files);
     return false;
   }
 
   ScopedTrace st("Collision check");
 
   // Add dex files from the oat file to check.
-  AddDexFilesFromOat(oat_file, /*already_loaded*/false, &queue);
+  AddDexFilesFromOat(oat_file, /*already_loaded*/false, &queue, &opened_dex_files);
 
   // Now drain the queue.
   while (!queue.empty()) {
@@ -538,17 +523,16 @@
                            compare_pop.GetCachedDescriptor(),
                            compare_pop.GetDexFile()->GetLocation().c_str(),
                            top.GetDexFile()->GetLocation().c_str());
-          FreeDexFilesInHeap(&queue, owning_dex_files);
           return true;
         }
         queue.pop();
-        AddNext(&top, &queue, owning_dex_files);
+        AddNext(&top, &queue);
       } else {
         // Something else. Done here.
         break;
       }
     }
-    AddNext(&compare_pop, &queue, owning_dex_files);
+    AddNext(&compare_pop, &queue);
   }
 
   return false;