From patchwork Tue Dec 23 06:59:40 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alexandre Oliva X-Patchwork-Id: 423597 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 7066E1400B7 for ; Tue, 23 Dec 2014 18:00:08 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:cc:subject:references:date:in-reply-to :message-id:mime-version:content-type; q=dns; s=default; b=u0gTi eU9qBTP6kJoaCyyTVuYMmqSblPi6T7QblVRrrlmgyADoVwpYom6wDE5t00kPcsbT s40Sr3K39ivEFTcA4tHF4Mz2lrLk6mA+f0liOj/ZRIMngjLc5k80yGbJEGe+W3NB R3pDq4ajhjt6f5wLlXL0n/wzhhZ1eh0LKumPbM= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:cc:subject:references:date:in-reply-to :message-id:mime-version:content-type; s=default; bh=TMHMPUt65h6 PY+XApjKDV9cyOKc=; b=Xz1CjJB4Zvomz9sEzHttBwFaNe+r6/qDECFHIMlRh5k 5AG4knpyFzcBMCciDW8U4EFkzrDuTIwhpovCoCdxGC2fCCtD356zB3jH99Mb4MEl 4nFAE1VY5fjIMQ9SUblCAvMb+BKRR9cX71Dg8EeiNhVyeaK/Rka3Lo14u0RdaceQ = Received: (qmail 5300 invoked by alias); 23 Dec 2014 07:00:00 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 5285 invoked by uid 89); 23 Dec 2014 06:59:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.9 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com From: Alexandre Oliva To: MaShimiao Cc: , Subject: Re: question about which sleep is noted in manual References: <547ED48B.2060509@cn.fujitsu.com> <5487EAF0.70305@cn.fujitsu.com> <549787C1.9020807@cn.fujitsu.com> Date: Tue, 23 Dec 2014 04:59:40 -0200 In-Reply-To: (Alexandre Oliva's message of "Mon, 22 Dec 2014 16:24:50 -0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 On Dec 22, 2014, Alexandre Oliva wrote: > On Dec 22, 2014, MaShimiao wrote: >> 2. if funciont __sleep blocks SIGCHLD, no matter nanosleep() is cancelled or >> not, __seleep will restore the original signal mask before return. > If the thread is canceled, we won't take the normal execution path after > the nanosleep call; we'll only run cleanup handlers set up earlier in > the call chain, until the thread is terminated. > You may want to try it yourself: install a cleanup handler that probes > SIGCHLD in the signal mask, in a thread that calls sleep, and then > cancel the thread while sleep is running. You shouldn't see SIGCHLD > blocked in your cleanup handler, but without the proposed patch, you > will. Or use the testcase in the first (updated) patch below ;-) It fixes the missing include you reported in another message, too. Thanks! As for the problem you noticed in the other patch, s/arg/&set/ fixes it, but I won't repost it now to avoid confusion. This one is actually tested (sorry, I forgot to mention I hadn't tested the earlier patchset due to the -Werror build failures, now fixed in my tree with the patch I posted earlier). I have also verified that, without the change to sleep.c, the test fails. I don't think this is user-visible enough to deserve a bug report and a mention in NEWS, but if anyone disagrees with this assessment, please let me now and I'll file the bug report and adjust the patch. Ok to install? for ChangeLog * nptl/Makefile (tests): Add tst-sleep-cancel. * nptl/tst-sleep-cancel.c: New. * sysdeps/unix/sysv/linux/sleep.c: Include bits/libc-lock.h. (__sleep): Enable cleanup handler to restore the signal mask at least on synchronous cancellation. * intro/time.texi (sleep): Expand unsafety rationale. --- manual/time.texi | 8 ++ nptl/Makefile | 3 + nptl/tst-sleep-cancel.c | 133 +++++++++++++++++++++++++++++++++++++++ sysdeps/unix/sysv/linux/sleep.c | 8 +- 4 files changed, 144 insertions(+), 8 deletions(-) create mode 100644 nptl/tst-sleep-cancel.c diff --git a/manual/time.texi b/manual/time.texi index 8a5f94e..8fe97dc 100644 --- a/manual/time.texi +++ b/manual/time.texi @@ -2828,8 +2828,12 @@ any descriptors to wait for. @deftypefun {unsigned int} sleep (unsigned int @var{seconds}) @safety{@prelim{}@mtunsafe{@mtascusig{:SIGCHLD/linux}}@asunsafe{}@acunsafe{}} @c On Mach, it uses ports and calls time. On generic posix, it calls -@c nanosleep. On Linux, it temporarily blocks SIGCHLD, which is MT- and -@c AS-Unsafe, and in a way that makes it AC-Unsafe (C-unsafe, even!). +@c nanosleep. On GNU/Linux, it may temporarily block SIGCHLD while +@c calling nanosleep, restoring it in a cleanup handler that does not +@c cover the entire asynchronous region, and other threads might modify +@c the signal handler after we test whether it is SIG_IGN, causing the +@c function to wake up when it shouldn't or to block a signal that +@c should wake it up. The @code{sleep} function waits for @var{seconds} or until a signal is delivered, whichever happens first. diff --git a/nptl/Makefile b/nptl/Makefile index 3d61ec1..67fc349 100644 --- a/nptl/Makefile +++ b/nptl/Makefile @@ -272,7 +272,8 @@ tests = tst-typesizes \ tst-getpid1 tst-getpid2 tst-getpid3 \ tst-setuid3 \ tst-initializers1 $(addprefix tst-initializers1-,c89 gnu89 c99 gnu99) \ - tst-bad-schedattr + tst-bad-schedattr \ + tst-sleep-cancel xtests = tst-setuid1 tst-setuid1-static tst-setuid2 \ tst-mutexpp1 tst-mutexpp6 tst-mutexpp10 test-srcs = tst-oddstacklimit diff --git a/nptl/tst-sleep-cancel.c b/nptl/tst-sleep-cancel.c new file mode 100644 index 0000000..7cc8073 --- /dev/null +++ b/nptl/tst-sleep-cancel.c @@ -0,0 +1,133 @@ +/* Make sure cancelling sleep doesn't run cleanups with SIGCHLD blocked + Copyright (C) 2014 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 + +static int fail = -1; + +static void +cleanup (void *arg __attribute__((__unused__))) +{ + sigset_t set; + if (sigprocmask (SIG_UNBLOCK, NULL, &set) != 0) + { + puts ("FAIL: sigprocmask in cleanup failed"); + fail = -2; + } + else if (sigismember (&set, SIGCHLD)) + { + puts ("FAIL: SIGCHLD is blocked in cleanup handler"); + fail = 1; + } + else + fail = 0; +} + +/* Check that, when sleep is canceled, it does not leave SIGCHLD + blocked. */ +static void * +sleeping_beauty (void *arg __attribute__((__unused__))) +{ + sigset_t set; + + if (sigprocmask (SIG_UNBLOCK, NULL, &set) != 0) + { + puts ("FAIL: sigprocmask prior to sleep failed"); + fail = -3; + return &fail; + } + + if (sigismember (&set, SIGCHLD)) + { + puts ("FAIL: SIGCHLD is already blocked prior to sleep"); + fail = -4; + return &fail; + } + + pthread_cleanup_push (cleanup, NULL); + + sleep (1); + + pthread_cleanup_pop (1); + + return &fail; +} + +static int +do_test (void) +{ + pthread_t sleeper; + + if (signal (SIGCHLD, SIG_IGN) != 0) + { + puts ("UNDECIDED: signal failed"); + return 0; + } + + if (pthread_create (&sleeper, NULL, sleeping_beauty, NULL) != 0) + { + puts ("UNDECIDED: pthread_create failed"); + return 0; + } + + if (pthread_cancel (sleeper) != 0) + { + puts ("UNDECIDED: pthread_cancel failed"); + return 0; + } + + void *result; + if (pthread_join (sleeper, &result) != 0) + { + puts ("UNDECIDED: pthread_join failed"); + return 0; + } + + if (result != 0 && result != (void*)-1) + { + printf ("UNDECIDED: sleeping thread returned non-NULL: %i\n", + *(int *)result); + return 0; + } + + if (fail == -1) + { + puts ("UNDECIDED: fail set to -1, thread canceled too early"); + return 0; + } + else if (fail != 0) + { + printf ("FAIL: fail set to %i\n", fail); + return 1; + } + + puts ("All OK"); + return 0; +} + +#ifdef MAIN +int main() { + return do_test(); +} +#else +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" +#endif diff --git a/sysdeps/unix/sysv/linux/sleep.c b/sysdeps/unix/sysv/linux/sleep.c index 5411fd5..2777867 100644 --- a/sysdeps/unix/sysv/linux/sleep.c +++ b/sysdeps/unix/sysv/linux/sleep.c @@ -24,15 +24,13 @@ #include #include #include +#include - -#if 0 static void cl (void *arg) { (void) __sigprocmask (SIG_SETMASK, arg, (sigset_t *) NULL); } -#endif /* We are going to use the `nanosleep' syscall of the kernel. But the @@ -104,7 +102,7 @@ __sleep (unsigned int seconds) have to do anything here. */ if (oact.sa_handler == SIG_IGN) { - //__libc_cleanup_push (cl, &oset); + __libc_cleanup_push (cl, &oset); /* We should leave SIGCHLD blocked. */ while (1) @@ -121,7 +119,7 @@ __sleep (unsigned int seconds) } } - //__libc_cleanup_pop (0); + __libc_cleanup_pop (0); saved_errno = errno; /* Restore the original signal mask. */