]> sourceware.org Git - glibc.git/commitdiff
Fix BZ 14333
authorPaul Pluzhnikov <ppluzhnikov@google.com>
Wed, 20 Sep 2017 16:31:48 +0000 (09:31 -0700)
committerPaul Pluzhnikov <ppluzhnikov@google.com>
Wed, 20 Sep 2017 16:31:48 +0000 (09:31 -0700)
ChangeLog
stdlib/Makefile
stdlib/cxa_atexit.c
stdlib/cxa_finalize.c
stdlib/exit.c
stdlib/exit.h
stdlib/on_exit.c
stdlib/test-at_quick_exit-race.c [new file with mode: 0644]
stdlib/test-atexit-race.c [new file with mode: 0644]
stdlib/test-cxa_atexit-race.c [new file with mode: 0644]
stdlib/test-on_exit-race.c [new file with mode: 0644]

index 935247563f2dc00fff2da5467f4c6af106391aef..a07c903731f6e27ea68e8869018101a3e2bd57bb 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,25 @@
+2017-09-20  Paul Pluzhnikov  <ppluzhnikov@google.com>
+            Ricky Zhou  <rickyz@google.com>
+            Anoop V Chakkalakkal  <anoop.vijayan@in.ibm.com>
+
+       [BZ #14333]
+       * stdlib/cxa_atexit.c (__internal_atexit): Use __exit_funcs_lock.
+       Remove atomics.
+       (__new_exitfn): Fail registration when we finished at_exit processing.
+       * stdlib/cxa_finalize.c (__cxa_finalize): Likewise.
+       * stdlib/on_exit.c (__on_exit): Likewise.
+       * stdlib/exit.c (__exit_funcs_done): New variable.
+       (__run_exit_handlers): Use __exit_funcs_lock.
+       * stdlib/exit.h (__exit_funcs_lock, __exit_funcs_done): New
+       declarations.
+       * stdlib/Makefile (test-atexit-race, test-quick_at_exit-race)
+       (test-cxa_atexit-race, test-on_exit-race): New tests.
+       * stdlib/test-atexit-race-common.c: New file.
+       * stdlib/test-atexit-race.c: New file.
+       * stdlib/test-at_quick_exit-race.c: New file.
+       * stdlib/test-cxa_atexit-race.c: New file.
+       * stdlib/test-on_exit-race.c: New file.
+
 2017-09-20  Szabolcs Nagy  <szabolcs.nagy@arm.com>
 
        * benchtests/Makefile: Add exp2f and log2f benchmarks.
index 2da39e067c9f7c592f2470b9c4a0ff0f1ff5983a..2fb08342e0453d3e166dfd869fa685310a7d7bc6 100644 (file)
@@ -81,7 +81,9 @@ tests         := tst-strtol tst-strtod testmb testrand testsort testdiv   \
                   tst-quick_exit tst-thread-quick_exit tst-width           \
                   tst-width-stdint tst-strfrom tst-strfrom-locale          \
                   tst-getrandom tst-atexit tst-at_quick_exit               \
-                  tst-cxa_atexit tst-on_exit
+                  tst-cxa_atexit tst-on_exit test-atexit-race              \
+                  test-at_quick_exit-race test-cxa_atexit-race             \
+                  test-on_exit-race
 
 tests-internal := tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
                   tst-tls-atexit tst-tls-atexit-nodelete
@@ -91,6 +93,11 @@ ifeq ($(build-hardcoded-path-in-tests),yes)
 tests += tst-empty-env
 endif
 
+LDLIBS-test-atexit-race = $(shared-thread-library)
+LDLIBS-test-at_quick_exit-race = $(shared-thread-library)
+LDLIBS-test-cxa_atexit-race = $(shared-thread-library)
+LDLIBS-test-on_exit-race = $(shared-thread-library)
+
 ifeq ($(have-cxx-thread_local),yes)
 CFLAGS-tst-quick_exit.o = -std=c++11
 LDLIBS-tst-quick_exit = -lstdc++
index ce5d9f22b45f12b60b31f0b1c2ba4bce31e22e79..beb31691d5f9aca7fa56beb611ec79daf7de9e2f 100644 (file)
 
 #include <libc-lock.h>
 #include "exit.h"
-#include <atomic.h>
 #include <sysdep.h>
 
 #undef __cxa_atexit
 
+/* See concurrency notes in stdlib/exit.h where this lock is declared.  */
+__libc_lock_define_initialized (, __exit_funcs_lock)
+
 
 int
 attribute_hidden
 __internal_atexit (void (*func) (void *), void *arg, void *d,
                   struct exit_function_list **listp)
 {
-  struct exit_function *new = __new_exitfn (listp);
+  struct exit_function *new;
+
+  __libc_lock_lock (__exit_funcs_lock);
+  new = __new_exitfn (listp);
 
   if (new == NULL)
-    return -1;
+    {
+      __libc_lock_unlock (__exit_funcs_lock);
+      return -1;
+    }
 
 #ifdef PTR_MANGLE
   PTR_MANGLE (func);
@@ -43,8 +51,8 @@ __internal_atexit (void (*func) (void *), void *arg, void *d,
   new->func.cxa.fn = (void (*) (void *, int)) func;
   new->func.cxa.arg = arg;
   new->func.cxa.dso_handle = d;
-  atomic_write_barrier ();
   new->flavor = ef_cxa;
+  __libc_lock_unlock (__exit_funcs_lock);
   return 0;
 }
 
@@ -60,14 +68,11 @@ __cxa_atexit (void (*func) (void *), void *arg, void *d)
 libc_hidden_def (__cxa_atexit)
 
 
-/* We change global data, so we need locking.  */
-__libc_lock_define_initialized (static, lock)
-
-
 static struct exit_function_list initial;
 struct exit_function_list *__exit_funcs = &initial;
 uint64_t __new_exitfn_called;
 
+/* Must be called with __exit_funcs_lock held.  */
 struct exit_function *
 __new_exitfn (struct exit_function_list **listp)
 {
@@ -76,7 +81,10 @@ __new_exitfn (struct exit_function_list **listp)
   struct exit_function *r = NULL;
   size_t i = 0;
 
-  __libc_lock_lock (lock);
+  if (__exit_funcs_done)
+    /* Exit code is finished processing all registered exit functions,
+       therefore we fail this registration.  */
+    return NULL;
 
   for (l = *listp; l != NULL; p = l, l = l->next)
     {
@@ -127,7 +135,5 @@ __new_exitfn (struct exit_function_list **listp)
       ++__new_exitfn_called;
     }
 
-  __libc_lock_unlock (lock);
-
   return r;
 }
index aa0a70cb58c1ca60131b9ffa2a469ca32c319dab..23fc6dcafb335b08bbb915846f22bfb524be8f12 100644 (file)
@@ -17,7 +17,6 @@
 
 #include <assert.h>
 #include <stdlib.h>
-#include <atomic.h>
 #include "exit.h"
 #include <fork.h>
 #include <sysdep.h>
@@ -31,36 +30,64 @@ __cxa_finalize (void *d)
 {
   struct exit_function_list *funcs;
 
+  __libc_lock_lock (__exit_funcs_lock);
+
  restart:
   for (funcs = __exit_funcs; funcs; funcs = funcs->next)
     {
       struct exit_function *f;
 
       for (f = &funcs->fns[funcs->idx - 1]; f >= &funcs->fns[0]; --f)
-       {
-         void (*cxafn) (void *arg, int status);
-         void *cxaarg;
+       if ((d == NULL || d == f->func.cxa.dso_handle) && f->flavor == ef_cxa)
+         {
+           const uint64_t check = __new_exitfn_called;
+           void (*cxafn) (void *arg, int status) = f->func.cxa.fn;
+           void *cxaarg = f->func.cxa.arg;
+
+           /* We don't want to run this cleanup more than once.  The Itanium
+              C++ ABI requires that multiple calls to __cxa_finalize not
+              result in calling termination functions more than once.  One
+              potential scenario where that could happen is with a concurrent
+              dlclose and exit, where the running dlclose must at some point
+              release the list lock, an exiting thread may acquire it, and
+              without setting flavor to ef_free, might re-run this destructor
+              which could result in undefined behaviour.  Therefore we must
+              set flavor to ef_free to avoid calling this destructor again.
+              Note that the concurrent exit must also take the dynamic loader
+              lock (for library finalizer processing) and therefore will
+              block while dlclose completes the processing of any in-progress
+              exit functions. Lastly, once we release the list lock for the
+              entry marked ef_free, we must not read from that entry again
+              since it may have been reused by the time we take the list lock
+              again.  Lastly the detection of new registered exit functions is
+              based on a monotonically incrementing counter, and there is an
+              ABA if between the unlock to run the exit function and the
+              re-lock after completion the user registers 2^64 exit functions,
+              the implementation will not detect this and continue without
+              executing any more functions.
 
-         if ((d == NULL || d == f->func.cxa.dso_handle)
-             /* We don't want to run this cleanup more than once.  */
-             && (cxafn = f->func.cxa.fn,
-                 cxaarg = f->func.cxa.arg,
-                 ! catomic_compare_and_exchange_bool_acq (&f->flavor, ef_free,
-                                                          ef_cxa)))
-           {
-             uint64_t check = __new_exitfn_called;
+              One minor issue remains: A registered exit function that is in
+              progress by a call to dlclose() may not completely finish before
+              the next registered exit function is run. This may, according to
+              some readings of POSIX violate the requirement that functions
+              run in effective LIFO order.  This should probably be fixed in a
+              future implementation to ensure the functions do not run in
+              parallel.  */
+           f->flavor = ef_free;
 
 #ifdef PTR_DEMANGLE
-             PTR_DEMANGLE (cxafn);
+           PTR_DEMANGLE (cxafn);
 #endif
-             cxafn (cxaarg, 0);
+           /* Unlock the list while we call a foreign function.  */
+           __libc_lock_unlock (__exit_funcs_lock);
+           cxafn (cxaarg, 0);
+           __libc_lock_lock (__exit_funcs_lock);
 
-             /* It is possible that that last exit function registered
-                more exit functions.  Start the loop over.  */
-             if (__glibc_unlikely (check != __new_exitfn_called))
-               goto restart;
-           }
-       }
+           /* It is possible that that last exit function registered
+              more exit functions.  Start the loop over.  */
+           if (__glibc_unlikely (check != __new_exitfn_called))
+             goto restart;
+         }
     }
 
   /* Also remove the quick_exit handlers, but do not call them.  */
@@ -79,4 +106,5 @@ __cxa_finalize (void *d)
   if (d != NULL)
     UNREGISTER_ATFORK (d);
 #endif
+  __libc_lock_unlock (__exit_funcs_lock);
 }
index c0b6d666c7be7e109165fdebb495f5e6478fb31e..b74f1825f08b8d64143b54a1af8fc53ed9f88ec2 100644 (file)
 #include <stdlib.h>
 #include <unistd.h>
 #include <sysdep.h>
+#include <libc-lock.h>
 #include "exit.h"
 
 #include "set-hooks.h"
 DEFINE_HOOK (__libc_atexit, (void))
 
+/* Initialize the flag that indicates exit function processing
+   is complete. See concurrency notes in stdlib/exit.h where
+   __exit_funcs_lock is declared.  */
+bool __exit_funcs_done = false;
 
 /* Call all functions registered with `atexit' and `on_exit',
    in the reverse of the order in which they were registered
@@ -44,14 +49,32 @@ __run_exit_handlers (int status, struct exit_function_list **listp,
      the functions registered with `atexit' and `on_exit'. We call
      everyone on the list and use the status value in the last
      exit (). */
-  while (*listp != NULL)
+  while (true)
     {
-      struct exit_function_list *cur = *listp;
+      struct exit_function_list *cur;
+
+      __libc_lock_lock (__exit_funcs_lock);
+
+    restart:
+      cur = *listp;
+
+      if (cur == NULL)
+       {
+         /* Exit processing complete.  We will not allow any more
+            atexit/on_exit registrations.  */
+         __exit_funcs_done = true;
+         __libc_lock_unlock (__exit_funcs_lock);
+         break;
+       }
 
       while (cur->idx > 0)
        {
          const struct exit_function *const f =
            &cur->fns[--cur->idx];
+         const uint64_t new_exitfn_called = __new_exitfn_called;
+
+         /* Unlock the list while we call a foreign function.  */
+         __libc_lock_unlock (__exit_funcs_lock);
          switch (f->flavor)
            {
              void (*atfct) (void);
@@ -83,6 +106,13 @@ __run_exit_handlers (int status, struct exit_function_list **listp,
              cxafct (f->func.cxa.arg, status);
              break;
            }
+         /* Re-lock again before looking at global state.  */
+         __libc_lock_lock (__exit_funcs_lock);
+
+         if (__glibc_unlikely (new_exitfn_called != __new_exitfn_called))
+           /* The last exit function, or another thread, has registered
+              more exit functions.  Start the loop over.  */
+           goto restart;
        }
 
       *listp = cur->next;
@@ -90,6 +120,8 @@ __run_exit_handlers (int status, struct exit_function_list **listp,
        /* Don't free the last element in the chain, this is the statically
           allocate element.  */
        free (cur);
+
+      __libc_lock_unlock (__exit_funcs_lock);
     }
 
   if (run_list_atexit)
index 7f2e67924642655e470fdb3444f687703caa7443..dbf9f2d01f5542bd8d382e48b4d48ca15e7541f8 100644 (file)
@@ -20,6 +20,7 @@
 
 #include <stdbool.h>
 #include <stdint.h>
+#include <libc-lock.h>
 
 enum
 {
@@ -57,11 +58,27 @@ struct exit_function_list
     size_t idx;
     struct exit_function fns[32];
   };
+
 extern struct exit_function_list *__exit_funcs attribute_hidden;
 extern struct exit_function_list *__quick_exit_funcs attribute_hidden;
+extern uint64_t __new_exitfn_called attribute_hidden;
+
+/* True once all registered atexit/at_quick_exit/onexit handlers have been
+   called */
+extern bool __exit_funcs_done attribute_hidden;
+
+/* This lock protects __exit_funcs, __quick_exit_funcs, __exit_funcs_done
+   and __new_exitfn_called globals against simultaneous access from
+   atexit/on_exit/at_quick_exit in multiple threads, and also from
+   simultaneous access while another thread is in the middle of calling
+   exit handlers.  See BZ#14333.  Note: for lists, the entire list, and
+   each associated entry in the list, is protected for all access by this
+   lock.  */
+__libc_lock_define (extern, __exit_funcs_lock);
+
 
 extern struct exit_function *__new_exitfn (struct exit_function_list **listp);
-extern uint64_t __new_exitfn_called attribute_hidden;
+
 
 extern void __run_exit_handlers (int status,
                                 struct exit_function_list **listp,
index 83845e76d8b7be5f28c7e9ecb84056ec581f3e53..f4ede2b1a7469d78bef7a3f0182c27bb25e6548b 100644 (file)
 
 #include <stdlib.h>
 #include "exit.h"
-#include <atomic.h>
 #include <sysdep.h>
 
 /* Register a function to be called by exit.  */
 int
 __on_exit (void (*func) (int status, void *arg), void *arg)
 {
-  struct exit_function *new = __new_exitfn (&__exit_funcs);
+  struct exit_function *new;
+
+   __libc_lock_lock (__exit_funcs_lock);
+  new = __new_exitfn (&__exit_funcs);
 
   if (new == NULL)
-    return -1;
+    {
+      __libc_lock_unlock (__exit_funcs_lock);
+      return -1;
+    }
 
 #ifdef PTR_MANGLE
   PTR_MANGLE (func);
 #endif
   new->func.on.fn = func;
   new->func.on.arg = arg;
-  atomic_write_barrier ();
   new->flavor = ef_on;
+  __libc_lock_unlock (__exit_funcs_lock);
   return 0;
 }
 weak_alias (__on_exit, on_exit)
diff --git a/stdlib/test-at_quick_exit-race.c b/stdlib/test-at_quick_exit-race.c
new file mode 100644 (file)
index 0000000..f93fb85
--- /dev/null
@@ -0,0 +1,32 @@
+/* Bug 14333: a test for at_quick_exit/quick_exit race.
+
+   Copyright (C) 2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* This file must be run from within a directory called "stdlib".  */
+
+/* See stdlib/test-atexit-race-common.c for details on this test.  */
+
+#define CALL_ATEXIT at_quick_exit (&no_op)
+#define CALL_EXIT quick_exit (0)
+
+static void
+no_op (void)
+{
+}
+
+#include <stdlib/test-atexit-race-common.c>
diff --git a/stdlib/test-atexit-race.c b/stdlib/test-atexit-race.c
new file mode 100644 (file)
index 0000000..11e0244
--- /dev/null
@@ -0,0 +1,31 @@
+/* Bug 14333: a test for atexit/exit race.
+   Copyright (C) 2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* This file must be run from within a directory called "stdlib".  */
+
+/* See stdlib/test-atexit-race-common.c for details on this test.  */
+
+#define CALL_ATEXIT atexit (&no_op)
+#define CALL_EXIT exit (0)
+
+static void
+no_op (void)
+{
+}
+
+#include <stdlib/test-atexit-race-common.c>
diff --git a/stdlib/test-cxa_atexit-race.c b/stdlib/test-cxa_atexit-race.c
new file mode 100644 (file)
index 0000000..c695e71
--- /dev/null
@@ -0,0 +1,35 @@
+/* Bug 14333: a test for __cxa_atexit/exit race.
+   Copyright (C) 2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* This file must be run from within a directory called "stdlib".  */
+
+/* See stdlib/test-atexit-race-common.c for details on this test.  */
+
+#include <stdio.h>
+
+#define CALL_ATEXIT __cxa_atexit (&no_op, NULL, NULL)
+#define CALL_EXIT exit (0)
+
+int __cxa_atexit (void (*func) (void *), void *arg, void *d);
+
+static void
+no_op (void *ignored)
+{
+}
+
+#include <stdlib/test-atexit-race-common.c>
diff --git a/stdlib/test-on_exit-race.c b/stdlib/test-on_exit-race.c
new file mode 100644 (file)
index 0000000..3e67d3b
--- /dev/null
@@ -0,0 +1,31 @@
+/* Bug 14333: a test for on_exit/exit race.
+   Copyright (C) 2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* This file must be run from within a directory called "stdlib".  */
+
+/* See stdlib/test-atexit-race-common.c for details on this test.  */
+
+#define CALL_ATEXIT on_exit (&no_op, (void *) 0)
+#define CALL_EXIT exit (0)
+
+static void
+no_op (int exit_code, void *ignored)
+{
+}
+
+#include <stdlib/test-atexit-race-common.c>
This page took 0.090964 seconds and 5 git commands to generate.