From patchwork Mon Aug 23 19:50:37 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 44757 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 9106C385781B for ; Mon, 23 Aug 2021 20:00:27 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 9106C385781B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1629748827; bh=b8in3puBZ50zKZaI6XJJdaHoSDG1ikVvJYIdyrlleb0=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=ARtK2Y+SFopuRmQIEdzIVv+92Fk/3juIABBOQRrl3Es2pnTSNK1v+u65SWbwWiNNp dJ6yJ3S8d6axHUQaOqU+Iuvzie5EirVP+AxQ4NQ9NiLaDO78RaklbdR2uUV6VGk0C/ CMnzp4JjfmldwSSXv/rmXnjEWHHN6Xf5FLx72QPc= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail-qk1-x730.google.com (mail-qk1-x730.google.com [IPv6:2607:f8b0:4864:20::730]) by sourceware.org (Postfix) with ESMTPS id 740583857C52 for ; Mon, 23 Aug 2021 19:52:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 740583857C52 Received: by mail-qk1-x730.google.com with SMTP id bk29so14789213qkb.8 for ; Mon, 23 Aug 2021 12:52:11 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=b8in3puBZ50zKZaI6XJJdaHoSDG1ikVvJYIdyrlleb0=; b=oqVEvzlWl2IiJwamlX0yPajGlO8m+5GH7GkDDHhHq1J7Lq5WQd8cHsGM3NBg8n4fb2 ILuHf8YHPe6CKG+SaEdhK0+MT5p1+04rCyHkiur13m3hzMEMi7I55aImQmJ98qG7g5Dc Hcmag0E9Ld74Q2878sfnBQauee6SXL6YLqPHFjb2sEGgYFZdSdE+BBIodL0tDLydU/fn wRCfMSC53mMrQQfCxm+tE6Bo8g5g1qEAXgfxLQFmDS8dzW0a46PO83ampsXhI1g65N7Q UaBukJUhsgtcbp+Tc1ZpMZvvneRF/RyU4YeBlU9qOfz+sHw2TtsoXwR8kjie+wsZyDwK sJbw== X-Gm-Message-State: AOAM532xv5sWj1PMAXiDy4qqMXbJt17BieC6Zr3wrDMmCuOZ2ZQFpXkv rMvsZpQxIBqPb3aIL8MdEoVTp07+/10uwA== X-Google-Smtp-Source: ABdhPJwuSZmS49h6N9DSqpR/w59eHilLATzi0I02elOMmho0338lDBsc7C9JQVR7IKPDy0yya5UDyA== X-Received: by 2002:a05:620a:4495:: with SMTP id x21mr22110361qkp.378.1629748330814; Mon, 23 Aug 2021 12:52:10 -0700 (PDT) Received: from birita.. ([2804:431:c7ca:cd83:c38b:b50d:5d9a:43d4]) by smtp.gmail.com with ESMTPSA id s10sm9210935qko.134.2021.08.23.12.52.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 23 Aug 2021 12:52:10 -0700 (PDT) To: libc-alpha@sourceware.org Subject: [PATCH v2 09/19] nptl: Fix race between pthread_kill and thread exit (bug 12889) Date: Mon, 23 Aug 2021 16:50:37 -0300 Message-Id: <20210823195047.543237-10-adhemerval.zanella@linaro.org> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20210823195047.543237-1-adhemerval.zanella@linaro.org> References: <20210823195047.543237-1-adhemerval.zanella@linaro.org> MIME-Version: 1.0 X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Adhemerval Zanella via Libc-alpha From: Adhemerval Zanella Netto Reply-To: Adhemerval Zanella Cc: Florian Weimer Errors-To: libc-alpha-bounces+patchwork=sourceware.org@sourceware.org Sender: "Libc-alpha" From: Florian Weimer Now that the 'tid' pthread member is not used to advertise the thread lifetime through set_tid_address syscall, we can use a simple lock to guarantee it is valid on pthread_kill(). This patch adds a new lock, tidlock, which is used to synchronize the pthread 'tid' field access. Also, there is no need to access the 'tid' member using atomic operations. Checked on x86_64-linux-gnu. Co-authored-by: Adhemerval Zanella --- nptl/allocatestack.c | 1 + nptl/descr.h | 4 + nptl/pthread_create.c | 7 ++ nptl/pthread_kill.c | 14 ++- sysdeps/pthread/Makefile | 2 + sysdeps/pthread/tst-kill4.c | 90 -------------- .../pthread/tst-pthread_cancel-select-loop.c | 87 ++++++++++++++ sysdeps/pthread/tst-pthread_kill-exiting.c | 110 ++++++++++++++++++ 8 files changed, 221 insertions(+), 94 deletions(-) delete mode 100644 sysdeps/pthread/tst-kill4.c create mode 100644 sysdeps/pthread/tst-pthread_cancel-select-loop.c create mode 100644 sysdeps/pthread/tst-pthread_kill-exiting.c diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c index a2a1fa7b81..63e6fe9d60 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->tidlock = LLL_LOCK_INITIALIZER; /* No pending event. */ result->nextevent = NULL; diff --git a/nptl/descr.h b/nptl/descr.h index 1bfa2b9b52..f9bb58d7cc 100644 --- a/nptl/descr.h +++ b/nptl/descr.h @@ -181,6 +181,10 @@ struct pthread /* Thread ID set by the kernel with CLONE_PARENT_SETTID. */ pid_t tid; + /* Lock to synchronize access to TID value (used on pthread_kill and thread + exit). */ + int tidlock; + /* Ununsed. */ pid_t pid_ununsed; diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c index b354f62995..a0f3ddf5fa 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->tidlock, 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->tidlock, 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 5d4c86f920..63fe8c44ca 100644 --- a/nptl/pthread_kill.c +++ b/nptl/pthread_kill.c @@ -26,15 +26,18 @@ __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_cancel + (which should be async-signal-safe). */ + sigset_t oldmask; + __libc_signal_block_all (&oldmask); + lll_lock (pd->tidlock, 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). */ tid = INLINE_SYSCALL_CALL (gettid); else - /* Force load of pd->tid into local variable or register. Otherwise - if a thread exits between ESRCH test and tgkill, we might return - EINVAL, because pd->tid would be cleared by the kernel. */ - tid = atomic_forced_read (pd->tid); + tid = pd->tid; int val; if (__glibc_likely (tid > 0)) @@ -53,6 +56,9 @@ __pthread_kill_internal (pthread_t threadid, int signo) as an error. */ val = 0; + lll_unlock (pd->tidlock, 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