From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS3215 2.6.0.0/16 X-Spam-Status: No, score=-5.4 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,MAILING_LIST_MULTI,NICE_REPLY_A, RCVD_IN_DNSWL_MED,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 6C0E11F8C6 for ; Tue, 7 Sep 2021 12:42:30 +0000 (UTC) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id EE7A6385EC52 for ; Tue, 7 Sep 2021 12:42:28 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org EE7A6385EC52 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1631018549; bh=hQK3+j8iZC5S3ocRDzY0QEOuIzNUEAh0aIqXyrHV7PI=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=GkHvaCSMTjBoE81++KtPXs44MrrBGpy8RCsCRmp93QB/kQlzlN8OaX2V82LyFLVnc 80Rhbe0NqAkcJplzjWpJYMSsHKpWIuPac3HlgaL9eE3rBtc+eAX8ify+HMLLscsWv9 GturnrqoupuD6AvoQfCLU7McdfjYN0g7ea9u8TRk= Received: from mail-qv1-xf30.google.com (mail-qv1-xf30.google.com [IPv6:2607:f8b0:4864:20::f30]) by sourceware.org (Postfix) with ESMTPS id 6350B385842D for ; Tue, 7 Sep 2021 12:42:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6350B385842D Received: by mail-qv1-xf30.google.com with SMTP id 93so5663747qva.7 for ; Tue, 07 Sep 2021 05:42:08 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=hQK3+j8iZC5S3ocRDzY0QEOuIzNUEAh0aIqXyrHV7PI=; b=ZNPOowcZqMVKJAoI3d8BDSq8Vv4zVUn0yspcbZ1zgails/HkwAVmZcyu4xMoHzK+1a Ly7fi1J9zFhQdJgvEy007NU04bX9Fd6g182hXLGcI+jRHHCxHt1RM14VVtAq1rl2y0xA 0wj3X4/6FG5qksqOmCoxeff5oZvujufCFCCsKp+fu11CkE4o3yXT0xApaXWtUt88+/Lr rVHDncXXQ0eUUzhOhxm3yrC31foJx5b1y1d0CV9seazNponln38FX1XVDsZsOmTat1oW yDWSwLTNteFu+OiCnKzID3DOlK22qHJ5hfV9WBipjhYtaqNvimS7hKtejFHP7/TkFnUw NQPg== X-Gm-Message-State: AOAM533DjajxZHT5WfT14S1wy375JYIPYv43sMCdbiy007A8KgKgLlPf W7s7BTr+CdFC3jcqz2uraHKdoziE/mKc0Q== X-Google-Smtp-Source: ABdhPJzW7vPO8ZiWFuGOoYDjcyiZuXCgj+ne9SbOYtL7Nf83/up5DlAqGCrovtHIpn02bcEEwe6nGg== X-Received: by 2002:a05:6214:238a:: with SMTP id fw10mr16918741qvb.27.1631018527878; Tue, 07 Sep 2021 05:42:07 -0700 (PDT) Received: from ?IPv6:2804:431:c7cb:733d:f360:5bf3:9813:d8f5? ([2804:431:c7cb:733d:f360:5bf3:9813:d8f5]) by smtp.gmail.com with ESMTPSA id s187sm8845125qkf.34.2021.09.07.05.42.06 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 07 Sep 2021 05:42:07 -0700 (PDT) Subject: Re: [PATCH v3 1/2] nptl: pthread_kill, pthread_cancel should not fail after exit (bug 19193) To: libc-alpha@sourceware.org, Florian Weimer References: Message-ID: Date: Tue, 7 Sep 2021 09:42:05 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: , From: Adhemerval Zanella via Libc-alpha Reply-To: Adhemerval Zanella Errors-To: libc-alpha-bounces+e=80x24.org@sourceware.org Sender: "Libc-alpha" 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 > --- > 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 > - . */ > - > -#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" 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 > + . */ > + > +/* This test verifies that pthread_kill returns 0 (and not ESRCH) for > + a thread that has exited on the kernel side. */ > + > +#include > +#include > +#include > + > +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 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 > + . */ > + > +/* This test verifies that pthread_kill returns 0 (and not ESRCH) for > + a thread that has exited on the kernel side. */ > + > +#include > +#include > +#include > +#include > + > +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 > Ok.