From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 1791) id C066938A880A; Fri, 20 Aug 2021 17:56:03 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org C066938A880A Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: Adhemerval Zanella To: glibc-cvs@sourceware.org Subject: [glibc/azanella/pthread-multiple-fixes] nptl: Fix race between pthread_kill and thread exit (bug 12889) X-Act-Checkin: glibc X-Git-Author: Florian Weimer X-Git-Refname: refs/heads/azanella/pthread-multiple-fixes X-Git-Oldrev: 6ace783138d9d54bffdea8be3c0d06463bb976ea X-Git-Newrev: bb94df8eaba1af636bd6af7ec9142bc3e0e25868 Message-Id: <20210820175603.C066938A880A@sourceware.org> Date: Fri, 20 Aug 2021 17:56:03 +0000 (GMT) X-BeenThere: glibc-cvs@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Glibc-cvs mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 20 Aug 2021 17:56:03 -0000 https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=bb94df8eaba1af636bd6af7ec9142bc3e0e25868 commit bb94df8eaba1af636bd6af7ec9142bc3e0e25868 Author: Florian Weimer Date: Fri Aug 20 13:45:07 2021 -0300 nptl: Fix race between pthread_kill and thread exit (bug 12889) A new lock is used to synchronize between pthread_kill and thread exit. Checked on x86_64-linux-gnu. Co-authored-by: Adhemerval Zanella Diff: --- nptl/allocatestack.c | 1 + nptl/descr.h | 3 + nptl/pthread_create.c | 7 ++ nptl/pthread_kill.c | 9 ++ sysdeps/pthread/Makefile | 2 + sysdeps/pthread/tst-kill4.c | 90 ------------------- sysdeps/pthread/tst-pthread_cancel-select-loop.c | 87 ++++++++++++++++++ sysdeps/pthread/tst-pthread_kill-exiting.c | 110 +++++++++++++++++++++++ 8 files changed, 219 insertions(+), 90 deletions(-) diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c index a2a1fa7b81..84e8972eab 100644 --- a/nptl/allocatestack.c +++ b/nptl/allocatestack.c @@ -124,6 +124,7 @@ get_cached_stack (size_t *sizep, void **memp) result->canceltype = PTHREAD_CANCEL_DEFERRED; result->cleanup = NULL; result->setup_failed = 0; + result->killlock = 0; /* No pending event. */ result->nextevent = NULL; diff --git a/nptl/descr.h b/nptl/descr.h index 1bfa2b9b52..d69e2a3df4 100644 --- a/nptl/descr.h +++ b/nptl/descr.h @@ -327,6 +327,9 @@ struct pthread /* Lock to synchronize access to the descriptor. */ int lock; + /* Lock to synchronize pthread_kill and thread exit. */ + int killlock; + /* Indicate whether thread is supposed to change XID. */ int setxid_flag; /* Lock for synchronizing setxid calls. */ diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c index b354f62995..5b51f54190 100644 --- a/nptl/pthread_create.c +++ b/nptl/pthread_create.c @@ -531,6 +531,12 @@ start_thread (void *arg) /* This was the last thread. */ exit (0); + /* This prevents sending a signal from this thread to itself during its + final stages. This must come after the exit call above because + atexit handlers must not run with signals blocked. */ + __libc_signal_block_all (NULL); + lll_lock (pd->killlock, LLL_PRIVATE); + if (!pd->user_stack) advise_stack_range (pd->stackblock, pd->stackblock_size, (uintptr_t) pd, pd->guardsize); @@ -555,6 +561,7 @@ start_thread (void *arg) __nptl_free_tcb (pd); pd->tid = 0; + lll_unlock (pd->killlock, LLL_PRIVATE); out: /* We cannot call '_exit' here. '_exit' will terminate the process. diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c index f935b1fb3e..23f3f03b6e 100644 --- a/nptl/pthread_kill.c +++ b/nptl/pthread_kill.c @@ -26,6 +26,12 @@ __pthread_kill_internal (pthread_t threadid, int signo) pid_t tid; struct pthread *pd = (struct pthread *) threadid; + /* Block all signal, since the lock is recursive and used on pthread_cnacel + (which should be async-signal-safe). */ + sigset_t oldmask; + __libc_signal_block_all (&oldmask); + lll_lock (pd->killlock, LLL_PRIVATE); + if (pd == THREAD_SELF) /* It is a special case to handle raise() implementation after a vfork call (which does not update the PD tid field). */ @@ -48,6 +54,9 @@ __pthread_kill_internal (pthread_t threadid, int signo) else val = 0; + lll_unlock (pd->killlock, LLL_PRIVATE); + __libc_signal_restore_set (&oldmask); + return val; } diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile index dedfa0d290..48dba717a1 100644 --- a/sysdeps/pthread/Makefile +++ b/sysdeps/pthread/Makefile @@ -119,7 +119,9 @@ tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \ tst-unwind-thread \ tst-pt-vfork1 tst-pt-vfork2 tst-vfork1x tst-vfork2x \ tst-pthread_cancel-exited \ + tst-pthread_cancel-select-loop \ tst-pthread_kill-exited \ + tst-pthread_kill-exiting \ # tests tests-time64 := \ diff --git a/sysdeps/pthread/tst-kill4.c b/sysdeps/pthread/tst-kill4.c deleted file mode 100644 index 9563939792..0000000000 --- a/sysdeps/pthread/tst-kill4.c +++ /dev/null @@ -1,90 +0,0 @@ -/* Copyright (C) 2003-2021 Free Software Foundation, Inc. - This file is part of the GNU C Library. - Contributed by Ulrich Drepper , 2003. - - 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 - . */ - -#include -#include -#include -#include -#include -#include - - -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-select-loop.c b/sysdeps/pthread/tst-pthread_cancel-select-loop.c new file mode 100644 index 0000000000..a62087589c --- /dev/null +++ b/sysdeps/pthread/tst-pthread_cancel-select-loop.c @@ -0,0 +1,87 @@ +/* Test that pthread_cancel succeeds during thread exit. + 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 + . */ + +/* This test tries to trigger an internal race condition in + pthread_cancel, where the cancellation signal is sent after the + thread has begun the cancellation process. This can result in a + spurious ESRCH error. For the original bug 12889, the window is + quite small, so the bug was not reproduced in every run. */ + +#include +#include +#include +#include +#include +#include +#include + +/* Set to true by timeout_thread_function when the test should + terminate. */ +static bool timeout; + +static void * +timeout_thread_function (void *unused) +{ + usleep (5 * 1000 * 1000); + __atomic_store_n (&timeout, true, __ATOMIC_RELAXED); + return NULL; +} + +/* Used for blocking the select function below. */ +static int pipe_fds[2]; + +static void * +canceled_thread_function (void *unused) +{ + while (true) + { + fd_set rfs; + fd_set wfs; + fd_set efs; + FD_ZERO (&rfs); + FD_ZERO (&wfs); + FD_ZERO (&efs); + FD_SET (pipe_fds[0], &rfs); + + /* If the cancellation request is recognized early, the thread + begins exiting while the cancellation signal arrives. */ + select (FD_SETSIZE, &rfs, &wfs, &efs, NULL); + } + return NULL; +} + +static int +do_test (void) +{ + xpipe (pipe_fds); + pthread_t thr_timeout = xpthread_create (NULL, timeout_thread_function, NULL); + + while (!__atomic_load_n (&timeout, __ATOMIC_RELAXED)) + { + pthread_t thr = xpthread_create (NULL, canceled_thread_function, NULL); + xpthread_cancel (thr); + TEST_VERIFY (xpthread_join (thr) == PTHREAD_CANCELED); + } + + xpthread_join (thr_timeout); + xclose (pipe_fds[0]); + xclose (pipe_fds[1]); + return 0; +} + +#include diff --git a/sysdeps/pthread/tst-pthread_kill-exiting.c b/sysdeps/pthread/tst-pthread_kill-exiting.c new file mode 100644 index 0000000000..15550d2310 --- /dev/null +++ b/sysdeps/pthread/tst-pthread_kill-exiting.c @@ -0,0 +1,110 @@ +/* Test that pthread_kill succeeds during thread exit. + 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 + . */ + +/* This test verifies that pthread_kill for a thread that is exiting + succeeds (with or without actually delivering the signal). */ + +#include +#include +#include +#include +#include +#include + +/* Set to true by timeout_thread_function when the test should + terminate. */ +static bool timeout; + +static void * +timeout_thread_function (void *unused) +{ + usleep (1000 * 1000); + __atomic_store_n (&timeout, true, __ATOMIC_RELAXED); + return NULL; +} + +/* Used to synchronize the sending threads with the target thread and + main thread. */ +static pthread_barrier_t barrier_1; +static pthread_barrier_t barrier_2; + +/* The target thread to which signals are to be sent. */ +static pthread_t target_thread; + +static void * +sender_thread_function (void *unused) +{ + while (!__atomic_load_n (&timeout, __ATOMIC_RELAXED)) + { + /* Wait until target_thread has been initialized. The target + thread and main thread participate in this barrier. */ + xpthread_barrier_wait (&barrier_1); + + xpthread_kill (target_thread, SIGUSR1); + + /* Communicate that the signal has been sent. The main thread + participates in this barrier. */ + xpthread_barrier_wait (&barrier_2); + } + return NULL; +} + +static void * +target_thread_function (void *unused) +{ + target_thread = pthread_self (); + xpthread_barrier_wait (&barrier_1); + return NULL; +} + +static int +do_test (void) +{ + xsignal (SIGUSR1, SIG_IGN); + + pthread_t thr_timeout = xpthread_create (NULL, timeout_thread_function, NULL); + + pthread_t threads[4]; + xpthread_barrier_init (&barrier_1, NULL, array_length (threads) + 2); + xpthread_barrier_init (&barrier_2, NULL, array_length (threads) + 1); + + for (int i = 0; i < array_length (threads); ++i) + threads[i] = xpthread_create (NULL, sender_thread_function, NULL); + + while (!__atomic_load_n (&timeout, __ATOMIC_RELAXED)) + { + xpthread_create (NULL, target_thread_function, NULL); + + /* Wait for the target thread to be set up and signal sending to + start. */ + xpthread_barrier_wait (&barrier_1); + + /* Wait for signal sending to complete. */ + xpthread_barrier_wait (&barrier_2); + + xpthread_join (target_thread); + } + + for (int i = 0; i < array_length (threads); ++i) + xpthread_join (threads[i]); + xpthread_join (thr_timeout); + + return 0; +} + +#include