Merge "Minimize calls to mprotect"

4 files changed, 106 insertions, 23 deletions

diff --git a/linker/linker.cpp b/linker/linker.cpp
index df8e52e..f7bcd27 100644
--- a/linker/linker.cpp
+++ b/linker/linker.cpp
@@ -262,11 +262,6 @@ void SoinfoListAllocator::free(LinkedListEntry<soinfo>* entry) {
   g_soinfo_links_allocator.free(entry);
 }
 
-static void protect_data(int protection) {
-  g_soinfo_allocator.protect_all(protection);
-  g_soinfo_links_allocator.protect_all(protection);
-}
-
 static soinfo* soinfo_alloc(const char* name, struct stat* file_stat, off64_t file_offset, uint32_t rtld_flags) {
   if (strlen(name) >= SOINFO_NAME_LEN) {
     DL_ERR("library name \"%s\" too long", name);
@@ -589,6 +584,34 @@ ElfW(Sym)* soinfo_do_lookup(soinfo* si_from, const char* name, soinfo** si_found
   return s;
 }
 
+class ProtectedDataGuard {
+ public:
+  ProtectedDataGuard() {
+    if (ref_count_++ == 0) {
+      protect_data(PROT_READ | PROT_WRITE);
+    }
+  }
+
+  ~ProtectedDataGuard() {
+    if (ref_count_ == 0) { // overflow
+      __libc_fatal("Too many nested calls to dlopen()");
+    }
+
+    if (--ref_count_ == 0) {
+      protect_data(PROT_READ);
+    }
+  }
+ private:
+  void protect_data(int protection) {
+    g_soinfo_allocator.protect_all(protection);
+    g_soinfo_links_allocator.protect_all(protection);
+  }
+
+  static size_t ref_count_;
+};
+
+size_t ProtectedDataGuard::ref_count_ = 0;
+
 // Each size has it's own allocator.
 template<size_t size>
 class SizeBasedAllocator {
@@ -1237,19 +1260,18 @@ soinfo* do_dlopen(const char* name, int flags, const android_dlextinfo* extinfo)
       return nullptr;
     }
   }
-  protect_data(PROT_READ | PROT_WRITE);
+
+  ProtectedDataGuard guard;
   soinfo* si = find_library(name, flags, extinfo);
   if (si != nullptr) {
     si->call_constructors();
   }
-  protect_data(PROT_READ);
   return si;
 }
 
 void do_dlclose(soinfo* si) {
-  protect_data(PROT_READ | PROT_WRITE);
+  ProtectedDataGuard guard;
   soinfo_unload(si);
-  protect_data(PROT_READ);
 }
 
 static ElfW(Addr) call_ifunc_resolver(ElfW(Addr) resolver_addr) {
@@ -1595,10 +1617,6 @@ void soinfo::call_function(const char* function_name __unused, linker_function_t
   TRACE("[ Calling %s @ %p for '%s' ]", function_name, function, name);
   function();
   TRACE("[ Done calling %s @ %p for '%s' ]", function_name, function, name);
-
-  // The function may have called dlopen(3) or dlclose(3), so we need to ensure our data structures
-  // are still writable. This happens with our debug malloc (see http://b/7941716).
-  protect_data(PROT_READ | PROT_WRITE);
 }
 
 void soinfo::call_pre_init_constructors() {
@@ -2522,15 +2540,19 @@ static ElfW(Addr) __linker_init_post_relocation(KernelArgumentBlock& args, ElfW(
 
   add_vdso(args);
 
-  si->call_pre_init_constructors();
+  {
+    ProtectedDataGuard guard;
 
-  /* After the prelink_image, the si->load_bias is initialized.
-   * For so lib, the map->l_addr will be updated in notify_gdb_of_load.
-   * We need to update this value for so exe here. So Unwind_Backtrace
-   * for some arch like x86 could work correctly within so exe.
-   */
-  map->l_addr = si->load_bias;
-  si->call_constructors();
+    si->call_pre_init_constructors();
+
+    /* After the prelink_image, the si->load_bias is initialized.
+     * For so lib, the map->l_addr will be updated in notify_gdb_of_load.
+     * We need to update this value for so exe here. So Unwind_Backtrace
+     * for some arch like x86 could work correctly within so exe.
+     */
+    map->l_addr = si->load_bias;
+    si->call_constructors();
+  }
 
 #if TIMING
   gettimeofday(&t1, nullptr);
@@ -2673,8 +2695,6 @@ extern "C" ElfW(Addr) __linker_init(void* raw_args) {
   args.abort_message_ptr = &g_abort_message;
   ElfW(Addr) start_address = __linker_init_post_relocation(args, linker_addr);
 
-  protect_data(PROT_READ);
-
   INFO("[ jumping to _start ]");
 
   // Return the address that the calling assembly stub should jump to.
diff --git a/tests/dlfcn_test.cpp b/tests/dlfcn_test.cpp
index 6fdfdc7..3b1001a 100644
--- a/tests/dlfcn_test.cpp
+++ b/tests/dlfcn_test.cpp
@@ -850,3 +850,17 @@ TEST(dlfcn, dlopen_symlink) {
   dlclose(handle1);
   dlclose(handle2);
 }
+
+// libtest_dlopen_from_ctor_main.so depends on
+// libtest_dlopen_from_ctor.so which has a constructor
+// that calls dlopen(libc...). This is to test the situation
+// described in b/7941716.
+TEST(dlfcn, dlopen_dlopen_from_ctor) {
+#if defined(__BIONIC__)
+  void* handle = dlopen("libtest_dlopen_from_ctor_main.so", RTLD_NOW);
+  ASSERT_TRUE(handle != nullptr) << dlerror();
+  dlclose(handle);
+#else
+  GTEST_LOG_(INFO) << "This test is disabled for glibc (glibc segfaults if you try to call dlopen from a constructor).\n";
+#endif
+}
diff --git a/tests/libs/Android.mk b/tests/libs/Android.mk
index 50d96b2..7ca856c 100644
--- a/tests/libs/Android.mk
+++ b/tests/libs/Android.mk
@@ -367,3 +367,29 @@ libtest_dlopen_weak_undefined_func_src_files := \
 
 module := libtest_dlopen_weak_undefined_func
 include $(LOCAL_PATH)/Android.build.testlib.mk
+
+# -----------------------------------------------------------------------------
+# Library with constructor that calls dlopen() b/7941716
+# -----------------------------------------------------------------------------
+libtest_dlopen_from_ctor_src_files := \
+   dlopen_testlib_dlopen_from_ctor.cpp
+
+module := libtest_dlopen_from_ctor
+
+build_target := SHARED_LIBRARY
+build_type := host
+include $(TEST_PATH)/Android.build.mk
+
+libtest_dlopen_from_ctor_shared_libraries := libdl
+build_type := target
+include $(TEST_PATH)/Android.build.mk
+
+# -----------------------------------------------------------------------------
+# Library that depends on the library with constructor that calls dlopen() b/7941716
+# -----------------------------------------------------------------------------
+
+libtest_dlopen_from_ctor_main_src_files := empty.cpp
+libtest_dlopen_from_ctor_main_shared_libraries := libtest_dlopen_from_ctor
+
+module := libtest_dlopen_from_ctor_main
+include $(LOCAL_PATH)/Android.build.testlib.mk
diff --git a/tests/libs/dlopen_testlib_dlopen_from_ctor.cpp b/tests/libs/dlopen_testlib_dlopen_from_ctor.cpp
new file mode 100644
index 0000000..95233f7
--- /dev/null
+++ b/tests/libs/dlopen_testlib_dlopen_from_ctor.cpp
@@ -0,0 +1,23 @@
+/*
+ * Copyright (C) 2015 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <dlfcn.h>
+
+static void __attribute__((constructor)) call_dlopen_from_ctor() {
+  void* handle = dlopen("libc.so", RTLD_NOW);
+  dlclose(handle);
+}
+