This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: question about which sleep is noted in manual
- From: Alexandre Oliva <aoliva at redhat dot com>
- To: MaShimiao <mashimiao dot fnst at cn dot fujitsu dot com>
- Cc: <libc-alpha at sourceware dot org>, <codonell at redhat dot com>
- Date: Tue, 23 Dec 2014 04:59:40 -0200
- Subject: Re: question about which sleep is noted in manual
- Authentication-results: sourceware.org; auth=none
- References: <547ED48B dot 2060509 at cn dot fujitsu dot com> <oregs9zvl2 dot fsf at free dot home> <5487EAF0 dot 70305 at cn dot fujitsu dot com> <orfvcd7a7a dot fsf at free dot home> <549787C1 dot 9020807 at cn dot fujitsu dot com> <orvbl35yq5 dot fsf at free dot home>
On Dec 22, 2014, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Dec 22, 2014, MaShimiao <mashimiao.fnst@cn.fujitsu.com> 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
+ <http://www.gnu.org/licenses/>. */
+
+#include <stdio.h>
+#include <signal.h>
+#include <pthread.h>
+#include <unistd.h>
+
+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 <unistd.h>
#include <sys/param.h>
#include <nptl/pthreadP.h>
+#include <bits/libc-lock.h>
-
-#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. */
--
Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/ FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer