From 6b56691a678420477595a531b2d2829980eb51c0 Mon Sep 17 00:00:00 2001 From: Dmitriy Ivanov Date: Tue, 29 Apr 2014 08:41:29 -0700 Subject: [PATCH] Fixes for __cxa_finalize * Ability to register atexit handler from atexit handler * Correct way to handle both forms of atexit handler Bug: https://code.google.com/p/android/issues/detail?id=66595 Bug: 4998315 Change-Id: I39529afaef97b6e1469c21389d54c0d7d175da28 --- libc/arch-common/bionic/atexit.h | 11 ++++- libc/stdlib/atexit.c | 97 +++++++++++++++++++++++++++++----------- libc/stdlib/atexit.h | 5 +-- tests/Android.mk | 18 +++++++- tests/atexit_test.cpp | 44 ++++++++++++++++++ tests/atexit_testlib.cpp | 75 +++++++++++++++++++++++++++++++ 6 files changed, 217 insertions(+), 33 deletions(-) create mode 100644 tests/atexit_test.cpp create mode 100644 tests/atexit_testlib.cpp diff --git a/libc/arch-common/bionic/atexit.h b/libc/arch-common/bionic/atexit.h index 16ae7aab2..90aa030ea 100644 --- a/libc/arch-common/bionic/atexit.h +++ b/libc/arch-common/bionic/atexit.h @@ -26,11 +26,20 @@ * SUCH DAMAGE. */ +#include + extern void* __dso_handle; extern int __cxa_atexit(void (*)(void*), void*, void*); __attribute__ ((visibility ("hidden"))) +void __atexit_handler_wrapper(void* func) { + if (func != NULL) { + (*(void (*)(void))func)(); + } +} + +__attribute__ ((visibility ("hidden"))) int atexit(void (*func)(void)) { - return (__cxa_atexit((void (*)(void*)) func, (void*) 0, &__dso_handle)); + return (__cxa_atexit(&__atexit_handler_wrapper, func, &__dso_handle)); } diff --git a/libc/stdlib/atexit.c b/libc/stdlib/atexit.c index 4e14434d6..b051e22d5 100644 --- a/libc/stdlib/atexit.c +++ b/libc/stdlib/atexit.c @@ -41,11 +41,52 @@ int __atexit_invalid = 1; struct atexit *__atexit; /* + * TODO: Read this before upstreaming: + * + * As of Apr 2014 there is a bug regaring function type detection logic in + * Free/Open/NetBSD implementations of __cxa_finalize(). + * + * What it is about: + * First of all there are two kind of atexit handlers: + * 1) void handler(void) - this is the regular type + * available for to user via atexit(.) function call. + * + * 2) void internal_handler(void*) - this is the type + * __cxa_atexit() function expects. This handler is used + * by C++ compiler to register static destructor calls. + * Note that calling this function as the handler of type (1) + * results in incorrect this pointer in static d-tors. + * + * What is wrong with BSD implementations: + * + * They use dso argument to identify the handler type. The problem + * with it is dso is also used to identify the handlers associated + * with particular dynamic library and allow __cxa_finalize to call correct + * set of functions on dlclose(). And it cannot identify both. + * + * What is correct way to identify function type? + * + * Consider this: + * 1. __cxa_finalize and __cxa_atexit are part of libc and do not have access to hidden + * &__dso_handle. + * 2. __cxa_atexit has only 3 arguments: function pointer, function argument, dso. + * none of them can be reliably used to pass information about handler type. + * 3. following http://www.codesourcery.com/cxx-abi/abi.html#dso-dtor (3.3.5.3 - B) + * translation of user atexit -> __cxa_atexit(f, NULL, NULL) results in crashes + * on exit() after dlclose() of a library with an atexit() call. + * + * One way to resolve this is to always call second form of handler, which will + * result in storing unused argument in register/stack depending on architecture + * and should not present any problems. + * + * Another way is to make them dso-local in one way or the other. + */ + +/* * Function pointers are stored in a linked list of pages. The list * is initially empty, and pages are allocated on demand. The first * function pointer in the first allocated page (the last one in * the linked list) was reserved for the cleanup function. - * TODO: switch to the regular FreeBSD/NetBSD atexit implementation. * * Outside the following functions, all pages are mprotect()'ed * to prevent unintentional/malicious corruption. @@ -94,7 +135,7 @@ __cxa_atexit(void (*func)(void *), void *arg, void *dso) __atexit_invalid = 0; } fnp = &p->fns[p->ind++]; - fnp->fn_ptr.cxa_func = func; + fnp->cxa_func = func; fnp->fn_arg = arg; fnp->fn_dso = dso; if (mprotect(p, pgsize, PROT_READ)) @@ -113,57 +154,59 @@ unlock: void __cxa_finalize(void *dso) { - struct atexit *p, *q; + struct atexit *p, *q, *original_atexit; struct atexit_fn fn; - int n, pgsize = getpagesize(); + int n, pgsize = getpagesize(), original_ind; static int call_depth; if (__atexit_invalid) return; - _ATEXIT_LOCK(); call_depth++; - for (p = __atexit; p != NULL; p = p->next) { - for (n = p->ind; --n >= 0;) { - if (p->fns[n].fn_ptr.cxa_func == NULL) - continue; /* already called */ - if (dso != NULL && dso != p->fns[n].fn_dso) - continue; /* wrong DSO */ - + p = original_atexit = __atexit; + n = original_ind = p != NULL ? p->ind : 0; + while (p != NULL) { + if (p->fns[n].cxa_func != NULL /* not called */ + && (dso == NULL || dso == p->fns[n].fn_dso)) { /* correct DSO */ /* * Mark handler as having been already called to avoid * dupes and loops, then call the appropriate function. */ fn = p->fns[n]; if (mprotect(p, pgsize, PROT_READ | PROT_WRITE) == 0) { - p->fns[n].fn_ptr.cxa_func = NULL; + p->fns[n].cxa_func = NULL; mprotect(p, pgsize, PROT_READ); } + _ATEXIT_UNLOCK(); -#if ANDROID - /* it looks like we should always call the function - * with an argument, even if dso is not NULL. Otherwise - * static destructors will not be called properly on - * the ARM. - */ - (*fn.fn_ptr.cxa_func)(fn.fn_arg); -#else /* !ANDROID */ - if (dso != NULL) - (*fn.fn_ptr.cxa_func)(fn.fn_arg); - else - (*fn.fn_ptr.std_func)(); -#endif /* !ANDROID */ + (*fn.cxa_func)(fn.fn_arg); _ATEXIT_LOCK(); + // check for new atexit handlers + if ((__atexit->ind != original_ind) || (__atexit != original_atexit)) { + // need to restart now to preserve correct + // call order - LIFO + p = original_atexit = __atexit; + n = original_ind = p->ind; + continue; + } + } + if (n == 0) { + p = p->next; + n = p != NULL ? p->ind : 0; + } else { + --n; } } + --call_depth; + /* * If called via exit(), unmap the pages since we have now run * all the handlers. We defer this until calldepth == 0 so that * we don't unmap things prematurely if called recursively. */ - if (dso == NULL && --call_depth == 0) { + if (dso == NULL && call_depth == 0) { for (p = __atexit; p != NULL; ) { q = p; p = p->next; diff --git a/libc/stdlib/atexit.h b/libc/stdlib/atexit.h index 4b3e5aba4..2e88ad646 100644 --- a/libc/stdlib/atexit.h +++ b/libc/stdlib/atexit.h @@ -37,10 +37,7 @@ struct atexit { int ind; /* next index in this table */ int max; /* max entries >= ATEXIT_SIZE */ struct atexit_fn { - union { - void (*std_func)(void); - void (*cxa_func)(void *); - } fn_ptr; + void (*cxa_func)(void *); void *fn_arg; /* argument for CXA callback */ void *fn_dso; /* shared module handle */ } fns[1]; /* the table itself */ diff --git a/tests/Android.mk b/tests/Android.mk index fccf3f1d1..4e82591ac 100644 --- a/tests/Android.mk +++ b/tests/Android.mk @@ -210,6 +210,18 @@ build_target := SHARED_LIBRARY include $(LOCAL_PATH)/Android.build.mk # ----------------------------------------------------------------------------- +# This library used by atexit tests +# ----------------------------------------------------------------------------- + +libtest_atexit_src_files := \ + atexit_testlib.cpp + +module := libtest_atexit +build_type := target +build_target := SHARED_LIBRARY +include $(LOCAL_PATH)/Android.build.mk + +# ----------------------------------------------------------------------------- # Tests for the device using bionic's .so. Run with: # adb shell /data/nativetest/bionic-unit-tests/bionic-unit-tests # ----------------------------------------------------------------------------- @@ -217,6 +229,7 @@ bionic-unit-tests_whole_static_libraries := \ libBionicTests \ bionic-unit-tests_src_files := \ + atexit_test.cpp \ dlext_test.cpp \ dlfcn_test.cpp \ @@ -263,11 +276,14 @@ include $(LOCAL_PATH)/Android.build.mk ifeq ($(HOST_OS)-$(HOST_ARCH),linux-x86) +bionic-unit-tests-glibc_src_files := \ + atexit_test.cpp \ + bionic-unit-tests-glibc_whole_static_libraries := \ libBionicStandardTests \ bionic-unit-tests-glibc_ldlibs := \ - -lrt \ + -lrt -ldl \ module := bionic-unit-tests-glibc module_tag := optional diff --git a/tests/atexit_test.cpp b/tests/atexit_test.cpp new file mode 100644 index 000000000..e52235d20 --- /dev/null +++ b/tests/atexit_test.cpp @@ -0,0 +1,44 @@ +/* + * 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 + +#include +#include +#include +#include +#include + +#include + +TEST(atexit, combined_test) { + std::string atexit_call_sequence; + bool valid_this_in_static_dtor = false; + void* handle = dlopen("libtest_atexit.so", RTLD_NOW); + ASSERT_TRUE(handle != NULL); + + void* sym = dlsym(handle, "register_atexit"); + ASSERT_TRUE(sym != NULL); + reinterpret_cast(sym)(&atexit_call_sequence, &valid_this_in_static_dtor); + + ASSERT_EQ(0, dlclose(handle)); + // this test verifies atexit call from atexit handler. as well as the order of calls + ASSERT_EQ("Humpty Dumpty sat on a wall", atexit_call_sequence); + ASSERT_TRUE(valid_this_in_static_dtor); +} + +// TODO: test for static dtor calls from from exit(.) -> __cxa_finalize(NULL) + diff --git a/tests/atexit_testlib.cpp b/tests/atexit_testlib.cpp new file mode 100644 index 000000000..36393e738 --- /dev/null +++ b/tests/atexit_testlib.cpp @@ -0,0 +1,75 @@ +/* + * 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 +#include + +#include + +// use external control number from main test +static std::string* atexit_sequence = NULL; +static bool* atexit_valid_this_in_static_dtor = NULL; + +class AtExitStaticClass; + +static const AtExitStaticClass* valid_this = NULL; + +static class AtExitStaticClass { +public: + AtExitStaticClass() { valid_this = this; } + ~AtExitStaticClass() { + if (atexit_valid_this_in_static_dtor) { + *atexit_valid_this_in_static_dtor = (valid_this == this); + } + } +} staticObj; + +// 4 +static void atexit_handler_from_atexit_from_atexit2() { + *atexit_sequence += " on"; +} + +// 3 +static void atexit_handler_from_atexit_from_atexit1() { + *atexit_sequence += " sat"; +} + +// 2 +static void atexit_handler_from_atexit() { + *atexit_sequence += " Dumpty"; + // register 2 others + atexit(atexit_handler_from_atexit_from_atexit2); + atexit(atexit_handler_from_atexit_from_atexit1); +} + +// 1 +static void atexit_handler_with_atexit() { + *atexit_sequence += "Humpty"; + atexit(atexit_handler_from_atexit); +} + +// last +static void atexit_handler_regular() { + *atexit_sequence += " a wall"; +} + +extern "C" void register_atexit(std::string* sequence, bool* valid_this_in_static_dtor) { + atexit_sequence = sequence; + atexit_valid_this_in_static_dtor = valid_this_in_static_dtor; + atexit(atexit_handler_regular); + atexit(atexit_handler_with_atexit); + atexit(NULL); +} + -- 2.11.0