| Message ID | 20240805142758.806079-1-adhemerval.zanella@linaro.org |
|---|---|
| State | Accepted |
| Commit | c6af8a9a3ce137a9704825d173be22a2b2d9cb49 |
| Headers | show |
| Series | [v2] stdlib: Allow concurrent quick_exit (BZ 31997) | expand |
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.
On 05/08/24 13:06, Carlos O'Donell wrote: > 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? We have in fact some issue with concurrent dlclose / exit: 1. dclose might unmmap the object during exit __exit_func list transversal, and this might trigger invalid memory accesses [1]. I think this falls to the same issues of the under-specification of concurrent exit along other functions that access the same underlying data structure that holds the atexit/quick_exit/__cxa_atexit handlers. This leads to different strategies on how to avoid such issues [2] (which afaik is not only presented on glibc, but on some other libc implementations). I think a cleanly solution would to if a dlopen shared object contains a binding to __cxa_finalize to mark if with DF_1_NODELETE. This would change the expected semantic slight, but at least would avoid a whole set of subtle bugs. 2. This seems to be also a small memory leak where __cxa_finalize does not free of the allocated chain (I just noted by reading the code, I need to check if it where the case indeed). 3. Concurrent __cxa_finalize does not synchronize with exit/quick_exit. The 'removal' on __cxa_finalize is to avoid calling termination functions more than once, and I think it is required. [1] https://sourceware.org/bugzilla/show_bug.cgi?id=31285 [2] https://maskray.me/blog/2024-03-17-c++-exit-time-destructors > > 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? There is a subtle issue where doing this would be tricky: _dl_fini, registered at process startup (csu/libc-start.c:312) tries to lock the GL(dl_load_lock) lock (elf/dl-fini.c:49). And dlclose also takes the lock (elf/dl-close.c:763), and thus trying to take the __exit_lock on __cxa_finalize could easily deadlock the process. I still don't have a solution to this. > >> 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> Thanks. > >> >> 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. >
The new test fails to link for Hurd (pthread_create not in libc).
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 \ 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) + /* 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"