[PATCH v2] stdlib: Allow concurrent quick_exit (BZ 31997)
Carlos O'Donell
carlos@redhat.com
Mon Aug 5 16:06:04 GMT 2024
On 8/5/24 10:27 AM, Adhemerval Zanella wrote:
> 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.
Do we also have the same problem with dlclose() which is going to possibly remove
an entire object along with registered functions?
Should dlclose() take the same recursive lock such that a developer either sees
the dlclose() happen first, followed by exit/quick_exit, or not see it at all?
> 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].
LGTM. Though we may need a follow-up dlclose change.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
>
> Checked on x86_64-linux-gnu.
>
> [1] https://austingroupbugs.net/view.php?id=1845
> --
> Changes from v1:
> - Fixed lint-makefiles issue.
> ---
> 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 --git a/stdlib/Makefile b/stdlib/Makefile
> index f659c38feb..043a452f72 100644
> --- a/stdlib/Makefile
> +++ b/stdlib/Makefile
> @@ -274,6 +274,7 @@ tests := \
> tst-bz20544 \
> tst-canon-bz26341 \
> tst-concurrent-exit \
> + tst-concurrent-quick_exit \
OK.
> tst-cxa_atexit \
> tst-environ \
> tst-getrandom \
> 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)
OK. Moved up from below and added note about quick_exit.
> +
> /* 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);
OK.
> +
> /* 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)
> -
OK.
> void
> exit (int status)
> {
> - /* The exit should never return, so there is no need to unlock it. */
> - __libc_lock_lock_recursive (__exit_lock);
OK. Moved into the handling of exit handlers.
> __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;
> + }
OK.
> +
> + 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"
OK. Cleanup.
> 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"
OK.
--
Cheers,
Carlos.
More information about the Libc-alpha
mailing list