question about which sleep is noted in manual
Commit Message
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
Comments
On 12/23/2014 02:59 PM, Alexandre Oliva wrote:
> 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?
This patch looks good to me.
On Dec 23, 2014, Alexandre Oliva <aoliva@redhat.com> wrote:
> 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.
Ping?
@@ -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.
@@ -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
new file mode 100644
@@ -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
@@ -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. */