From 279a22f96e639e76c801bdb39aee5576f2280fe0 Mon Sep 17 00:00:00 2001
From: Dmitriy Ivanov <dimitry@google.com>
Date: Fri, 23 Jan 2015 12:03:53 -0800
Subject: [PATCH] Minimize calls to mprotect
Implement refcounter based data protection guard
to avoid unnecessary calls to mprotect when dlopen/dlclose
is called from a constructor.
Bug: 19124318
Big: 7941716
Change-Id: Id221b84ce75443094f99756dc9950b0a1dc87222
---
linker/linker.cpp | 66 ++++++++++++-------
tests/dlfcn_test.cpp | 14 ++++
tests/libs/Android.mk | 26 ++++++++
.../libs/dlopen_testlib_dlopen_from_ctor.cpp | 23 +++++++
4 files changed, 106 insertions(+), 23 deletions(-)
create mode 100644 tests/libs/dlopen_testlib_dlopen_from_ctor.cpp
diff --git a/linker/linker.cpp b/linker/linker.cpp
index df8e52e7d..f7bcd2750 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 6fdfdc75b..3b1001a8e 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 50d96b2b8..7ca856cdd 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 000000000..95233f75c
--- /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);
+}
+