Fix dlclose for libraries with thread_local dtors
Introduce new flag to mark soinfo as TLS_NODELETE when
there are thread_local dtors associated with dso_handle
belonging to it.
Test: bionic-unit-tests --gtest_filter=dl*
Test: bionic-unit-tests-glibc --gtest_filter=dl*
Bug: https://github.com/android-ndk/ndk/issues/360
Change-Id: I724ef89fc899788f95c47e6372c38b3313f18fed
diff --git a/libc/Android.bp b/libc/Android.bp
index 20648d0..a0748a9 100644
--- a/libc/Android.bp
+++ b/libc/Android.bp
@@ -1706,15 +1706,18 @@
// Do not pack libc.so relocations; see http://b/20645321 for details.
pack_relocations: false,
- // WARNING: The only library libc.so should depend on is libdl.so! If you add other libraries,
- // make sure to add -Wl,--exclude-libs=libgcc.a to the LOCAL_LDFLAGS for those libraries. This
- // ensures that symbols that are pulled into those new libraries from libgcc.a are not declared
- // external; if that were the case, then libc would not pull those symbols from libgcc.a as it
- // should, instead relying on the external symbols from the dependent libraries. That would
- // create a "cloaked" dependency on libgcc.a in libc though the libraries, which is not what
- // you wanted!
+ // WARNING: The only libraries libc.so should depend on are libdl.so and ld-android.so!
+ // If you add other libraries, make sure to add -Wl,--exclude-libs=libgcc.a to the
+ // LOCAL_LDFLAGS for those libraries. This ensures that symbols that are pulled into
+ // those new libraries from libgcc.a are not declared external; if that were the case,
+ // then libc would not pull those symbols from libgcc.a as it should, instead relying
+ // on the external symbols from the dependent libraries. That would create a "cloaked"
+ // dependency on libgcc.a in libc though the libraries, which is not what you wanted!
- shared_libs: ["libdl"],
+ shared_libs: [
+ "ld-android",
+ "libdl",
+ ],
whole_static_libs: ["libc_common", "libjemalloc"],
nocrt: true,
diff --git a/libc/bionic/__cxa_thread_atexit_impl.cpp b/libc/bionic/__cxa_thread_atexit_impl.cpp
index f687fd1..99077c1 100644
--- a/libc/bionic/__cxa_thread_atexit_impl.cpp
+++ b/libc/bionic/__cxa_thread_atexit_impl.cpp
@@ -28,6 +28,8 @@
};
extern "C" int __cxa_thread_atexit_impl(void (*func) (void *), void *arg, void *dso_handle);
+extern "C" void __loader_add_thread_local_dtor(void* dso_handle) __attribute__((weak));
+extern "C" void __loader_remove_thread_local_dtor(void* dso_handle) __attribute__((weak));
__BIONIC_WEAK_FOR_NATIVE_BRIDGE
int __cxa_thread_atexit_impl(void (*func) (void *), void *arg, void *dso_handle) {
@@ -40,6 +42,9 @@
pthread_internal_t* thread = __get_thread();
dtor->next = thread->thread_local_dtors;
thread->thread_local_dtors = dtor;
+ if (__loader_add_thread_local_dtor != nullptr) {
+ __loader_add_thread_local_dtor(dso_handle);
+ }
return 0;
}
@@ -50,6 +55,9 @@
thread->thread_local_dtors = current->next;
current->func(current->arg);
+ if (__loader_remove_thread_local_dtor != nullptr) {
+ __loader_remove_thread_local_dtor(current->dso_handle);
+ }
delete current;
}
}
diff --git a/linker/dlfcn.cpp b/linker/dlfcn.cpp
index 869e0c1..7f40f90 100644
--- a/linker/dlfcn.cpp
+++ b/linker/dlfcn.cpp
@@ -82,6 +82,8 @@
const char* symbol,
const char* version,
const void* caller_addr) __LINKER_PUBLIC__;
+void __loader_add_thread_local_dtor(void* dso_handle) __LINKER_PUBLIC__;
+void __loader_remove_thread_local_dtor(void* dso_handle) __LINKER_PUBLIC__;
#if defined(__arm__)
_Unwind_Ptr __loader_dl_unwind_find_exidx(_Unwind_Ptr pc, int* pcount) __LINKER_PUBLIC__;
#endif
@@ -272,6 +274,16 @@
CFIShadowWriter::CfiFail(CallSiteTypeId, Ptr, DiagData, CallerPc);
}
+void __loader_add_thread_local_dtor(void* dso_handle) {
+ ScopedPthreadMutexLocker locker(&g_dl_mutex);
+ increment_dso_handle_reference_counter(dso_handle);
+}
+
+void __loader_remove_thread_local_dtor(void* dso_handle) {
+ ScopedPthreadMutexLocker locker(&g_dl_mutex);
+ decrement_dso_handle_reference_counter(dso_handle);
+}
+
static uint8_t __libdl_info_buf[sizeof(soinfo)] __attribute__((aligned(8)));
static soinfo* __libdl_info = nullptr;
diff --git a/linker/ld_android.cpp b/linker/ld_android.cpp
index 8e8608b..c4ce1b9 100644
--- a/linker/ld_android.cpp
+++ b/linker/ld_android.cpp
@@ -59,6 +59,8 @@
__strong_alias(__loader_dlopen, __internal_linker_error);
__strong_alias(__loader_dlsym, __internal_linker_error);
__strong_alias(__loader_dlvsym, __internal_linker_error);
+__strong_alias(__loader_add_thread_local_dtor, __internal_linker_error);
+__strong_alias(__loader_remove_thread_local_dtor, __internal_linker_error);
#if defined(__arm__)
__strong_alias(__loader_dl_unwind_find_exidx, __internal_linker_error);
#endif
diff --git a/linker/linker.arm.map b/linker/linker.arm.map
index 67b0632..4a3f177 100644
--- a/linker/linker.arm.map
+++ b/linker/linker.arm.map
@@ -19,6 +19,8 @@
__loader_android_link_namespaces;
__loader_android_get_exported_namespace;
__loader_dl_unwind_find_exidx;
+ __loader_add_thread_local_dtor;
+ __loader_remove_thread_local_dtor;
rtld_db_dlactivity;
local:
*;
diff --git a/linker/linker.cpp b/linker/linker.cpp
index 4ad44fa..317fdff 100644
--- a/linker/linker.cpp
+++ b/linker/linker.cpp
@@ -75,6 +75,8 @@
#undef ELF_ST_TYPE
#define ELF_ST_TYPE(x) (static_cast<uint32_t>(x) & 0xf)
+static std::unordered_map<void*, size_t> g_dso_handle_counters;
+
static android_namespace_t* g_anonymous_namespace = &g_default_namespace;
static std::unordered_map<std::string, android_namespace_t*> g_exported_namespaces;
@@ -1780,26 +1782,10 @@
return si;
}
-static void soinfo_unload(soinfo* unload_si) {
- // Note that the library can be loaded but not linked;
- // in which case there is no root but we still need
- // to walk the tree and unload soinfos involved.
- //
- // This happens on unsuccessful dlopen, when one of
- // the DT_NEEDED libraries could not be linked/found.
- bool is_linked = unload_si->is_linked();
- soinfo* root = is_linked ? unload_si->get_local_group_root() : unload_si;
-
- LD_LOG(kLogDlopen,
- "... dlclose(realpath=\"%s\"@%p) ... load group root is \"%s\"@%p",
- unload_si->get_realpath(),
- unload_si,
- root->get_realpath(),
- root);
-
+static void soinfo_unload_impl(soinfo* root) {
ScopedTrace trace((std::string("unload ") + root->get_realpath()).c_str());
+ bool is_linked = root->is_linked();
- size_t ref_count = is_linked ? root->decrement_ref_count() : 0;
if (!root->can_unload()) {
LD_LOG(kLogDlopen,
"... dlclose(root=\"%s\"@%p) ... not unloading - the load group is flagged with NODELETE",
@@ -1808,14 +1794,6 @@
return;
}
- if (ref_count > 0) {
- LD_LOG(kLogDlopen,
- "... dlclose(root=\"%s\"@%p) ... not unloading - decrementing ref_count to %zd",
- root->get_realpath(),
- root,
- ref_count);
- return;
- }
soinfo_list_t unload_list;
unload_list.push_back(root);
@@ -1916,6 +1894,86 @@
}
}
+static void soinfo_unload(soinfo* unload_si) {
+ // Note that the library can be loaded but not linked;
+ // in which case there is no root but we still need
+ // to walk the tree and unload soinfos involved.
+ //
+ // This happens on unsuccessful dlopen, when one of
+ // the DT_NEEDED libraries could not be linked/found.
+ bool is_linked = unload_si->is_linked();
+ soinfo* root = is_linked ? unload_si->get_local_group_root() : unload_si;
+
+ LD_LOG(kLogDlopen,
+ "... dlclose(realpath=\"%s\"@%p) ... load group root is \"%s\"@%p",
+ unload_si->get_realpath(),
+ unload_si,
+ root->get_realpath(),
+ root);
+
+
+ size_t ref_count = is_linked ? root->decrement_ref_count() : 0;
+ if (ref_count > 0) {
+ LD_LOG(kLogDlopen,
+ "... dlclose(root=\"%s\"@%p) ... not unloading - decrementing ref_count to %zd",
+ root->get_realpath(),
+ root,
+ ref_count);
+ return;
+ }
+
+ soinfo_unload_impl(root);
+}
+
+void increment_dso_handle_reference_counter(void* dso_handle) {
+ if (dso_handle == nullptr) {
+ return;
+ }
+
+ auto it = g_dso_handle_counters.find(dso_handle);
+ if (it != g_dso_handle_counters.end()) {
+ CHECK(++it->second != 0);
+ } else {
+ soinfo* si = find_containing_library(dso_handle);
+ if (si != nullptr) {
+ ProtectedDataGuard guard;
+ si->set_tls_nodelete();
+ } else {
+ async_safe_fatal(
+ "increment_dso_handle_reference_counter: Couldn't find soinfo by dso_handle=%p",
+ dso_handle);
+ }
+ g_dso_handle_counters[dso_handle] = 1U;
+ }
+}
+
+void decrement_dso_handle_reference_counter(void* dso_handle) {
+ if (dso_handle == nullptr) {
+ return;
+ }
+
+ auto it = g_dso_handle_counters.find(dso_handle);
+ CHECK(it != g_dso_handle_counters.end());
+ CHECK(it->second != 0);
+
+ if (--it->second == 0) {
+ soinfo* si = find_containing_library(dso_handle);
+ if (si != nullptr) {
+ ProtectedDataGuard guard;
+ si->unset_tls_nodelete();
+ if (si->get_ref_count() == 0) {
+ // Perform deferred unload - note that soinfo_unload_impl does not decrement ref_count
+ soinfo_unload_impl(si);
+ }
+ } else {
+ async_safe_fatal(
+ "decrement_dso_handle_reference_counter: Couldn't find soinfo by dso_handle=%p",
+ dso_handle);
+ }
+ g_dso_handle_counters.erase(it);
+ }
+}
+
static std::string symbol_display_name(const char* sym_name, const char* sym_ver) {
if (sym_ver == nullptr) {
return sym_name;
diff --git a/linker/linker.generic.map b/linker/linker.generic.map
index abd792a..04f4c8a 100644
--- a/linker/linker.generic.map
+++ b/linker/linker.generic.map
@@ -18,6 +18,8 @@
__loader_cfi_fail;
__loader_android_link_namespaces;
__loader_android_get_exported_namespace;
+ __loader_add_thread_local_dtor;
+ __loader_remove_thread_local_dtor;
rtld_db_dlactivity;
local:
*;
diff --git a/linker/linker.h b/linker/linker.h
index f5a202f..6ebca4c 100644
--- a/linker/linker.h
+++ b/linker/linker.h
@@ -182,4 +182,7 @@
android_namespace_t* get_exported_namespace(const char* name);
+void increment_dso_handle_reference_counter(void* dso_handle);
+void decrement_dso_handle_reference_counter(void* dso_handle);
+
#endif
diff --git a/linker/linker_soinfo.cpp b/linker/linker_soinfo.cpp
index dd91752..54bfcf0 100644
--- a/linker/linker_soinfo.cpp
+++ b/linker/linker_soinfo.cpp
@@ -537,6 +537,14 @@
rtld_flags_ |= RTLD_NODELETE;
}
+void soinfo::set_tls_nodelete() {
+ flags_ |= FLAG_TLS_NODELETE;
+}
+
+void soinfo::unset_tls_nodelete() {
+ flags_ &= ~FLAG_TLS_NODELETE;
+}
+
const char* soinfo::get_realpath() const {
#if defined(__work_around_b_24465209__)
if (has_min_version(2)) {
@@ -650,7 +658,11 @@
}
bool soinfo::can_unload() const {
- return !is_linked() || ((get_rtld_flags() & (RTLD_NODELETE | RTLD_GLOBAL)) == 0);
+ return !is_linked() ||
+ (
+ (get_rtld_flags() & (RTLD_NODELETE | RTLD_GLOBAL)) == 0 &&
+ (flags_ & FLAG_TLS_NODELETE) == 0
+ );
}
bool soinfo::is_linked() const {
@@ -693,6 +705,10 @@
return --local_group_root_->ref_count_;
}
+size_t soinfo::get_ref_count() const {
+ return local_group_root_->ref_count_;
+}
+
soinfo* soinfo::get_local_group_root() const {
return local_group_root_;
}
diff --git a/linker/linker_soinfo.h b/linker/linker_soinfo.h
index 273c36c..91118b0 100644
--- a/linker/linker_soinfo.h
+++ b/linker/linker_soinfo.h
@@ -52,6 +52,14 @@
// when load group is crossing
// namespace boundary twice and second
// local group depends on the same libraries.
+#define FLAG_TLS_NODELETE 0x00000200 // This flag set when there is at least one
+ // outstanding thread_local dtor
+ // registered with this soinfo. In such
+ // a case the actual unload is
+ // postponed until the last thread_local
+ // destructor associated with this
+ // soinfo is executed and this flag is
+ // unset.
#define FLAG_NEW_SOINFO 0x40000000 // new soinfo format
#define SOINFO_VERSION 3
@@ -253,9 +261,12 @@
void set_linker_flag();
void set_main_executable();
void set_nodelete();
+ void set_tls_nodelete();
+ void unset_tls_nodelete();
size_t increment_ref_count();
size_t decrement_ref_count();
+ size_t get_ref_count() const;
soinfo* get_local_group_root() const;
diff --git a/tests/dlfcn_test.cpp b/tests/dlfcn_test.cpp
index 6bd6e0c..000d1f7 100644
--- a/tests/dlfcn_test.cpp
+++ b/tests/dlfcn_test.cpp
@@ -1317,6 +1317,84 @@
dlclose(handle);
}
+TEST(dlfcn, dlclose_after_thread_local_dtor) {
+ bool is_dtor_triggered = false;
+
+ auto f = [](void* handle, bool* is_dtor_triggered) {
+ typedef void (*fn_t)(bool*);
+ fn_t fn = reinterpret_cast<fn_t>(dlsym(handle, "init_thread_local_variable"));
+ ASSERT_TRUE(fn != nullptr) << dlerror();
+
+ fn(is_dtor_triggered);
+
+ ASSERT_TRUE(!*is_dtor_triggered);
+ };
+
+ void* handle = dlopen("libtest_thread_local_dtor.so", RTLD_NOW | RTLD_NOLOAD);
+ ASSERT_TRUE(handle == nullptr);
+
+ handle = dlopen("libtest_thread_local_dtor.so", RTLD_NOW);
+ ASSERT_TRUE(handle != nullptr) << dlerror();
+
+ std::thread t(f, handle, &is_dtor_triggered);
+ t.join();
+
+ ASSERT_TRUE(is_dtor_triggered);
+ dlclose(handle);
+
+ handle = dlopen("libtest_thread_local_dtor.so", RTLD_NOW | RTLD_NOLOAD);
+ ASSERT_TRUE(handle == nullptr);
+}
+
+TEST(dlfcn, dlclose_before_thread_local_dtor) {
+ bool is_dtor_triggered = false;
+
+ auto f = [](bool* is_dtor_triggered) {
+ void* handle = dlopen("libtest_thread_local_dtor.so", RTLD_NOW | RTLD_NOLOAD);
+ ASSERT_TRUE(handle == nullptr);
+
+ handle = dlopen("libtest_thread_local_dtor.so", RTLD_NOW);
+ ASSERT_TRUE(handle != nullptr) << dlerror();
+
+ typedef void (*fn_t)(bool*);
+ fn_t fn = reinterpret_cast<fn_t>(dlsym(handle, "init_thread_local_variable"));
+ ASSERT_TRUE(fn != nullptr) << dlerror();
+
+ fn(is_dtor_triggered);
+
+ dlclose(handle);
+
+ ASSERT_TRUE(!*is_dtor_triggered);
+
+ // Since we have thread_atexit dtors associated with handle - the library should
+ // still be availabe.
+ handle = dlopen("libtest_thread_local_dtor.so", RTLD_NOW | RTLD_NOLOAD);
+ ASSERT_TRUE(handle != nullptr) << dlerror();
+ dlclose(handle);
+ };
+
+ void* handle = dlopen("libtest_thread_local_dtor.so", RTLD_NOW);
+ ASSERT_TRUE(handle != nullptr) << dlerror();
+ dlclose(handle);
+
+ handle = dlopen("libtest_thread_local_dtor.so", RTLD_NOW | RTLD_NOLOAD);
+ ASSERT_TRUE(handle == nullptr);
+
+ std::thread t(f, &is_dtor_triggered);
+ t.join();
+#if defined(__BIONIC__)
+ // ld-android.so unloads unreferenced libraries on pthread_exit()
+ ASSERT_TRUE(is_dtor_triggered);
+ handle = dlopen("libtest_thread_local_dtor.so", RTLD_NOW | RTLD_NOLOAD);
+ ASSERT_TRUE(handle == nullptr);
+#else
+ // GLIBC does not unload libraries with ref_count = 0 on pthread_exit
+ ASSERT_TRUE(is_dtor_triggered);
+ handle = dlopen("libtest_thread_local_dtor.so", RTLD_NOW | RTLD_NOLOAD);
+ ASSERT_TRUE(handle != nullptr) << dlerror();
+#endif
+}
+
TEST(dlfcn, RTLD_macros) {
#if !defined(RTLD_LOCAL)
#error no RTLD_LOCAL
diff --git a/tests/libs/Android.bp b/tests/libs/Android.bp
index e45eb6e..61a837f 100644
--- a/tests/libs/Android.bp
+++ b/tests/libs/Android.bp
@@ -636,6 +636,16 @@
}
// -----------------------------------------------------------------------------
+// Library with non-trivial thread_local variable to test dlclose()
+// -----------------------------------------------------------------------------
+cc_test_library {
+ name: "libtest_thread_local_dtor",
+ defaults: ["bionic_testlib_defaults"],
+ srcs: ["thread_local_dtor.cpp"],
+}
+
+
+// -----------------------------------------------------------------------------
// Tool to use to align the shared libraries in a zip file.
// -----------------------------------------------------------------------------
cc_binary_host {
diff --git a/tests/libs/thread_local_dtor.cpp b/tests/libs/thread_local_dtor.cpp
new file mode 100644
index 0000000..cefff7a
--- /dev/null
+++ b/tests/libs/thread_local_dtor.cpp
@@ -0,0 +1,45 @@
+/*
+ * Copyright (C) 2018 The Android Open Source Project
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in
+ * the documentation and/or other materials provided with the
+ * distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+ * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
+ * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
+ * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS
+ * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
+ * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+namespace {
+
+class TestClass {
+ public:
+ TestClass(bool* flag) : flag_(flag) {}
+ ~TestClass() {
+ *flag_ = true;
+ }
+ private:
+ bool* flag_;
+};
+
+}; // namespace
+
+extern "C" void init_thread_local_variable(bool* flag) {
+ thread_local TestClass test(flag);
+}