commit c7d3f4b8c294f36209ea2c129f9714a36ae1ec6b
author Mathieu Chartier <mathieuc@google.com> Wed Jun 01 10:48:19 2016 -0700
committer Mathieu Chartier <mathieuc@google.com> Wed Jun 01 11:02:07 2016 -0700
tree 3ac0e2896bde73f089c77fbc07e935a804358d36
parent a21decf759af7dc29f64ff7ee02a758397578cd3

Fix race with host_dlopen_handles_

Thread 1 opens an already opened oat file.
Thread 2 dlcloses the oat file and removes it from
host_dlopen_handles_.
Thread 1 checks that it is not already in host_dlopen_handles_ and
proceeds to return the oat file. The problem now is that the BSS is
not cleared since it is the same oat file that was opened earlier.

The fix is to just hold the lock for dlopen / dlclose. This only
affects contention on host.

Bug: 28992179
Bug: 28990799
Bug: 28826195

Change-Id: I126e3d4da69f493b21604205eeea75352a6cf736

runtime/class_linker.cc

diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index 9ac27a1..c9c3136 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -2092,6 +2092,21 @@
       reinterpret_cast<ArtMethod**>(raw_arrays + layout.MethodsOffset());
   ArtField** fields = (dex_file.NumFieldIds() == 0u) ? nullptr :
       reinterpret_cast<ArtField**>(raw_arrays + layout.FieldsOffset());
+  if (kIsDebugBuild) {
+    // Sanity check to make sure all the dex cache arrays are empty. b/28992179
+    for (size_t i = 0; i < dex_file.NumStringIds(); ++i) {
+      CHECK(strings[i].Read<kWithoutReadBarrier>() == nullptr);
+    }
+    for (size_t i = 0; i < dex_file.NumTypeIds(); ++i) {
+      CHECK(types[i].Read<kWithoutReadBarrier>() == nullptr);
+    }
+    for (size_t i = 0; i < dex_file.NumMethodIds(); ++i) {
+      CHECK(mirror::DexCache::GetElementPtrSize(methods, i, image_pointer_size_) == nullptr);
+    }
+    for (size_t i = 0; i < dex_file.NumFieldIds(); ++i) {
+      CHECK(mirror::DexCache::GetElementPtrSize(fields, i, image_pointer_size_) == nullptr);
+    }
+  }
   dex_cache->Init(&dex_file,
                   location.Get(),
                   strings,

runtime/oat_file.cc

diff --git a/runtime/oat_file.cc b/runtime/oat_file.cc
index 995ea99..62c723e 100644
--- a/runtime/oat_file.cc
+++ b/runtime/oat_file.cc
@@ -500,11 +500,12 @@
 
   ~DlOpenOatFile() {
     if (dlopen_handle_ != nullptr) {
-      dlclose(dlopen_handle_);
-
       if (!kIsTargetBuild) {
         MutexLock mu(Thread::Current(), *Locks::host_dlopen_handles_lock_);
         host_dlopen_handles_.erase(dlopen_handle_);
+        dlclose(dlopen_handle_);
+      } else {
+        dlclose(dlopen_handle_);
       }
     }
   }
@@ -660,9 +661,9 @@
 #else
     UNUSED(oat_file_begin);
     static_assert(!kIsTargetBuild, "host_dlopen_handles_ will leak handles");
+    MutexLock mu(Thread::Current(), *Locks::host_dlopen_handles_lock_);
     dlopen_handle_ = dlopen(absolute_path.get(), RTLD_NOW);
     if (dlopen_handle_ != nullptr) {
-      MutexLock mu(Thread::Current(), *Locks::host_dlopen_handles_lock_);
       if (!host_dlopen_handles_.insert(dlopen_handle_).second) {
         dlclose(dlopen_handle_);
         dlopen_handle_ = nullptr;