Subject: bug#9076: closed (Re: bug#9098: timeout should use waitpid)

From: [email-masked] (GNU bug Tracking System)
Date: Sat, 16 Jul 2011 19:20:04 +0000

Your bug report

#9076: coreutils-8.12 uses SA_RESETHAND and SA_RESTART unconditionally

which was filed against the coreutils package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
9076: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=9076
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#9098: timeout should use waitpid
From: Paul Eggert <[email-masked]>
To: Andreas Schwab <[email-masked]>
Cc: [email-masked],Pádraig Brady <[email-masked]>, [email-masked]
Date: Sat, 16 Jul 2011 12:18:52 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

Thanks for reporting that.  I merged that into some
other timeout stuff I was already looking at, and pushed
the following set of patches.  They also should fix Bug#9076,
so I'm CC'ing that and marking both bugs "done".

I have a couple of other ideas for improving "timeout" but
figure they could use some review so I'll post them as
separate bug reports.

>From bbd4c9edfa4bd5650106261b7d9b1dd434d91581 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Fri, 15 Jul 2011 17:38:32 -0700
Subject: [PATCH 1/6] dd: port to NonStop (Bug#9076)

* src/dd.c (SA_RESETHAND): Define to 0 if not defined.
---
 src/dd.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/src/dd.c b/src/dd.c
index 3e75412..0824f6c 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -55,6 +55,11 @@
 # endif
 #endif
 
+/* NonStop circa 2011 lacks SA_RESETHAND; see Bug#9076.  */
+#ifndef SA_RESETHAND
+# define SA_RESETHAND 0
+#endif
+
 #ifndef SIGINFO
 # define SIGINFO SIGUSR1
 #endif
-- 
1.7.4.4

>From 995299ff155bef70a3b153dc8cef11ed9b1d8904 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Fri, 15 Jul 2011 17:39:28 -0700
Subject: [PATCH 2/6] ls: port to NonStop (Bug#9076)

* src/ls.c (SA_RESTART): Define to 0 if not defined.
---
 src/ls.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/src/ls.c b/src/ls.c
index c604e14..680a7c3 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -74,6 +74,14 @@
 # endif
 #endif
 
+/* NonStop circa 2011 lacks both SA_RESTART and siginterrupt, so don't
+   restart syscalls after a signal handler fires.  This may cause
+   colors to get messed up on the screen if 'ls' is interrupted, but
+   that's the best we can do on such a platform.  */
+#ifndef SA_RESTART
+# define SA_RESTART 0
+#endif
+
 #include "system.h"
 #include <fnmatch.h>
 
-- 
1.7.4.4

>From 638e670d76b3bf573f6a9930b376811b5663881a Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Fri, 15 Jul 2011 17:48:38 -0700
Subject: [PATCH 3/6] timeout: port to NonStop (Bug#9077)

* src/timeout.c (SA_RESTART): Define to	0 if not defined.
(main): Don't assume signal handling uses SA_RESTART.
---
 src/timeout.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/src/timeout.c b/src/timeout.c
index ef660a7..895d720 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -64,6 +64,11 @@
 # include <sys/resource.h>
 #endif
 
+/* NonStop circa 2011 lacks both SA_RESTART and siginterrupt.  */
+#ifndef SA_RESTART
+# define SA_RESTART 0
+#endif
+
 #define PROGRAM_NAME "timeout"
 
 #define AUTHORS proper_name_utf8 ("Padraig Brady", "P\303\241draig Brady")
@@ -256,7 +261,8 @@ install_signal_handlers (int sigterm)
   struct sigaction sa;
   sigemptyset (&sa.sa_mask);  /* Allow concurrent calls to handler */
   sa.sa_handler = cleanup;
-  sa.sa_flags = SA_RESTART;  /* restart syscalls (like wait() below) */
+  sa.sa_flags = SA_RESTART;   /* Restart syscalls if possible, as that's
+                                 more likely to work cleanly.  */
 
   sigaction (SIGALRM, &sa, NULL); /* our timeout.  */
   sigaction (SIGINT, &sa, NULL);  /* Ctrl-C at terminal for example.  */
@@ -354,18 +360,15 @@ main (int argc, char **argv)
     }
   else
     {
+      pid_t wait_result;
       int status;
 
       alarm (timeout);
 
-      /* We're just waiting for a single process here, so wait() suffices.
-         Note the signal() calls above on GNU/Linux and BSD at least,
-         essentially call the lower level sigaction() with the SA_RESTART flag
-         set, which ensures the following wait call will only return if the
-         child exits, not on this process receiving a signal. Also we're not
-         passing WUNTRACED | WCONTINUED to a waitpid() call and so will not get
-         indication that the child has stopped or continued.  */
-      if (wait (&status) == -1)
+      while ((wait_result = wait (&status)) < 0 && errno == EINTR)
+        continue;
+
+      if (wait_result < 0)
         {
           /* shouldn't happen.  */
           error (0, errno, _("error waiting for command"));
-- 
1.7.4.4

>From 1f97794a3acfad6d169c60adec5a7ce916baa8f9 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Sat, 16 Jul 2011 05:57:19 -0700
Subject: [PATCH 4/6] * src/timeout.c (main): Use waitpid, not wait
 (Bug#9098).

Reported by Andreas Schwab.

* src/timeout.c (SA_RESTART): Define to 0 if not defined.
---
 src/timeout.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/src/timeout.c b/src/timeout.c
index 895d720..2d6dad8 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -365,7 +365,8 @@ main (int argc, char **argv)
 
       alarm (timeout);
 
-      while ((wait_result = wait (&status)) < 0 && errno == EINTR)
+      while ((wait_result = waitpid (monitored_pid, &status, 0)) < 0
+             && errno == EINTR)
         continue;
 
       if (wait_result < 0)
-- 
1.7.4.4

>From 22e9697c795148548410673260595745d4e8d764 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Sat, 16 Jul 2011 06:03:47 -0700
Subject: [PATCH 5/6] Fix capiTalization in comments.

---
 src/timeout.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/timeout.c b/src/timeout.c
index 2d6dad8..33bb8e4 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -35,7 +35,7 @@
      This can be seen with `timeout 10 dd&` for example.
      However if one brings this group to the foreground with the `fg`
      command before the timer expires, the command will remain
-     in the sTop state as the shell doesn't send a SIGCONT
+     in the stop state as the shell doesn't send a SIGCONT
      because the timeout process (group leader) is already running.
      To get the command running again one can Ctrl-Z, and do fg again.
      Note one can Ctrl-C the whole job when in this state.
@@ -333,9 +333,9 @@ main (int argc, char **argv)
   /* Setup handlers before fork() so that we
      handle any signals caused by child, without races.  */
   install_signal_handlers (term_signal);
-  signal (SIGTTIN, SIG_IGN);    /* don't sTop if background child needs tty.  */
-  signal (SIGTTOU, SIG_IGN);    /* don't sTop if background child needs tty.  */
-  signal (SIGCHLD, SIG_DFL);    /* Don't inherit CHLD handling from parent.   */
+  signal (SIGTTIN, SIG_IGN);   /* Don't stop if background child needs tty.  */
+  signal (SIGTTOU, SIG_IGN);   /* Don't stop if background child needs tty.  */
+  signal (SIGCHLD, SIG_DFL);   /* Don't inherit CHLD handling from parent.   */
 
   monitored_pid = fork ();
   if (monitored_pid == -1)
-- 
1.7.4.4

>From f9bcef4765dc7c3ecb8d7c58ae410087195e6fb1 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Sat, 16 Jul 2011 12:07:46 -0700
Subject: [PATCH 6/6] timeout: treat seconds counts like 'sleep' does

Treat fractions as a request to round up to the next representable
value, and treat out-of-range values as maximal ones.  This is
consistent with how "sleep" works.  And this way, "timeout
999999999999999999d FOO" and "timeout 4.5 foo" are more likely to
do what the user wants.
* src/timeout.c: Include c-strtod.h and xstrtod.h, not xstrtol.h.
(apply_time_suffix): Change it to the way sleep.c's time_suffix
does things.  Maybe this function (identical in both programs,
other than its name) should be moved to a library?
(parse_duration): Return a maximal value on overflow.  Return
unsigned int, not unsigned long.  Allow fractions, which round
up to the next integer value.
* tests/misc/timeout-parameters: Adjust tests to match new behavior.
Add a very large number.
---
 src/timeout.c                 |   60 ++++++++++++++++++++++++-----------------
 tests/misc/timeout-parameters |   16 +++++++++-
 2 files changed, 49 insertions(+), 27 deletions(-)

diff --git a/src/timeout.c b/src/timeout.c
index 33bb8e4..ccb4f85 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -52,7 +52,8 @@
 #include <sys/wait.h>
 
 #include "system.h"
-#include "xstrtol.h"
+#include "c-strtod.h"
+#include "xstrtod.h"
 #include "sig2str.h"
 #include "operand2sig.h"
 #include "error.h"
@@ -196,54 +197,51 @@ use the KILL (9) signal, since this signal cannot be caught.\n"), stdout);
   exit (status);
 }
 
-/* Given a long integer value *X, and a suffix character, SUFFIX_CHAR,
+/* Given a floating point value *X, and a suffix character, SUFFIX_CHAR,
    scale *X by the multiplier implied by SUFFIX_CHAR.  SUFFIX_CHAR may
    be the NUL byte or `s' to denote seconds, `m' for minutes, `h' for
    hours, or `d' for days.  If SUFFIX_CHAR is invalid, don't modify *X
-   and return false.  If *X would overflow an integer, don't modify *X
-   and return false. Otherwise return true.  */
+   and return false.  Otherwise return true.  */
 
 static bool
-apply_time_suffix (unsigned long *x, char suffix_char)
+apply_time_suffix (double *x, char suffix_char)
 {
-  unsigned int multiplier = 1;
+  int multiplier;
 
   switch (suffix_char)
     {
     case 0:
     case 's':
-      return true;
-    case 'd':
-      multiplier *= 24;
-    case 'h':
-      multiplier *= 60;
+      multiplier = 1;
+      break;
     case 'm':
-      if (multiplier > UINT_MAX / 60) /* 16 bit overflow */
-        return false;
-      multiplier *= 60;
+      multiplier = 60;
+      break;
+    case 'h':
+      multiplier = 60 * 60;
+      break;
+    case 'd':
+      multiplier = 60 * 60 * 24;
       break;
     default:
       return false;
     }
 
-  if (*x > UINT_MAX / multiplier)
-    return false;
-
   *x *= multiplier;
 
   return true;
 }
 
-static unsigned long
+static unsigned int
 parse_duration (const char* str)
 {
-  unsigned long duration;
-  char *ep;
+  double duration;
+  const char *ep;
 
-  if (xstrtoul (str, &ep, 10, &duration, NULL)
-      /* Invalid interval. Note 0 disables timeout  */
-      || (duration > UINT_MAX)
-      /* Extra chars after the number and an optional s,m,h,d char.  */
+  if (!xstrtod (str, &ep, &duration, c_strtod)
+      /* Nonnegative interval.  */
+      || ! (0 <= duration)
+      /* No extra chars after the number and an optional s,m,h,d char.  */
       || (*ep && *(ep + 1))
       /* Check any suffix char and update timeout based on the suffix.  */
       || !apply_time_suffix (&duration, *ep))
@@ -252,7 +250,19 @@ parse_duration (const char* str)
       usage (EXIT_CANCELED);
     }
 
-  return duration;
+  /* Return the requested duration, rounded up to the next representable value.
+     Treat out-of-range values as if they were maximal,
+     as that's more useful in practice than reporting an error.
+
+     FIXME: Use dtotimespec + setitimer if setitimer is available,
+     as that has higher resolution.  */
+  if (UINT_MAX <= duration)
+    return UINT_MAX;
+  else
+    {
+      unsigned int duration_floor = duration;
+      return duration_floor + (duration_floor < duration);
+    }
 }
 
 static void
diff --git a/tests/misc/timeout-parameters b/tests/misc/timeout-parameters
index 6e9152b..56804a8 100755
--- a/tests/misc/timeout-parameters
+++ b/tests/misc/timeout-parameters
@@ -37,11 +37,23 @@ test $? = 125 || fail=1
 
 # timeout overflow
 timeout $UINT_OFLOW sleep 0
-test $? = 125 || fail=1
+test $? = 0 || fail=1
 
 # timeout overflow
 timeout $(expr $UINT_MAX / 86400 + 1)d sleep 0
-test $? = 125 || fail=1
+test $? = 0 || fail=1
+
+# timeout overflow
+timeout 999999999999999999999999999999999999999999999999999999999999d sleep 0
+test $? = 0 || fail=1
+
+# floating point notation
+timeout 2.34 sleep 0
+test $? = 0 || fail=1
+
+# floating point notation
+timeout 2.34e+5d sleep 0
+test $? = 0 || fail=1
 
 # invalid signal spec
 timeout --signal=invalid 1 sleep 0
-- 
1.7.4.4

Subject: coreutils-8.12 uses SA_RESETHAND and SA_RESTART unconditionally
From: "Joachim Schmitz" <[email-masked]>
To: <[email-masked]>
Date: Thu, 14 Jul 2011 08:54:59 +0200
List-Id: <debbugs-submit.debbugs.gnu.org>

Hi folks

coreutils-8.12 uses SA_RESETHAND and SA_RESTART unconditionally and on HP
NonStop these don't exist.

SA_RESETHAND is used in csplit.c and dd.c 

SA_RESTART is used in ls.c and timeout.c

Bye, Jojo