Message ID | a565c298740c6ecfe9768dfe3e5e3bb8265213f2.1630931178.git.fweimer@redhat.com |
---|---|
State | New |
Headers | show |
Series | Fix pthread_cancel, pthread_kill race (bug 12889) | expand |
On 06/09/2021 09:33, Florian Weimer via Libc-alpha wrote: > This closes one remaining race condition related to bug 12889: if > the thread already exited on the kernel side, returning ESRCH > is not correct because that error is reserved for the thread IDs > (pthread_t values) whose lifetime has ended. In case of a > kernel-side exit and a valid thread ID, no signal needs to be sent > and cancellation does not have an effect, so just return 0. > > sysdeps/pthread/tst-kill4.c triggers undefined behavior and is > removed with this commit. LGTM, thanks. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > --- > nptl/pthread_cancel.c | 9 ++- > nptl/pthread_kill.c | 7 +- > sysdeps/pthread/Makefile | 5 +- > sysdeps/pthread/tst-kill4.c | 89 --------------------- > sysdeps/pthread/tst-pthread_cancel-exited.c | 45 +++++++++++ > sysdeps/pthread/tst-pthread_kill-exited.c | 46 +++++++++++ > 6 files changed, 106 insertions(+), 95 deletions(-) > delete mode 100644 sysdeps/pthread/tst-kill4.c > create mode 100644 sysdeps/pthread/tst-pthread_cancel-exited.c > create mode 100644 sysdeps/pthread/tst-pthread_kill-exited.c > > diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c > index 2bb523c0ec..a8aa3b3d15 100644 > --- a/nptl/pthread_cancel.c > +++ b/nptl/pthread_cancel.c > @@ -61,10 +61,11 @@ __pthread_cancel (pthread_t th) > { > volatile struct pthread *pd = (volatile struct pthread *) th; > > - /* Make sure the descriptor is valid. */ > - if (INVALID_TD_P (pd)) > - /* Not a valid thread handle. */ > - return ESRCH; > + if (pd->tid == 0) > + /* The thread has already exited on the kernel side. Its outcome > + (regular exit, other cancelation) has already been > + determined. */ > + return 0; > > static int init_sigcancel = 0; > if (atomic_load_relaxed (&init_sigcancel) == 0) Ok. > diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c > index f79a2b26fc..5d4c86f920 100644 > --- a/nptl/pthread_kill.c > +++ b/nptl/pthread_kill.c > @@ -46,7 +46,12 @@ __pthread_kill_internal (pthread_t threadid, int signo) > ? INTERNAL_SYSCALL_ERRNO (val) : 0); > } > else > - val = ESRCH; > + /* The kernel reports that the thread has exited. POSIX specifies > + the ESRCH error only for the case when the lifetime of a thread > + ID has ended, but calling pthread_kill on such a thread ID is > + undefined in glibc. Therefore, do not treat kernel thread exit > + as an error. */ > + val = 0; > > return val; > } Ok. > diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile > index 42f9fc5072..dedfa0d290 100644 > --- a/sysdeps/pthread/Makefile > +++ b/sysdeps/pthread/Makefile > @@ -89,7 +89,7 @@ tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \ > tst-join8 tst-join9 tst-join10 tst-join11 tst-join12 tst-join13 \ > tst-join14 tst-join15 \ > tst-key1 tst-key2 tst-key3 tst-key4 \ > - tst-kill1 tst-kill2 tst-kill3 tst-kill4 tst-kill5 tst-kill6 \ > + tst-kill1 tst-kill2 tst-kill3 tst-kill5 tst-kill6 \ > tst-locale1 tst-locale2 \ > tst-memstream \ > tst-mutex-errorcheck tst-mutex1 tst-mutex2 tst-mutex3 tst-mutex4 \ > @@ -118,6 +118,9 @@ tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \ > tst-unload \ > tst-unwind-thread \ > tst-pt-vfork1 tst-pt-vfork2 tst-vfork1x tst-vfork2x \ > + tst-pthread_cancel-exited \ > + tst-pthread_kill-exited \ > + # tests > > tests-time64 := \ > tst-abstime-time64 \ Ok. > diff --git a/sysdeps/pthread/tst-kill4.c b/sysdeps/pthread/tst-kill4.c > deleted file mode 100644 > index 222ceb724c..0000000000 > --- a/sysdeps/pthread/tst-kill4.c > +++ /dev/null > @@ -1,89 +0,0 @@ > -/* Copyright (C) 2003-2021 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 <errno.h> > -#include <pthread.h> > -#include <signal.h> > -#include <stdio.h> > -#include <stdlib.h> > -#include <unistd.h> > - > - > -static void * > -tf (void *a) > -{ > - return NULL; > -} > - > - > -int > -do_test (void) > -{ > - pthread_attr_t at; > - if (pthread_attr_init (&at) != 0) > - { > - puts ("attr_create failed"); > - exit (1); > - } > - > - /* Limit thread stack size, because if it is too large, pthread_join > - will free it immediately rather than put it into stack cache. */ > - if (pthread_attr_setstacksize (&at, 2 * 1024 * 1024) != 0) > - { > - puts ("setstacksize failed"); > - exit (1); > - } > - > - pthread_t th; > - if (pthread_create (&th, &at, tf, NULL) != 0) > - { > - puts ("create failed"); > - exit (1); > - } > - > - pthread_attr_destroy (&at); > - > - if (pthread_join (th, NULL) != 0) > - { > - puts ("join failed"); > - exit (1); > - } > - > - /* The following only works because we assume here something about > - the implementation. Namely, that the memory allocated for the > - thread descriptor is not going away, that the TID field is > - cleared and therefore the signal is sent to process 0, and that > - we can savely assume there is no other process with this ID at > - that time. */ > - int e = pthread_kill (th, 0); > - if (e == 0) > - { > - puts ("pthread_kill succeeded"); > - exit (1); > - } > - if (e != ESRCH) > - { > - puts ("pthread_kill didn't return ESRCH"); > - exit (1); > - } > - > - return 0; > -} > - > - > -#define TEST_FUNCTION do_test () > -#include "../test-skeleton.c" Ok. > diff --git a/sysdeps/pthread/tst-pthread_cancel-exited.c b/sysdeps/pthread/tst-pthread_cancel-exited.c > new file mode 100644 > index 0000000000..811c9bee07 > --- /dev/null > +++ b/sysdeps/pthread/tst-pthread_cancel-exited.c > @@ -0,0 +1,45 @@ > +/* Test that pthread_kill succeeds for an exited thread. > + Copyright (C) 2021 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/>. */ > + > +/* This test verifies that pthread_kill returns 0 (and not ESRCH) for > + a thread that has exited on the kernel side. */ > + > +#include <stddef.h> > +#include <support/support.h> > +#include <support/xthread.h> > + > +static void * > +noop_thread (void *closure) > +{ > + return NULL; > +} > + > +static int > +do_test (void) > +{ > + pthread_t thr = xpthread_create (NULL, noop_thread, NULL); > + > + support_wait_for_thread_exit (); > + > + xpthread_cancel (thr); > + xpthread_join (thr); > + > + return 0; > +} > + > +#include <support/test-driver.c> Ok. > diff --git a/sysdeps/pthread/tst-pthread_kill-exited.c b/sysdeps/pthread/tst-pthread_kill-exited.c > new file mode 100644 > index 0000000000..7575fb6d58 > --- /dev/null > +++ b/sysdeps/pthread/tst-pthread_kill-exited.c > @@ -0,0 +1,46 @@ > +/* Test that pthread_kill succeeds for an exited thread. > + Copyright (C) 2021 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/>. */ > + > +/* This test verifies that pthread_kill returns 0 (and not ESRCH) for > + a thread that has exited on the kernel side. */ > + > +#include <signal.h> > +#include <stddef.h> > +#include <support/support.h> > +#include <support/xthread.h> > + > +static void * > +noop_thread (void *closure) > +{ > + return NULL; > +} > + > +static int > +do_test (void) > +{ > + pthread_t thr = xpthread_create (NULL, noop_thread, NULL); > + > + support_wait_for_thread_exit (); > + > + xpthread_kill (thr, SIGUSR1); > + xpthread_join (thr); > + > + return 0; > +} > + > +#include <support/test-driver.c> > Ok.
diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c index 2bb523c0ec..a8aa3b3d15 100644 --- a/nptl/pthread_cancel.c +++ b/nptl/pthread_cancel.c @@ -61,10 +61,11 @@ __pthread_cancel (pthread_t th) { volatile struct pthread *pd = (volatile struct pthread *) th; - /* Make sure the descriptor is valid. */ - if (INVALID_TD_P (pd)) - /* Not a valid thread handle. */ - return ESRCH; + if (pd->tid == 0) + /* The thread has already exited on the kernel side. Its outcome + (regular exit, other cancelation) has already been + determined. */ + return 0; static int init_sigcancel = 0; if (atomic_load_relaxed (&init_sigcancel) == 0) diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c index f79a2b26fc..5d4c86f920 100644 --- a/nptl/pthread_kill.c +++ b/nptl/pthread_kill.c @@ -46,7 +46,12 @@ __pthread_kill_internal (pthread_t threadid, int signo) ? INTERNAL_SYSCALL_ERRNO (val) : 0); } else - val = ESRCH; + /* The kernel reports that the thread has exited. POSIX specifies + the ESRCH error only for the case when the lifetime of a thread + ID has ended, but calling pthread_kill on such a thread ID is + undefined in glibc. Therefore, do not treat kernel thread exit + as an error. */ + val = 0; return val; } diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile index 42f9fc5072..dedfa0d290 100644 --- a/sysdeps/pthread/Makefile +++ b/sysdeps/pthread/Makefile @@ -89,7 +89,7 @@ tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \ tst-join8 tst-join9 tst-join10 tst-join11 tst-join12 tst-join13 \ tst-join14 tst-join15 \ tst-key1 tst-key2 tst-key3 tst-key4 \ - tst-kill1 tst-kill2 tst-kill3 tst-kill4 tst-kill5 tst-kill6 \ + tst-kill1 tst-kill2 tst-kill3 tst-kill5 tst-kill6 \ tst-locale1 tst-locale2 \ tst-memstream \ tst-mutex-errorcheck tst-mutex1 tst-mutex2 tst-mutex3 tst-mutex4 \ @@ -118,6 +118,9 @@ tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \ tst-unload \ tst-unwind-thread \ tst-pt-vfork1 tst-pt-vfork2 tst-vfork1x tst-vfork2x \ + tst-pthread_cancel-exited \ + tst-pthread_kill-exited \ + # tests tests-time64 := \ tst-abstime-time64 \ diff --git a/sysdeps/pthread/tst-kill4.c b/sysdeps/pthread/tst-kill4.c deleted file mode 100644 index 222ceb724c..0000000000 --- a/sysdeps/pthread/tst-kill4.c +++ /dev/null @@ -1,89 +0,0 @@ -/* Copyright (C) 2003-2021 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 <errno.h> -#include <pthread.h> -#include <signal.h> -#include <stdio.h> -#include <stdlib.h> -#include <unistd.h> - - -static void * -tf (void *a) -{ - return NULL; -} - - -int -do_test (void) -{ - pthread_attr_t at; - if (pthread_attr_init (&at) != 0) - { - puts ("attr_create failed"); - exit (1); - } - - /* Limit thread stack size, because if it is too large, pthread_join - will free it immediately rather than put it into stack cache. */ - if (pthread_attr_setstacksize (&at, 2 * 1024 * 1024) != 0) - { - puts ("setstacksize failed"); - exit (1); - } - - pthread_t th; - if (pthread_create (&th, &at, tf, NULL) != 0) - { - puts ("create failed"); - exit (1); - } - - pthread_attr_destroy (&at); - - if (pthread_join (th, NULL) != 0) - { - puts ("join failed"); - exit (1); - } - - /* The following only works because we assume here something about - the implementation. Namely, that the memory allocated for the - thread descriptor is not going away, that the TID field is - cleared and therefore the signal is sent to process 0, and that - we can savely assume there is no other process with this ID at - that time. */ - int e = pthread_kill (th, 0); - if (e == 0) - { - puts ("pthread_kill succeeded"); - exit (1); - } - if (e != ESRCH) - { - puts ("pthread_kill didn't return ESRCH"); - exit (1); - } - - return 0; -} - - -#define TEST_FUNCTION do_test () -#include "../test-skeleton.c" diff --git a/sysdeps/pthread/tst-pthread_cancel-exited.c b/sysdeps/pthread/tst-pthread_cancel-exited.c new file mode 100644 index 0000000000..811c9bee07 --- /dev/null +++ b/sysdeps/pthread/tst-pthread_cancel-exited.c @@ -0,0 +1,45 @@ +/* Test that pthread_kill succeeds for an exited thread. + Copyright (C) 2021 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/>. */ + +/* This test verifies that pthread_kill returns 0 (and not ESRCH) for + a thread that has exited on the kernel side. */ + +#include <stddef.h> +#include <support/support.h> +#include <support/xthread.h> + +static void * +noop_thread (void *closure) +{ + return NULL; +} + +static int +do_test (void) +{ + pthread_t thr = xpthread_create (NULL, noop_thread, NULL); + + support_wait_for_thread_exit (); + + xpthread_cancel (thr); + xpthread_join (thr); + + return 0; +} + +#include <support/test-driver.c> diff --git a/sysdeps/pthread/tst-pthread_kill-exited.c b/sysdeps/pthread/tst-pthread_kill-exited.c new file mode 100644 index 0000000000..7575fb6d58 --- /dev/null +++ b/sysdeps/pthread/tst-pthread_kill-exited.c @@ -0,0 +1,46 @@ +/* Test that pthread_kill succeeds for an exited thread. + Copyright (C) 2021 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/>. */ + +/* This test verifies that pthread_kill returns 0 (and not ESRCH) for + a thread that has exited on the kernel side. */ + +#include <signal.h> +#include <stddef.h> +#include <support/support.h> +#include <support/xthread.h> + +static void * +noop_thread (void *closure) +{ + return NULL; +} + +static int +do_test (void) +{ + pthread_t thr = xpthread_create (NULL, noop_thread, NULL); + + support_wait_for_thread_exit (); + + xpthread_kill (thr, SIGUSR1); + xpthread_join (thr); + + return 0; +} + +#include <support/test-driver.c>