diff mbox series

stdlib: Allow concurrent quick_exit (BZ 31997)

Message ID 20240801165707.3977885-1-adhemerval.zanella@linaro.org
State Superseded
Headers show
Series stdlib: Allow concurrent quick_exit (BZ 31997) | expand

Commit Message

Adhemerval Zanella Aug. 1, 2024, 4:56 p.m. UTC
As for exit, also allows concurrent quick_exit to avoid race
conditions when it is called concurrently.  Since it uses the same
internal function as exit, the __exit_lock lock is moved to
__run_exit_handlers.  It also solved a potential concurrent when
calling exit and quick_exit concurrently.

The test case 'expected' is expanded to a value larger than the
minimum required by C/POSIX (32 entries) so at_quick_exit() will
require libc to allocate a new block.  This makes the test mre likely to
trigger concurrent issues (through free() at __run_exit_handlers)
if quick_exit() interacts with the at_quick_exit list concurrently.

This is also the latest interpretation of the Austin Ticket [1].

Checked on x86_64-linux-gnu.

[1] https://austingroupbugs.net/view.php?id=1845
---
 stdlib/Makefile                       |   1 +
 stdlib/exit.c                         |  18 +--
 stdlib/tst-concurrent-exit-skeleton.c | 160 ++++++++++++++++++++++++++
 stdlib/tst-concurrent-exit.c          | 141 +----------------------
 stdlib/tst-concurrent-quick_exit.c    |  22 ++++
 5 files changed, 196 insertions(+), 146 deletions(-)
 create mode 100644 stdlib/tst-concurrent-exit-skeleton.c
 create mode 100644 stdlib/tst-concurrent-quick_exit.c
diff mbox series

Patch

diff --git a/stdlib/Makefile b/stdlib/Makefile
index 7eec68b73a..a1be390e72 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -273,6 +273,7 @@  tests := \
   tst-bsearch \
   tst-bz20544 \
   tst-canon-bz26341 \
+  tst-concurrent-quick_exit \
   tst-concurrent-exit \
   tst-cxa_atexit \
   tst-environ \
diff --git a/stdlib/exit.c b/stdlib/exit.c
index bbaf138806..8d7e2e53d0 100644
--- a/stdlib/exit.c
+++ b/stdlib/exit.c
@@ -28,6 +28,13 @@ 
    __exit_funcs_lock is declared.  */
 bool __exit_funcs_done = false;
 
+/* The lock handles concurrent exit() and quick_exit(), even though the
+   C/POSIX standard states that calling exit() more than once is UB.  The
+   recursive lock allows atexit() handlers or destructors to call exit()
+   itself.  In this case, the  handler list execution will resume at the
+   point of the current handler.  */
+__libc_lock_define_initialized_recursive (static, __exit_lock)
+
 /* Call all functions registered with `atexit' and `on_exit',
    in the reverse of the order in which they were registered
    perform stdio cleanup, and terminate program execution with STATUS.  */
@@ -36,6 +43,9 @@  attribute_hidden
 __run_exit_handlers (int status, struct exit_function_list **listp,
 		     bool run_list_atexit, bool run_dtors)
 {
+  /* The exit should never return, so there is no need to unlock it.  */
+  __libc_lock_lock_recursive (__exit_lock);
+
   /* First, call the TLS destructors.  */
   if (run_dtors)
     call_function_static_weak (__call_tls_dtors);
@@ -132,17 +142,9 @@  __run_exit_handlers (int status, struct exit_function_list **listp,
 }
 
 
-/* The lock handles concurrent exit(), even though the C/POSIX standard states
-   that calling exit() more than once is UB.  The recursive lock allows
-   atexit() handlers or destructors to call exit() itself.  In this case, the
-   handler list execution will resume at the point of the current handler.  */
-__libc_lock_define_initialized_recursive (static, __exit_lock)
-
 void
 exit (int status)
 {
-  /* The exit should never return, so there is no need to unlock it.  */
-  __libc_lock_lock_recursive (__exit_lock);
   __run_exit_handlers (status, &__exit_funcs, true, true);
 }
 libc_hidden_def (exit)
diff --git a/stdlib/tst-concurrent-exit-skeleton.c b/stdlib/tst-concurrent-exit-skeleton.c
new file mode 100644
index 0000000000..cfd5140466
--- /dev/null
+++ b/stdlib/tst-concurrent-exit-skeleton.c
@@ -0,0 +1,160 @@ 
+/* Check if exit/quick_exit can be called concurrently by multiple threads.
+   Copyright (C) 2024 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
+   <https://www.gnu.org/licenses/>.  */
+
+#include <array_length.h>
+#include <stdlib.h>
+#include <support/check.h>
+#include <support/xthread.h>
+#include <stdio.h>
+#include <support/xunistd.h>
+#include <string.h>
+
+/* A value larger than the minimum required by C/POSIX (32), to trigger a
+   new block memory allocation.  */
+#define MAX_atexit 64
+
+static pthread_barrier_t barrier;
+
+static void *
+tf (void *closure)
+{
+  xpthread_barrier_wait (&barrier);
+  EXIT (0);
+
+  return NULL;
+}
+
+static const char expected[] = "00000000000000000000000000000000000"
+			       "00000000000000000000003021121130211";
+static char crumbs[sizeof (expected)];
+static int next_slot = 0;
+
+static void
+exit_with_flush (int code)
+{
+  fflush (stdout);
+  /* glibc allows recursive EXIT, the ATEXIT handlers execution will be
+     resumed from the where the previous EXIT was interrupted.  */
+  EXIT (code);
+}
+
+/* Take some time, so another thread potentially issue EXIT.  */
+#define SETUP_NANOSLEEP \
+  if (nanosleep (&(struct timespec) { .tv_sec = 0, .tv_nsec = 1000L },	\
+		 NULL) != 0)						\
+    FAIL_EXIT1 ("nanosleep: %m")
+
+static void
+fn0 (void)
+{
+  crumbs[next_slot++] = '0';
+  SETUP_NANOSLEEP;
+}
+
+static void
+fn1 (void)
+{
+  crumbs[next_slot++] = '1';
+  SETUP_NANOSLEEP;
+}
+
+static void
+fn2 (void)
+{
+  crumbs[next_slot++] = '2';
+  ATEXIT (fn1);
+  SETUP_NANOSLEEP;
+}
+
+static void
+fn3 (void)
+{
+  crumbs[next_slot++] = '3';
+  ATEXIT (fn2);
+  ATEXIT (fn0);
+  SETUP_NANOSLEEP;
+}
+
+static void
+fn_final (void)
+{
+  TEST_COMPARE_STRING (crumbs, expected);
+  exit_with_flush (0);
+}
+
+_Noreturn static void
+child (void)
+{
+  enum { nthreads = 8 };
+
+  xpthread_barrier_init (&barrier, NULL, nthreads + 1);
+
+  pthread_t thr[nthreads];
+  for (int i = 0; i < nthreads; i++)
+    thr[i] = xpthread_create (NULL, tf, NULL);
+
+  xpthread_barrier_wait (&barrier);
+
+  for (int i = 0; i < nthreads; i++)
+    {
+      pthread_join (thr[i], NULL);
+      /* It should not be reached, it means that thread did not exit for
+	 some reason.  */
+      support_record_failure ();
+    }
+
+  EXIT (2);
+}
+
+static int
+do_test (void)
+{
+  /* Register a large number of handler that will trigger a heap allocation
+     for the handle state.  On EXIT, each block will be freed after the
+     handle is processed.  */
+  int slots_remaining = MAX_atexit;
+
+  /* Register this first so it can verify expected order of the rest.  */
+  ATEXIT (fn_final); --slots_remaining;
+
+  TEST_VERIFY_EXIT (ATEXIT (fn1) == 0); --slots_remaining;
+  TEST_VERIFY_EXIT (ATEXIT (fn3) == 0); --slots_remaining;
+  TEST_VERIFY_EXIT (ATEXIT (fn1) == 0); --slots_remaining;
+  TEST_VERIFY_EXIT (ATEXIT (fn2) == 0); --slots_remaining;
+  TEST_VERIFY_EXIT (ATEXIT (fn1) == 0); --slots_remaining;
+  TEST_VERIFY_EXIT (ATEXIT (fn3) == 0); --slots_remaining;
+
+  while (slots_remaining > 0)
+    {
+      TEST_VERIFY_EXIT (ATEXIT (fn0) == 0); --slots_remaining;
+    }
+
+  pid_t pid = xfork ();
+  if (pid != 0)
+    {
+      int status;
+      xwaitpid (pid, &status, 0);
+      TEST_VERIFY (WIFEXITED (status));
+    }
+  else
+    child ();
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/stdlib/tst-concurrent-exit.c b/stdlib/tst-concurrent-exit.c
index 1141130f87..421c39d631 100644
--- a/stdlib/tst-concurrent-exit.c
+++ b/stdlib/tst-concurrent-exit.c
@@ -16,142 +16,7 @@ 
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#include <array_length.h>
-#include <stdlib.h>
-#include <support/check.h>
-#include <support/xthread.h>
-#include <stdio.h>
-#include <support/xunistd.h>
-#include <string.h>
+#define EXIT(__r)    exit (__r)
+#define ATEXIT(__f)  atexit (__f)
 
-#define MAX_atexit 32
-
-static pthread_barrier_t barrier;
-
-static void *
-tf (void *closure)
-{
-  xpthread_barrier_wait (&barrier);
-  exit (0);
-
-  return NULL;
-}
-
-static const char expected[] = "00000000000000000000000003021121130211";
-static char crumbs[sizeof (expected)];
-static int next_slot = 0;
-
-static void
-exit_with_flush (int code)
-{
-  fflush (stdout);
-  /* glibc allows recursive exit, the atexit handlers execution will be
-     resumed from the where the previous exit was interrupted.  */
-  exit (code);
-}
-
-/* Take some time, so another thread potentially issue exit.  */
-#define SETUP_NANOSLEEP \
-  if (nanosleep (&(struct timespec) { .tv_sec = 0, .tv_nsec = 1000L },	\
-		 NULL) != 0)						\
-    FAIL_EXIT1 ("nanosleep: %m")
-
-static void
-fn0 (void)
-{
-  crumbs[next_slot++] = '0';
-  SETUP_NANOSLEEP;
-}
-
-static void
-fn1 (void)
-{
-  crumbs[next_slot++] = '1';
-  SETUP_NANOSLEEP;
-}
-
-static void
-fn2 (void)
-{
-  crumbs[next_slot++] = '2';
-  atexit (fn1);
-  SETUP_NANOSLEEP;
-}
-
-static void
-fn3 (void)
-{
-  crumbs[next_slot++] = '3';
-  atexit (fn2);
-  atexit (fn0);
-  SETUP_NANOSLEEP;
-}
-
-static void
-fn_final (void)
-{
-  TEST_COMPARE_STRING (crumbs, expected);
-  exit_with_flush (0);
-}
-
-_Noreturn static void
-child (void)
-{
-  enum { nthreads = 8 };
-
-  xpthread_barrier_init (&barrier, NULL, nthreads + 1);
-
-  pthread_t thr[nthreads];
-  for (int i = 0; i < nthreads; i++)
-    thr[i] = xpthread_create (NULL, tf, NULL);
-
-  xpthread_barrier_wait (&barrier);
-
-  for (int i = 0; i < nthreads; i++)
-    {
-      pthread_join (thr[i], NULL);
-      /* It should not be reached, it means that thread did not exit for
-	 some reason.  */
-      support_record_failure ();
-    }
-
-  exit (2);
-}
-
-static int
-do_test (void)
-{
-  /* Register a large number of handler that will trigger a heap allocation
-     for the handle state.  On exit, each block will be freed after the
-     handle is processed.  */
-  int slots_remaining = MAX_atexit;
-
-  /* Register this first so it can verify expected order of the rest.  */
-  atexit (fn_final); --slots_remaining;
-
-  TEST_VERIFY_EXIT (atexit (fn1) == 0); --slots_remaining;
-  TEST_VERIFY_EXIT (atexit (fn3) == 0); --slots_remaining;
-  TEST_VERIFY_EXIT (atexit (fn1) == 0); --slots_remaining;
-  TEST_VERIFY_EXIT (atexit (fn2) == 0); --slots_remaining;
-  TEST_VERIFY_EXIT (atexit (fn1) == 0); --slots_remaining;
-  TEST_VERIFY_EXIT (atexit (fn3) == 0); --slots_remaining;
-
-  while (slots_remaining > 0)
-    {
-      TEST_VERIFY_EXIT (atexit (fn0) == 0); --slots_remaining;
-    }
-
-  pid_t pid = xfork ();
-  if (pid != 0)
-    {
-      int status;
-      xwaitpid (pid, &status, 0);
-      TEST_VERIFY (WIFEXITED (status));
-    }
-  else
-    child ();
-
-  return 0;
-}
-
-#include <support/test-driver.c>
+#include "tst-concurrent-exit-skeleton.c"
diff --git a/stdlib/tst-concurrent-quick_exit.c b/stdlib/tst-concurrent-quick_exit.c
new file mode 100644
index 0000000000..3f321668d6
--- /dev/null
+++ b/stdlib/tst-concurrent-quick_exit.c
@@ -0,0 +1,22 @@ 
+/* Check if quick_exit can be called concurrently by multiple threads.
+   Copyright (C) 2024 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
+   <https://www.gnu.org/licenses/>.  */
+
+#define EXIT(__r)    quick_exit (__r)
+#define ATEXIT(__f)  at_quick_exit (__f)
+
+#include "tst-concurrent-exit-skeleton.c"