GNU bug report logs - #9098
timeout should use waitpid

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 9098 in the body.
You can then email your comments to 9098 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Andreas Schwab <schwab <at> linux-m68k.org>
To: bug-coreutils <at> gnu.org
Subject: timeout should use waitpid
Date: Sat, 16 Jul 2011 14:17:55 +0200

timeout assumes that it only has to wait for a single child.  But this
is not true, it can always inherit other children from its predecessor:

$ sh -c "sleep 1 & exec timeout 2 sh -c 'sleep 3; echo foo'"

This will echo foo after three seconds, although the inner shell is
supposed to be killed after two seconds.

Andreas.

From 331a32b8bbe3cc07d5522004e90fba2bde20a1c3 Mon Sep 17 00:00:00 2001
From: Andreas Schwab <schwab <at> linux-m68k.org>
Date: Sat, 16 Jul 2011 13:51:06 +0200
Subject: [PATCH] timeout: ignore inherited children

* src/timeout.c (main): Use waitpid instead of wait.
---
 src/timeout.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/timeout.c b/src/timeout.c
index ef660a7..a5d42f4 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -358,14 +358,15 @@ main (int argc, char **argv)
 
       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,
+      /* 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)
+         indication that the child has stopped or continued.  We may have
+         inherited children from our predecessor, so a simple wait
+         doesn't suffice.  */
+      if (waitpid (monitored_pid, &status, 0) == -1)
         {
           /* shouldn't happen.  */
           error (0, errno, _("error waiting for command"));
-- 
1.7.6


-- 
Andreas Schwab, schwab <at> linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Message #8 received at 9098 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Andreas Schwab <schwab <at> linux-m68k.org>
Cc: 9098 <at> debbugs.gnu.org
Subject: Re: bug#9098: timeout should use waitpid
Date: Sat, 16 Jul 2011 14:15:25 +0100

On 16/07/11 13:17, Andreas Schwab wrote:
> timeout assumes that it only has to wait for a single child.  But this
> is not true, it can always inherit other children from its predecessor:
> 
> $ sh -c "sleep 1 & exec timeout 2 sh -c 'sleep 3; echo foo'"
> 
> This will echo foo after three seconds, although the inner shell is
> supposed to be killed after two seconds.
> 
> Andreas.
> 
>>From 331a32b8bbe3cc07d5522004e90fba2bde20a1c3 Mon Sep 17 00:00:00 2001
> From: Andreas Schwab <schwab <at> linux-m68k.org>
> Date: Sat, 16 Jul 2011 13:51:06 +0200
> Subject: [PATCH] timeout: ignore inherited children
> 
> * src/timeout.c (main): Use waitpid instead of wait.

Thanks for that.
Also gnulib seems to support waitpid on mingw but not wait.

So I'll apply this.

thanks,
Pádraig.

Message #13 received at 9098-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Andreas Schwab <schwab <at> linux-m68k.org>
Cc: 9098-done <at> debbugs.gnu.org,
	Pádraig Brady <P <at> draigBrady.com>,
	9076-done <at> debbugs.gnu.org
Subject: Re: bug#9098: timeout should use waitpid
Date: Sat, 16 Jul 2011 12:18:52 -0700

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 <eggert <at> cs.ucla.edu>
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 <eggert <at> cs.ucla.edu>
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 <eggert <at> cs.ucla.edu>
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 <eggert <at> cs.ucla.edu>
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 <eggert <at> cs.ucla.edu>
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 <eggert <at> cs.ucla.edu>
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

Message #14 received at 9098-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 9098-done <at> debbugs.gnu.org, Andreas Schwab <schwab <at> linux-m68k.org>
Subject: Re: bug#9098: timeout should use waitpid
Date: Sun, 17 Jul 2011 13:20:24 +0100

On 16/07/11 20:18, Paul Eggert wrote:
> 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".

Thanks for doing that.
I'll add NEWS and a test for 9098 this evening if no one gets to it first.
I notice the commit message is incorrect for this change, but
I'll add some comments around the waitpid call to explain the history.

Also I hinted that the gnulib waitpid module might now be worth including,
but it isn't due to both alarm() and setitimer() not being available on mingw.

cheers,
Pádraig.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.