OSDN Git Service

Fixes for __cxa_finalize
authorDmitriy Ivanov <dimitry@google.com>
Tue, 29 Apr 2014 15:41:29 +0000 (08:41 -0700)
committerDmitriy Ivanov <dimitry@google.com>
Mon, 5 May 2014 18:36:57 +0000 (11:36 -0700)
  * 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
libc/stdlib/atexit.c
libc/stdlib/atexit.h
tests/Android.mk
tests/atexit_test.cpp [new file with mode: 0644]
tests/atexit_testlib.cpp [new file with mode: 0644]

index 16ae7aa..90aa030 100644 (file)
  * SUCH DAMAGE.
  */
 
+#include <stddef.h>
+
 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));
 }
index 4e14434..b051e22 100644 (file)
@@ -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;
index 4b3e5ab..2e88ad6 100644 (file)
@@ -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 */
index fccf3f1..4e82591 100644 (file)
@@ -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 (file)
index 0000000..e52235d
--- /dev/null
@@ -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 <gtest/gtest.h>
+
+#include <dlfcn.h>
+#include <libgen.h>
+#include <limits.h>
+#include <stdio.h>
+#include <stdint.h>
+
+#include <string>
+
+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<void (*)(std::string*, bool*)>(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 (file)
index 0000000..36393e7
--- /dev/null
@@ -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 <stdio.h>
+#include <stdlib.h>
+
+#include <string>
+
+// 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);
+}
+