Patchwork question about which sleep is noted in manual

login
register
mail settings
Submitter Alexandre Oliva
Date Dec. 23, 2014, 6:59 a.m.
Message ID <ormw6e4zs3.fsf@free.home>
Download mbox | patch
Permalink /patch/4409/
State New
Headers show

Comments

Alexandre Oliva - Dec. 23, 2014, 6:59 a.m.
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
Ma Shimiao - Dec. 23, 2014, 12:39 p.m.
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.
Alexandre Oliva - Jan. 6, 2015, 12:10 a.m.
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?

Patch

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.  */