From 6df122f8528f9b9fcf7dfea14ae98b0ef66274e1 Mon Sep 17 00:00:00 2001 From: Dmitriy Ivanov Date: Thu, 20 Nov 2014 20:47:02 -0800 Subject: [PATCH] Unregister pthread_atfork handlers on dlclose() Change-Id: I326fdf6bb06bed12743f08980b5c69d849c015b8 --- libc/Android.mk | 2 +- libc/arch-arm64/bionic/crtbegin.c | 1 + libc/arch-common/bionic/crtbegin.c | 1 + libc/arch-common/bionic/crtbegin_so.c | 1 + libc/arch-common/bionic/pthread_atfork.h | 29 +++++ libc/arch-mips/bionic/crtbegin.c | 1 + libc/bionic/pthread_atfork.cpp | 122 ++++++++++++++---- .../lib/libc => }/stdlib/atexit.c | 16 ++- .../lib/libc => }/stdlib/atexit.h | 0 tests/libs/Android.build.pthread_atfork.mk | 25 ++++ tests/libs/Android.mk | 6 + tests/libs/pthread_atfork.cpp | 21 +++ tests/pthread_test.cpp | 77 +++++++++-- 13 files changed, 261 insertions(+), 41 deletions(-) create mode 100644 libc/arch-common/bionic/pthread_atfork.h rename libc/{upstream-openbsd/lib/libc => }/stdlib/atexit.c (92%) rename libc/{upstream-openbsd/lib/libc => }/stdlib/atexit.h (100%) create mode 100644 tests/libs/Android.build.pthread_atfork.mk create mode 100644 tests/libs/pthread_atfork.cpp diff --git a/libc/Android.mk b/libc/Android.mk index 4a199e747..8ed969fe9 100644 --- a/libc/Android.mk +++ b/libc/Android.mk @@ -63,6 +63,7 @@ libc_common_src_files := \ stdio/sprintf.c \ stdio/stdio.c \ stdio/stdio_ext.cpp \ + stdlib/atexit.c \ stdlib/exit.c \ # Fortify implementations of libc functions. @@ -481,7 +482,6 @@ libc_upstream_openbsd_ndk_src_files := \ upstream-openbsd/lib/libc/stdio/wprintf.c \ upstream-openbsd/lib/libc/stdio/wscanf.c \ upstream-openbsd/lib/libc/stdio/wsetup.c \ - upstream-openbsd/lib/libc/stdlib/atexit.c \ upstream-openbsd/lib/libc/stdlib/atoi.c \ upstream-openbsd/lib/libc/stdlib/atol.c \ upstream-openbsd/lib/libc/stdlib/atoll.c \ diff --git a/libc/arch-arm64/bionic/crtbegin.c b/libc/arch-arm64/bionic/crtbegin.c index fec0b11af..7e2c5d766 100644 --- a/libc/arch-arm64/bionic/crtbegin.c +++ b/libc/arch-arm64/bionic/crtbegin.c @@ -67,3 +67,4 @@ __asm__ ( #include "../../arch-common/bionic/__dso_handle.h" #include "../../arch-common/bionic/atexit.h" +#include "../../arch-common/bionic/pthread_atfork.h" diff --git a/libc/arch-common/bionic/crtbegin.c b/libc/arch-common/bionic/crtbegin.c index fa9f3f32b..c46405c4f 100644 --- a/libc/arch-common/bionic/crtbegin.c +++ b/libc/arch-common/bionic/crtbegin.c @@ -59,6 +59,7 @@ void _start() { #include "__dso_handle.h" #include "atexit.h" +#include "pthread_atfork.h" #ifdef __i386__ # include "../../arch-x86/bionic/__stack_chk_fail_local.h" #endif diff --git a/libc/arch-common/bionic/crtbegin_so.c b/libc/arch-common/bionic/crtbegin_so.c index 641e45a4e..3754363ab 100644 --- a/libc/arch-common/bionic/crtbegin_so.c +++ b/libc/arch-common/bionic/crtbegin_so.c @@ -56,6 +56,7 @@ void __on_dlclose() { # include "__dso_handle_so.h" # include "atexit.h" #endif +#include "pthread_atfork.h" #ifdef __i386__ # include "../../arch-x86/bionic/__stack_chk_fail_local.h" #endif diff --git a/libc/arch-common/bionic/pthread_atfork.h b/libc/arch-common/bionic/pthread_atfork.h new file mode 100644 index 000000000..0c48a1269 --- /dev/null +++ b/libc/arch-common/bionic/pthread_atfork.h @@ -0,0 +1,29 @@ +/* + * 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. + */ + +extern void* __dso_handle; + +extern int __register_atfork(void (*prepare)(void), void (*parent)(void), void (*child)(void), void* dso); + +#ifndef _LIBC +// Libc used to export this in previous versions, therefore it needs +// to remain global for binary compatibility. +__attribute__ ((visibility ("hidden"))) +#endif +int pthread_atfork(void (*prepare)(void), void (*parent)(void), void (*child)(void)) { + return __register_atfork(prepare, parent, child, &__dso_handle); +} + diff --git a/libc/arch-mips/bionic/crtbegin.c b/libc/arch-mips/bionic/crtbegin.c index 50e9eeb02..d72ec7b8d 100644 --- a/libc/arch-mips/bionic/crtbegin.c +++ b/libc/arch-mips/bionic/crtbegin.c @@ -92,3 +92,4 @@ __asm__ ( #include "../../arch-common/bionic/__dso_handle.h" #include "../../arch-common/bionic/atexit.h" +#include "../../arch-common/bionic/pthread_atfork.h" diff --git a/libc/bionic/pthread_atfork.cpp b/libc/bionic/pthread_atfork.cpp index d1c4ad0c4..093ffd240 100644 --- a/libc/bionic/pthread_atfork.cpp +++ b/libc/bionic/pthread_atfork.cpp @@ -30,6 +30,8 @@ #include #include +#include "private/bionic_macros.h" + struct atfork_t { atfork_t* next; atfork_t* prev; @@ -37,79 +39,143 @@ struct atfork_t { void (*prepare)(void); void (*child)(void); void (*parent)(void); + + void* dso_handle; }; -struct atfork_list_t { - atfork_t* first; - atfork_t* last; +class atfork_list_t { + public: + atfork_list_t() : first_(nullptr), last_(nullptr) {} + + template + void walk_forward(F f) { + for (atfork_t* it = first_; it != nullptr; it = it->next) { + f(it); + } + } + + template + void walk_backwards(F f) { + for (atfork_t* it = last_; it != nullptr; it = it->prev) { + f(it); + } + } + + void push_back(atfork_t* entry) { + entry->next = nullptr; + entry->prev = last_; + if (entry->prev != nullptr) { + entry->prev->next = entry; + } + if (first_ == nullptr) { + first_ = entry; + } + last_ = entry; + } + + template + void remove_if(F predicate) { + atfork_t* it = first_; + while (it != nullptr) { + if (predicate(it)) { + atfork_t* entry = it; + it = it->next; + remove(entry); + } else { + it = it->next; + } + } + } + + private: + void remove(atfork_t* entry) { + if (entry->prev != nullptr) { + entry->prev->next = entry->next; + } else { + first_ = entry->next; + } + + if (entry->next != nullptr) { + entry->next->prev = entry->prev; + } else { + last_ = entry->prev; + } + + free(entry); + } + + atfork_t* first_; + atfork_t* last_; + + DISALLOW_COPY_AND_ASSIGN(atfork_list_t); }; static pthread_mutex_t g_atfork_list_mutex = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP; -static atfork_list_t g_atfork_list = { NULL, NULL }; +static atfork_list_t g_atfork_list; void __bionic_atfork_run_prepare() { // We lock the atfork list here, unlock it in the parent, and reset it in the child. // This ensures that nobody can modify the handler array between the calls // to the prepare and parent/child handlers. - // - // TODO: If a handler tries to mutate the list, they'll block. We should probably copy - // the list before forking, and have prepare, parent, and child all work on the consistent copy. pthread_mutex_lock(&g_atfork_list_mutex); // Call pthread_atfork() prepare handlers. POSIX states that the prepare // handlers should be called in the reverse order of the parent/child // handlers, so we iterate backwards. - for (atfork_t* it = g_atfork_list.last; it != NULL; it = it->prev) { - if (it->prepare != NULL) { + g_atfork_list.walk_backwards([](atfork_t* it) { + if (it->prepare != nullptr) { it->prepare(); } - } + }); } void __bionic_atfork_run_child() { - for (atfork_t* it = g_atfork_list.first; it != NULL; it = it->next) { - if (it->child != NULL) { + g_atfork_list.walk_forward([](atfork_t* it) { + if (it->child != nullptr) { it->child(); } - } + }); g_atfork_list_mutex = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP; } void __bionic_atfork_run_parent() { - for (atfork_t* it = g_atfork_list.first; it != NULL; it = it->next) { - if (it->parent != NULL) { + g_atfork_list.walk_forward([](atfork_t* it) { + if (it->parent != nullptr) { it->parent(); } - } + }); pthread_mutex_unlock(&g_atfork_list_mutex); } -int pthread_atfork(void (*prepare)(void), void (*parent)(void), void(*child)(void)) { +// __register_atfork is the name used by glibc +extern "C" int __register_atfork(void (*prepare)(void), void (*parent)(void), + void(*child)(void), void* dso) { atfork_t* entry = reinterpret_cast(malloc(sizeof(atfork_t))); - if (entry == NULL) { + if (entry == nullptr) { return ENOMEM; } entry->prepare = prepare; entry->parent = parent; entry->child = child; + entry->dso_handle = dso; pthread_mutex_lock(&g_atfork_list_mutex); - // Append 'entry' to the list. - entry->next = NULL; - entry->prev = g_atfork_list.last; - if (entry->prev != NULL) { - entry->prev->next = entry; - } - if (g_atfork_list.first == NULL) { - g_atfork_list.first = entry; - } - g_atfork_list.last = entry; + g_atfork_list.push_back(entry); pthread_mutex_unlock(&g_atfork_list_mutex); return 0; } + +extern "C" __LIBC_HIDDEN__ void __unregister_atfork(void* dso) { + pthread_mutex_lock(&g_atfork_list_mutex); + g_atfork_list.remove_if([&](const atfork_t* entry) { + return entry->dso_handle == dso; + }); + pthread_mutex_unlock(&g_atfork_list_mutex); +} + diff --git a/libc/upstream-openbsd/lib/libc/stdlib/atexit.c b/libc/stdlib/atexit.c similarity index 92% rename from libc/upstream-openbsd/lib/libc/stdlib/atexit.c rename to libc/stdlib/atexit.c index 6532b382e..df2b1b5d5 100644 --- a/libc/upstream-openbsd/lib/libc/stdlib/atexit.c +++ b/libc/stdlib/atexit.c @@ -35,11 +35,15 @@ #include #include #include "atexit.h" -#include "thread_private.h" +#include "private/thread_private.h" struct atexit *__atexit; static int restartloop; +/* BEGIN android-changed: __unregister_atfork is used by __cxa_finalize */ +extern void __unregister_atfork(void* dso); +/* END android-changed */ + /* * Function pointers are stored in a linked list of pages. The list * is initially empty, and pages are allocated on demand. The first @@ -62,7 +66,7 @@ __cxa_atexit(void (*func)(void *), void *arg, void *dso) { struct atexit *p = __atexit; struct atexit_fn *fnp; - int pgsize = getpagesize(); + size_t pgsize = getpagesize(); int ret = -1; if (pgsize < sizeof(*p)) @@ -161,6 +165,12 @@ restart: __atexit = NULL; } _ATEXIT_UNLOCK(); + + /* BEGIN android-changed: call __unregister_atfork if dso is not null */ + if (dso != NULL) { + __unregister_atfork(dso); + } + /* END android-changed */ } /* @@ -170,7 +180,7 @@ void __atexit_register_cleanup(void (*func)(void)) { struct atexit *p; - int pgsize = getpagesize(); + size_t pgsize = getpagesize(); if (pgsize < sizeof(*p)) return; diff --git a/libc/upstream-openbsd/lib/libc/stdlib/atexit.h b/libc/stdlib/atexit.h similarity index 100% rename from libc/upstream-openbsd/lib/libc/stdlib/atexit.h rename to libc/stdlib/atexit.h diff --git a/tests/libs/Android.build.pthread_atfork.mk b/tests/libs/Android.build.pthread_atfork.mk new file mode 100644 index 000000000..72ffec4ec --- /dev/null +++ b/tests/libs/Android.build.pthread_atfork.mk @@ -0,0 +1,25 @@ +# +# Copyright (C) 2014 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. +# + +# ----------------------------------------------------------------------------- +# This library used to test phtread_atfork handler behaviour +# during/after dlclose. +# ----------------------------------------------------------------------------- +libtest_pthread_atfork_src_files := pthread_atfork.cpp + +module := libtest_pthread_atfork +include $(LOCAL_PATH)/Android.build.testlib.mk + diff --git a/tests/libs/Android.mk b/tests/libs/Android.mk index 3d5b060de..c78661e83 100644 --- a/tests/libs/Android.mk +++ b/tests/libs/Android.mk @@ -25,6 +25,7 @@ common_additional_dependencies := \ $(LOCAL_PATH)/Android.build.dlopen_check_order_dlsym.mk \ $(LOCAL_PATH)/Android.build.dlopen_check_order_reloc_siblings.mk \ $(LOCAL_PATH)/Android.build.dlopen_check_order_reloc_main_executable.mk \ + $(LOCAL_PATH)/Android.build.pthread_atfork.mk \ $(LOCAL_PATH)/Android.build.testlib.mk \ $(LOCAL_PATH)/Android.build.versioned_lib.mk \ $(TEST_PATH)/Android.build.mk @@ -203,6 +204,11 @@ include $(LOCAL_PATH)/Android.build.dlopen_check_order_reloc_main_executable.mk # ----------------------------------------------------------------------------- include $(LOCAL_PATH)/Android.build.versioned_lib.mk +# ----------------------------------------------------------------------------- +# Build libraries needed by pthread_atfork tests +# ----------------------------------------------------------------------------- +include $(LOCAL_PATH)/Android.build.pthread_atfork.mk + # ----------------------------------------------------------------------------- # Library with dependency loop used by dlfcn tests # diff --git a/tests/libs/pthread_atfork.cpp b/tests/libs/pthread_atfork.cpp new file mode 100644 index 000000000..3a5aa4faf --- /dev/null +++ b/tests/libs/pthread_atfork.cpp @@ -0,0 +1,21 @@ +/* + * Copyright (C) 2014 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 + +extern "C" int proxy_pthread_atfork(void (*prepare)(void), void (*parent)(void), void (*child)(void)) { + return pthread_atfork(prepare, parent, child); +} diff --git a/tests/pthread_test.cpp b/tests/pthread_test.cpp index a299f02f7..b8cfd565b 100644 --- a/tests/pthread_test.cpp +++ b/tests/pthread_test.cpp @@ -16,6 +16,7 @@ #include +#include #include #include #include @@ -987,14 +988,14 @@ TEST(pthread, pthread_once_1934122) { } static int g_atfork_prepare_calls = 0; -static void AtForkPrepare1() { g_atfork_prepare_calls = (g_atfork_prepare_calls << 4) | 1; } -static void AtForkPrepare2() { g_atfork_prepare_calls = (g_atfork_prepare_calls << 4) | 2; } +static void AtForkPrepare1() { g_atfork_prepare_calls = (g_atfork_prepare_calls * 10) + 1; } +static void AtForkPrepare2() { g_atfork_prepare_calls = (g_atfork_prepare_calls * 10) + 2; } static int g_atfork_parent_calls = 0; -static void AtForkParent1() { g_atfork_parent_calls = (g_atfork_parent_calls << 4) | 1; } -static void AtForkParent2() { g_atfork_parent_calls = (g_atfork_parent_calls << 4) | 2; } +static void AtForkParent1() { g_atfork_parent_calls = (g_atfork_parent_calls * 10) + 1; } +static void AtForkParent2() { g_atfork_parent_calls = (g_atfork_parent_calls * 10) + 2; } static int g_atfork_child_calls = 0; -static void AtForkChild1() { g_atfork_child_calls = (g_atfork_child_calls << 4) | 1; } -static void AtForkChild2() { g_atfork_child_calls = (g_atfork_child_calls << 4) | 2; } +static void AtForkChild1() { g_atfork_child_calls = (g_atfork_child_calls * 10) + 1; } +static void AtForkChild2() { g_atfork_child_calls = (g_atfork_child_calls * 10) + 2; } TEST(pthread, pthread_atfork_smoke) { ASSERT_EQ(0, pthread_atfork(AtForkPrepare1, AtForkParent1, AtForkChild1)); @@ -1005,13 +1006,71 @@ TEST(pthread, pthread_atfork_smoke) { // Child and parent calls are made in the order they were registered. if (pid == 0) { - ASSERT_EQ(0x12, g_atfork_child_calls); + ASSERT_EQ(12, g_atfork_child_calls); _exit(0); } - ASSERT_EQ(0x12, g_atfork_parent_calls); + ASSERT_EQ(12, g_atfork_parent_calls); // Prepare calls are made in the reverse order. - ASSERT_EQ(0x21, g_atfork_prepare_calls); + ASSERT_EQ(21, g_atfork_prepare_calls); + int status; + ASSERT_EQ(pid, waitpid(pid, &status, 0)); +} + +static void AtForkPrepare3() { g_atfork_prepare_calls = (g_atfork_prepare_calls * 10) + 3; } +static void AtForkPrepare4() { g_atfork_prepare_calls = (g_atfork_prepare_calls * 10) + 4; } + +static void AtForkParent3() { g_atfork_parent_calls = (g_atfork_parent_calls * 10) + 3; } +static void AtForkParent4() { g_atfork_parent_calls = (g_atfork_parent_calls * 10) + 4; } + +static void AtForkChild3() { g_atfork_child_calls = (g_atfork_child_calls * 10) + 3; } +static void AtForkChild4() { g_atfork_child_calls = (g_atfork_child_calls * 10) + 4; } + +TEST(pthread, pthread_atfork_with_dlclose) { + ASSERT_EQ(0, pthread_atfork(AtForkPrepare1, AtForkParent1, AtForkChild1)); + + void* handle = dlopen("libtest_pthread_atfork.so", RTLD_NOW | RTLD_LOCAL); + ASSERT_TRUE(handle != nullptr) << dlerror(); + typedef int (*fn_t)(void (*)(void), void (*)(void), void (*)(void)); + fn_t fn = reinterpret_cast(dlsym(handle, "proxy_pthread_atfork")); + ASSERT_TRUE(fn != nullptr) << dlerror(); + // the library registers 2 additional atfork handlers in a constructor + ASSERT_EQ(0, fn(AtForkPrepare2, AtForkParent2, AtForkChild2)); + ASSERT_EQ(0, fn(AtForkPrepare3, AtForkParent3, AtForkChild3)); + + ASSERT_EQ(0, pthread_atfork(AtForkPrepare4, AtForkParent4, AtForkChild4)); + + int pid = fork(); + + ASSERT_NE(-1, pid) << strerror(errno); + + if (pid == 0) { + ASSERT_EQ(1234, g_atfork_child_calls); + _exit(0); + } + + ASSERT_EQ(1234, g_atfork_parent_calls); + ASSERT_EQ(4321, g_atfork_prepare_calls); + + EXPECT_EQ(0, dlclose(handle)); + g_atfork_prepare_calls = g_atfork_parent_calls = g_atfork_child_calls = 0; + + int status; + ASSERT_EQ(pid, waitpid(pid, &status, 0)); + + pid = fork(); + + ASSERT_NE(-1, pid) << strerror(errno); + + if (pid == 0) { + ASSERT_EQ(14, g_atfork_child_calls); + _exit(0); + } + + ASSERT_EQ(14, g_atfork_parent_calls); + ASSERT_EQ(41, g_atfork_prepare_calls); + + ASSERT_EQ(pid, waitpid(pid, &status, 0)); } TEST(pthread, pthread_attr_getscope) {