diff mbox

[v2] Remove signal handling for nanosleep (bug 16364)

Message ID 1447160038-11754-1-git-send-email-adhemerval.zanella@linaro.org
State New
Headers show

Commit Message

Adhemerval Zanella Netto Nov. 10, 2015, 12:53 p.m. UTC
From: Adhemerval Zanella <adhemerval.zanella@linaro.com>

Linux 2.6.32 and forward do not show the issue regarding SysV SIGCHLD
vs. SIG_IGN for nanosleep which make it feasible to use it for sleep
implementation without requiring any hacking to handle the spurious
wake up.  The issue is likely being fixed before 2.6 and git
history [1] [2].

This patch simplifies the sleep code to call nanosleep directly by
using the posix default version.  It also removes the early cancellation
tests for zero argument, since nanosleep will handle cancellation
in this case.

[1] https://lkml.org/lkml/2004/11/25/5
[2] https://lkml.org/lkml/2003/11/8/50

Checked on x86_64, ppc64le, and aarch64.

	[BZ #16364]
	* sysdeps/unix/sysv/linux/sleep.c: Remove file
	* sysdeps/posix/sleep.c (__sleep): Simplify cancellation handling.
	* posix/tst-nanosleep-signal.c: New file.
	* posix/Makefile (test): Add tst-nanosleep-signal.
---
 posix/Makefile                  |   4 +-
 posix/tst-nanosleep-signal.c    | 113 ++++++++++++++++++++++++++++++
 sysdeps/posix/sleep.c           |   9 ---
 sysdeps/unix/sysv/linux/sleep.c | 149 ----------------------------------------
 5 files changed, 123 insertions(+), 160 deletions(-)
 create mode 100644 posix/tst-nanosleep-signal.c
 delete mode 100644 sysdeps/unix/sysv/linux/sleep.c

Comments

Andreas Schwab Nov. 10, 2015, 1:16 p.m. UTC | #1
Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

> +/* Fork and terminate the child in 0.25s while parent waits on a nanosleep
> +   for 0.5s.  */
> +static int
> +check_nanosleep_base (void)
> +{
> +  int ret = 0;
> +  pid_t f = fork ();
> +  if (f == 0)
> +    {
> +      // child
> +      struct timespec tv = { .tv_sec = 0, .tv_nsec = 250000000UL };
> +      nanosleep (&tv, &tv);
> +      _exit (EXIT_SUCCESS);
> +    }
> +  else if (f > 0)
> +    {
> +      // parent
> +      struct timespec tv = { .tv_sec = 0, .tv_nsec = 500000000UL };
> +      ret = nanosleep (&tv, &tv);
> +    }
> +  else if (f == -1)
> +    {
> +      puts ("error: fork failed");
> +      _exit (EXIT_FAILURE);
> +    }
> +  return ret;
> +}

You should wait for the child before returning.
How do you avoid the race between the parent and the child?

Andreas.
Adhemerval Zanella Netto Nov. 10, 2015, 1:29 p.m. UTC | #2
On 10-11-2015 11:16, Andreas Schwab wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> 
>> +/* Fork and terminate the child in 0.25s while parent waits on a nanosleep
>> +   for 0.5s.  */
>> +static int
>> +check_nanosleep_base (void)
>> +{
>> +  int ret = 0;
>> +  pid_t f = fork ();
>> +  if (f == 0)
>> +    {
>> +      // child
>> +      struct timespec tv = { .tv_sec = 0, .tv_nsec = 250000000UL };
>> +      nanosleep (&tv, &tv);
>> +      _exit (EXIT_SUCCESS);
>> +    }
>> +  else if (f > 0)
>> +    {
>> +      // parent
>> +      struct timespec tv = { .tv_sec = 0, .tv_nsec = 500000000UL };
>> +      ret = nanosleep (&tv, &tv);
>> +    }
>> +  else if (f == -1)
>> +    {
>> +      puts ("error: fork failed");
>> +      _exit (EXIT_FAILURE);
>> +    }
>> +  return ret;
>> +}
> 
> You should wait for the child before returning.
> How do you avoid the race between the parent and the child?
> 

That's the hole points of the previous discussion in v1 patch while
Florian also pointed this racy.  He neither I could devise a race-free
testcase to check for this issue so my questioning was if someone have
a way to remove the race or if we really should push for this test. 

> Andreas.
>
Adhemerval Zanella Netto Nov. 10, 2015, 1:33 p.m. UTC | #3
On 10-11-2015 11:29, Adhemerval Zanella wrote:
> 
> 
> On 10-11-2015 11:16, Andreas Schwab wrote:
>> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>>
>>> +/* Fork and terminate the child in 0.25s while parent waits on a nanosleep
>>> +   for 0.5s.  */
>>> +static int
>>> +check_nanosleep_base (void)
>>> +{
>>> +  int ret = 0;
>>> +  pid_t f = fork ();
>>> +  if (f == 0)
>>> +    {
>>> +      // child
>>> +      struct timespec tv = { .tv_sec = 0, .tv_nsec = 250000000UL };
>>> +      nanosleep (&tv, &tv);
>>> +      _exit (EXIT_SUCCESS);
>>> +    }
>>> +  else if (f > 0)
>>> +    {
>>> +      // parent
>>> +      struct timespec tv = { .tv_sec = 0, .tv_nsec = 500000000UL };
>>> +      ret = nanosleep (&tv, &tv);
>>> +    }
>>> +  else if (f == -1)
>>> +    {
>>> +      puts ("error: fork failed");
>>> +      _exit (EXIT_FAILURE);
>>> +    }
>>> +  return ret;
>>> +}
>>
>> You should wait for the child before returning.
>> How do you avoid the race between the parent and the child?
>>
> 
> That's the hole points of the previous discussion in v1 patch while

s/hole points/whole point

> Florian also pointed this racy.  He neither I could devise a race-free
> testcase to check for this issue so my questioning was if someone have
> a way to remove the race or if we really should push for this test. 
> 
>> Andreas.
>>
Andreas Schwab Nov. 10, 2015, 2:22 p.m. UTC | #4
Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

> That's the hole points of the previous discussion in v1 patch while
> Florian also pointed this racy.  He neither I could devise a race-free
> testcase to check for this issue so my questioning was if someone have
> a way to remove the race or if we really should push for this test. 

A racy test is as good as a non-existing test.  Everyone will ignore it.

Andreas.
Florian Weimer Nov. 10, 2015, 5:48 p.m. UTC | #5
On 11/10/2015 03:22 PM, Andreas Schwab wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> 
>> That's the hole points of the previous discussion in v1 patch while
>> Florian also pointed this racy.  He neither I could devise a race-free
>> testcase to check for this issue so my questioning was if someone have
>> a way to remove the race or if we really should push for this test. 
> 
> A racy test is as good as a non-existing test.  Everyone will ignore it.

I disagree very strongly.  It depends on the frequency of the race, and
it which direction it errs (FAIL even without the bug, or PASS with the
bug).  Some properties are impossible to test without theoretic races.
It really depends on the rate of inappropriate FAILs whether such tests
have value or not.

Florian
Adhemerval Zanella Netto Nov. 11, 2015, 12:42 p.m. UTC | #6
On 10-11-2015 15:48, Florian Weimer wrote:
> On 11/10/2015 03:22 PM, Andreas Schwab wrote:
>> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>>
>>> That's the hole points of the previous discussion in v1 patch while
>>> Florian also pointed this racy.  He neither I could devise a race-free
>>> testcase to check for this issue so my questioning was if someone have
>>> a way to remove the race or if we really should push for this test. 
>>
>> A racy test is as good as a non-existing test.  Everyone will ignore it.
> 
> I disagree very strongly.  It depends on the frequency of the race, and
> it which direction it errs (FAIL even without the bug, or PASS with the
> bug).  Some properties are impossible to test without theoretic races.
> It really depends on the rate of inappropriate FAILs whether such tests
> have value or not.

Regarding to this specific test, IMHO I would prefer to not add it since
it clearly a kernel issue which has been fixed in a long time and it is
quite unlike to regress.  Also, any regression would be flagged a kernel
defect and I do not see this being deployed in any kernel release.

Now regarding the racy test, I see we need to assess by case basis. I
also I do not see strong reasoning to block this patch altogether: we
can evaluate/push the version v3 which do not have the testcase and 
if we decide this race-test is valuable I can prepare another patch 
to add it.  

> 
> Florian
>
Adhemerval Zanella Netto Nov. 17, 2015, 2:08 p.m. UTC | #7
On 11-11-2015 10:42, Adhemerval Zanella wrote:
> 
> 
> On 10-11-2015 15:48, Florian Weimer wrote:
>> On 11/10/2015 03:22 PM, Andreas Schwab wrote:
>>> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>>>
>>>> That's the hole points of the previous discussion in v1 patch while
>>>> Florian also pointed this racy.  He neither I could devise a race-free
>>>> testcase to check for this issue so my questioning was if someone have
>>>> a way to remove the race or if we really should push for this test. 
>>>
>>> A racy test is as good as a non-existing test.  Everyone will ignore it.
>>
>> I disagree very strongly.  It depends on the frequency of the race, and
>> it which direction it errs (FAIL even without the bug, or PASS with the
>> bug).  Some properties are impossible to test without theoretic races.
>> It really depends on the rate of inappropriate FAILs whether such tests
>> have value or not.
> 
> Regarding to this specific test, IMHO I would prefer to not add it since
> it clearly a kernel issue which has been fixed in a long time and it is
> quite unlike to regress.  Also, any regression would be flagged a kernel
> defect and I do not see this being deployed in any kernel release.
> 
> Now regarding the racy test, I see we need to assess by case basis. I
> also I do not see strong reasoning to block this patch altogether: we
> can evaluate/push the version v3 which do not have the testcase and 
> if we decide this race-test is valuable I can prepare another patch 
> to add it.  

It appears this discussion has been stalled and to move forward at least
with the patch core discussion (nanosleep refactor), I would like to
continue discuss solely the code modification at its third version [1].

I will send another patch with the only the testcase and then we can
continue discuss if it worth or not to include possible racy testcase
in GLIBC.

[1] https://sourceware.org/ml/libc-alpha/2015-11/msg00206.html

> 
>>
>> Florian
>>
Florian Weimer Nov. 17, 2015, 2:18 p.m. UTC | #8
On 11/17/2015 03:08 PM, Adhemerval Zanella wrote:

> It appears this discussion has been stalled and to move forward at least
> with the patch core discussion (nanosleep refactor), I would like to
> continue discuss solely the code modification at its third version [1].
> 
> I will send another patch with the only the testcase and then we can
> continue discuss if it worth or not to include possible racy testcase
> in GLIBC.
> 
> [1] https://sourceware.org/ml/libc-alpha/2015-11/msg00206.html

I have no objections to this patch as a first step.  (“Remove file” in
changelog should be followed by a “.”.)

Thanks,
Florian
Adhemerval Zanella Netto Nov. 18, 2015, 12:54 p.m. UTC | #9
On 17-11-2015 12:18, Florian Weimer wrote:
> On 11/17/2015 03:08 PM, Adhemerval Zanella wrote:
> 
>> It appears this discussion has been stalled and to move forward at least
>> with the patch core discussion (nanosleep refactor), I would like to
>> continue discuss solely the code modification at its third version [1].
>>
>> I will send another patch with the only the testcase and then we can
>> continue discuss if it worth or not to include possible racy testcase
>> in GLIBC.
>>
>> [1] https://sourceware.org/ml/libc-alpha/2015-11/msg00206.html
> 
> I have no objections to this patch as a first step.  (“Remove file” in
> changelog should be followed by a “.”.)

Thanks, I will push the patch and resend the testcase.

> 
> Thanks,
> Florian
>
diff mbox

Patch

diff --git a/posix/Makefile b/posix/Makefile
index aeb9890..cec06a6 100644
--- a/posix/Makefile
+++ b/posix/Makefile
@@ -74,8 +74,8 @@  tests		:= tstgetopt testfnm runtests runptests	     \
 		   bug-regex21 bug-regex22 bug-regex23 bug-regex24 \
 		   bug-regex25 bug-regex26 bug-regex27 bug-regex28 \
 		   bug-regex29 bug-regex30 bug-regex31 bug-regex32 \
-		   bug-regex33 tst-nice tst-nanosleep tst-regex2 \
-		   transbug tst-rxspencer tst-pcre tst-boost \
+		   bug-regex33 tst-nice tst-nanosleep tst-nanosleep-signal \
+		   tst-regex2 transbug tst-rxspencer tst-pcre tst-boost \
 		   bug-ga1 tst-vfork1 tst-vfork2 tst-vfork3 tst-waitid \
 		   tst-getaddrinfo2 bug-glob1 bug-glob2 bug-glob3 tst-sysconf \
 		   tst-execvp1 tst-execvp2 tst-execlp1 tst-execlp2 \
diff --git a/posix/tst-nanosleep-signal.c b/posix/tst-nanosleep-signal.c
new file mode 100644
index 0000000..cb0aa54
--- /dev/null
+++ b/posix/tst-nanosleep-signal.c
@@ -0,0 +1,113 @@ 
+/* Check if nanosleep handles correctly SIGCHLD.
+   Copyright (C) 2015 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 <stdlib.h>
+#include <unistd.h>
+#include <time.h>
+#include <signal.h>
+#include <sys/time.h>
+#include <errno.h>
+
+static int errors = 0;
+
+static void
+dummy (int sig)
+{
+}
+
+/* Fork and terminate the child in 0.25s while parent waits on a nanosleep
+   for 0.5s.  */
+static int
+check_nanosleep_base (void)
+{
+  int ret = 0;
+  pid_t f = fork ();
+  if (f == 0)
+    {
+      // child
+      struct timespec tv = { .tv_sec = 0, .tv_nsec = 250000000UL };
+      nanosleep (&tv, &tv);
+      _exit (EXIT_SUCCESS);
+    }
+  else if (f > 0)
+    {
+      // parent
+      struct timespec tv = { .tv_sec = 0, .tv_nsec = 500000000UL };
+      ret = nanosleep (&tv, &tv);
+    }
+  else if (f == -1)
+    {
+      puts ("error: fork failed");
+      _exit (EXIT_FAILURE);
+    }
+  return ret;
+}
+
+void
+check_nanosleep_sigdfl (void)
+{
+  /* For SIG_DFL nanosleep should not be interrupted by SIGCHLD.  */
+  signal (SIGCHLD, SIG_DFL);
+  if (check_nanosleep_base ())
+    {
+      printf ("error: nanosleep was interrupted with SIG_DFL (errno = %i)",
+	      errno);
+      errors = 1;
+    }
+}
+
+void
+check_nanosleep_dummy (void)
+{
+  /* With a dummy handler nanosleep should be interrupted.  */
+  signal (SIGCHLD, dummy);
+  int ret = check_nanosleep_base ();
+  if ((ret == 0) && (errno == EINTR))
+    {
+      puts ("error: nanosleep was not interrupted with dummy handler");
+      errors = 1;
+    }
+}
+
+void
+check_nanosleep_sigign (void)
+{
+  /* For SIG_IGN nanosleep should not be interrupted by SIGCHLD.  */
+  signal (SIGCHLD, SIG_IGN);
+  if (check_nanosleep_base ())
+    {
+      printf ("error: nanosleep was interrupted with SIG_IGN (errno = %i)",
+	      errno);
+      errors = 1;
+    }
+}
+
+
+static int
+do_test (void)
+{
+  check_nanosleep_sigdfl ();
+  check_nanosleep_dummy ();
+  check_nanosleep_sigign ();
+
+  return errors;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/sysdeps/posix/sleep.c b/sysdeps/posix/sleep.c
index 75d9c18..80beb2a 100644
--- a/sysdeps/posix/sleep.c
+++ b/sysdeps/posix/sleep.c
@@ -32,15 +32,6 @@ 
 unsigned int
 __sleep (unsigned int seconds)
 {
-  /* This is not necessary but some buggy programs depend on it.  */
-  if (__glibc_unlikely (seconds == 0))
-    {
-#ifdef CANCELLATION_P
-      CANCELLATION_P (THREAD_SELF);
-#endif
-      return 0;
-    }
-
   int save_errno = errno;
 
   const unsigned int max
diff --git a/sysdeps/unix/sysv/linux/sleep.c b/sysdeps/unix/sysv/linux/sleep.c
deleted file mode 100644
index 2a06d5a..0000000
--- a/sysdeps/unix/sysv/linux/sleep.c
+++ /dev/null
@@ -1,149 +0,0 @@ 
-/* Implementation of the POSIX sleep function using nanosleep.
-   Copyright (C) 1996-2015 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-   Contributed by Ulrich Drepper <drepper@cygnus.com>, 1996.
-
-   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 <errno.h>
-#include <time.h>
-#include <signal.h>
-#include <string.h>	/* For the real memset prototype.  */
-#include <unistd.h>
-#include <sys/param.h>
-#include <nptl/pthreadP.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
-   kernel does not implement the stupid SysV SIGCHLD vs. SIG_IGN
-   behaviour for this syscall.  Therefore we have to emulate it here.  */
-unsigned int
-__sleep (unsigned int seconds)
-{
-  const unsigned int max
-    = (unsigned int) (((unsigned long int) (~((time_t) 0))) >> 1);
-  struct timespec ts;
-  sigset_t set, oset;
-  unsigned int result;
-
-  /* This is not necessary but some buggy programs depend on this.  */
-  if (__glibc_unlikely (seconds == 0))
-    {
-#ifdef CANCELLATION_P
-      CANCELLATION_P (THREAD_SELF);
-#endif
-      return 0;
-    }
-
-  ts.tv_sec = 0;
-  ts.tv_nsec = 0;
- again:
-  if (sizeof (ts.tv_sec) <= sizeof (seconds))
-    {
-      /* Since SECONDS is unsigned assigning the value to .tv_sec can
-	 overflow it.  In this case we have to wait in steps.  */
-      ts.tv_sec += MIN (seconds, max);
-      seconds -= (unsigned int) ts.tv_sec;
-    }
-  else
-    {
-      ts.tv_sec = (time_t) seconds;
-      seconds = 0;
-    }
-
-  /* Linux will wake up the system call, nanosleep, when SIGCHLD
-     arrives even if SIGCHLD is ignored.  We have to deal with it
-     in libc.  We block SIGCHLD first.  */
-  __sigemptyset (&set);
-  __sigaddset (&set, SIGCHLD);
-  if (__sigprocmask (SIG_BLOCK, &set, &oset))
-    return -1;
-
-  /* If SIGCHLD is already blocked, we don't have to do anything.  */
-  if (!__sigismember (&oset, SIGCHLD))
-    {
-      int saved_errno;
-      struct sigaction oact;
-
-      __sigemptyset (&set);
-      __sigaddset (&set, SIGCHLD);
-
-      /* We get the signal handler for SIGCHLD.  */
-      if (__sigaction (SIGCHLD, (struct sigaction *) NULL, &oact) < 0)
-	{
-	  saved_errno = errno;
-	  /* Restore the original signal mask.  */
-	  (void) __sigprocmask (SIG_SETMASK, &oset, (sigset_t *) NULL);
-	  __set_errno (saved_errno);
-	  return -1;
-	}
-
-      /* Note the sleep() is a cancellation point.  But since we call
-	 nanosleep() which itself is a cancellation point we do not
-	 have to do anything here.  */
-      if (oact.sa_handler == SIG_IGN)
-	{
-	  //__libc_cleanup_push (cl, &oset);
-
-	  /* We should leave SIGCHLD blocked.  */
-	  while (1)
-	    {
-	      result = __nanosleep (&ts, &ts);
-
-	      if (result != 0 || seconds == 0)
-		break;
-
-	      if (sizeof (ts.tv_sec) <= sizeof (seconds))
-		{
-		  ts.tv_sec = MIN (seconds, max);
-		  seconds -= (unsigned int) ts.tv_nsec;
-		}
-	    }
-
-	  //__libc_cleanup_pop (0);
-
-	  saved_errno = errno;
-	  /* Restore the original signal mask.  */
-	  (void) __sigprocmask (SIG_SETMASK, &oset, (sigset_t *) NULL);
-	  __set_errno (saved_errno);
-
-	  goto out;
-	}
-
-      /* We should unblock SIGCHLD.  Restore the original signal mask.  */
-      (void) __sigprocmask (SIG_SETMASK, &oset, (sigset_t *) NULL);
-    }
-
-  result = __nanosleep (&ts, &ts);
-  if (result == 0 && seconds != 0)
-    goto again;
-
- out:
-  if (result != 0)
-    /* Round remaining time.  */
-    result = seconds + (unsigned int) ts.tv_sec + (ts.tv_nsec >= 500000000L);
-
-  return result;
-}
-weak_alias (__sleep, sleep)