bug#51135: timeout: --foreground causes exit status 124, even if KILL was used

2021-10-12 Thread Christoph Anton Mitterer
On Tue, 2021-10-12 at 14:50 +0100, Pádraig Brady wrote:
> That is a fair point.

Thanks for reconsidering :-)


> If one is using --kill-after you have to
> check for both 124 and 137 anyway to see if it timed out.
> It is useful to know whether the command was forcably killed.
> Using --foreground to avoid the 137 exit status upon --kill-after
> is not until now documented, so we should probably adjust
> the exit status to be always 137 if a SIGKILL is sent.
> 
> With the attached we now behave like:
> 
>    $ timeout -v -s0 --foreground -k2 1 sleep 3; echo $?
>    timeout: sending signal EXIT to command ‘sleep’
>    timeout: sending signal KILL to command ‘sleep’
>    137
> 
>    $ timeout -v -s0 -k2 1 sleep 3; echo $?
>    timeout: sending signal EXIT to command ‘sleep’
>    timeout: sending signal KILL to command ‘sleep’
>    Killed
>    137

Great :-)


Cheers,
Chris.





bug#51135: timeout: --foreground causes exit status 124, even if KILL was used

2021-10-12 Thread Pádraig Brady

On 12/10/2021 02:55, Christoph Anton Mitterer wrote:

Thinking again about this:

Don't you think one looses quite something if, with --foreground, one
cannot differ (via the exit status) between a timeout that allowed the
program to clean up and one (when KILLing) that didn't?


Even if the KILL happens via killing timeout itself, couldn't it just
return 128+9 in the case --foreground was enabled and the original
signal had already been sent?
Or is that technically not possible then?


That is a fair point.
If one is using --kill-after you have to
check for both 124 and 137 anyway to see if it timed out.
It is useful to know whether the command was forcably killed.
Using --foreground to avoid the 137 exit status upon --kill-after
is not until now documented, so we should probably adjust
the exit status to be always 137 if a SIGKILL is sent.

With the attached we now behave like:

  $ timeout -v -s0 --foreground -k2 1 sleep 3; echo $?
  timeout: sending signal EXIT to command ‘sleep’
  timeout: sending signal KILL to command ‘sleep’
  137

  $ timeout -v -s0 -k2 1 sleep 3; echo $?
  timeout: sending signal EXIT to command ‘sleep’
  timeout: sending signal KILL to command ‘sleep’
  Killed
  137

cheers,
Pádraig
>From 0750fcdf3447366b074cb47dd8cbe88c83ed984d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Tue, 12 Oct 2021 14:32:57 +0100
Subject: [PATCH] timeout: ensure --foreground -k exits with status 137

* src/timeout.c (main): Propagate the killed status from the child.
* doc/coreutils.texi (timeout invocation): Remove the
description of the --foreground specific handling of SIGKILL,
now that it's consistent with the default mode of operation.
* tests/misc/timeout.sh: Add a test case.
* NEWS: Mention the change in behavior.
Fixes https://bugs.gnu.org/51135
---
 NEWS  | 7 +++
 doc/coreutils.texi| 3 ---
 src/timeout.c | 5 +
 tests/misc/timeout.sh | 3 +++
 4 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/NEWS b/NEWS
index 1cb3c28a1..086da03ae 100644
--- a/NEWS
+++ b/NEWS
@@ -8,6 +8,13 @@ GNU coreutils NEWS-*- outline -*-
   All files would be processed correctly, but the exit status was incorrect.
   [bug introduced in coreutils-9.0]
 
+** Changes in behavior
+
+  timeout --foreground --kill-after=... will now exit with status 137
+  if the kill signal was sent, which is consistent with the behaviour
+  when the --foreground option is not specified.  This allows users to
+  distinguish if the command was more forcefully terminated.
+
 
 * Noteworthy changes in release 9.0 (2021-09-24) [stable]
 
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index e3ecbdcf8..02aae4ef4 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -18326,9 +18326,6 @@ exit status 137, regardless of whether that signal is sent to @var{command}
 or to @command{timeout} itself, i.e., these cases cannot be distinguished.
 In the latter case, the @var{command} process may still be alive after
 @command{timeout} has forcefully been terminated.
-However if the @option{--foreground} option is specified then
-@command{timeout} will not send any signals to its own process,
-and so it will exit with one of the other exit status values detailed above.
 
 Examples:
 
diff --git a/src/timeout.c b/src/timeout.c
index 34d792640..650563461 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -593,6 +593,11 @@ main (int argc, char **argv)
   unblock_signal (sig);
   raise (sig);
 }
+  /* Allow users to distinguish if command was forcably killed.
+ Needed with --foreground where we don't send SIGKILL to
+ the timeout process itself.  */
+  if (timed_out && sig == SIGKILL)
+preserve_status = true;
   status = sig + 128; /* what sh returns for signaled processes.  */
 }
   else
diff --git a/tests/misc/timeout.sh b/tests/misc/timeout.sh
index 44ca450d8..295a95773 100755
--- a/tests/misc/timeout.sh
+++ b/tests/misc/timeout.sh
@@ -42,7 +42,10 @@ returns_ 124 timeout --preserve-status .1 sleep 10 && fail=1
 # kill delay. Note once the initial timeout triggers,
 # the exit status will be 124 even if the command
 # exits on its own accord.
+# exit status should be 128+KILL
 returns_ 124 timeout -s0 -k1 .1 sleep 10 && fail=1
+# Ensure a consistent exit status with --foreground
+returns_ 124 timeout --foreground -s0 -k1 .1 sleep 10 && fail=1
 
 # Ensure 'timeout' is immune to parent's SIGCHLD handler
 # Use a subshell and an exec to work around a bug in FreeBSD 5.0 /bin/sh.
-- 
2.26.2



bug#51135: timeout: --foreground causes exit status 124, even if KILL was used

2021-10-11 Thread Christoph Anton Mitterer
Thinking again about this:

Don't you think one looses quite something if, with --foreground, one
cannot differ (via the exit status) between a timeout that allowed the
program to clean up and one (when KILLing) that didn't?


Even if the KILL happens via killing timeout itself, couldn't it just
return 128+9 in the case --foreground was enabled and the original
signal had already been sent?
Or is that technically not possible then?





bug#51135: timeout: --foreground causes exit status 124, even if KILL was used

2021-10-11 Thread Christoph Anton Mitterer
On Mon, 2021-10-11 at 22:20 +0100, Pádraig Brady wrote:
> For that use case it's probably best to use --preserve-status,
> in which case the 137 from the child getting the SIGKILL
> will be propagated through.

But wouldn't that make me loose the 124, if COMMAND could actually be
SIGTERMed?





bug#51135: timeout: --foreground causes exit status 124, even if KILL was used

2021-10-11 Thread Pádraig Brady

On 11/10/2021 22:11, Christoph Anton Mitterer wrote:

On Mon, 2021-10-11 at 22:04 +0100, Pádraig Brady wrote:

+However if the @option{--foreground} option is specified then
+@command{timeout} will not send any signals to its own process,
+and so it will exit with one of the other exit status values
detailed above.


So 137 is only used when the signal was sent to timeout itself?

I'd have actually found it quite nice if 137 was used *even* with
--foreground.

That way one could differentiate between whether COMMAND had the chance
to do cleanups, or whether the calling process should take care on
that.


For example, I use timeout with a program that reads a phassphrase and
prints it to stdout.
It disables the terminal's ECHO, so when it has to be killed, the
calling process would need to re-enable that mnually.


For that use case it's probably best to use --preserve-status,
in which case the 137 from the child getting the SIGKILL
will be propagated through.





bug#51135: timeout: --foreground causes exit status 124, even if KILL was used

2021-10-11 Thread Christoph Anton Mitterer
On Mon, 2021-10-11 at 22:04 +0100, Pádraig Brady wrote:
> +However if the @option{--foreground} option is specified then
> +@command{timeout} will not send any signals to its own process,
> +and so it will exit with one of the other exit status values
> detailed above.

So 137 is only used when the signal was sent to timeout itself?

I'd have actually found it quite nice if 137 was used *even* with
--foreground.

That way one could differentiate between whether COMMAND had the chance
to do cleanups, or whether the calling process should take care on
that.


For example, I use timeout with a program that reads a phassphrase and
prints it to stdout.
It disables the terminal's ECHO, so when it has to be killed, the
calling process would need to re-enable that mnually.


Cheers,
Chris.





bug#51135: timeout: --foreground causes exit status 124, even if KILL was used

2021-10-11 Thread Pádraig Brady

On 11/10/2021 19:01, Christoph Anton Mitterer wrote:

Hey.


This time I've also checked the 9.0 documentation (hopefully I wasn't
just too blind).


I noticed that whenever --foreground is used, timeout exits with a 124
status (instead of the documented 128+9) regardless of whether the KILL
is sent because of --signal=KILL or --kill-after=n .


Yes --foreground gives timeout(1) more control over the exit status in this 
case.
I'll add some clarification to the texinfo manual along these lines:


-In case of the @samp{KILL(9)} signal, @command{timeout} returns with
+In the case of the @samp{KILL(9)} signal, @command{timeout} returns with
 exit status 137, regardless of whether that signal is sent to @var{command}
 or to @command{timeout} itself, i.e., these cases cannot be distinguished.
 In the latter case, the @var{command} process may still be alive after
 @command{timeout} has forcefully been terminated.
+However if the @option{--foreground} option is specified then
+@command{timeout} will not send any signals to its own process,
+and so it will exit with one of the other exit status values detailed above.

thanks,
Pádraig





bug#51135: timeout: --foreground causes exit status 124, even if KILL was used

2021-10-11 Thread Christoph Anton Mitterer
Hey.


This time I've also checked the 9.0 documentation (hopefully I wasn't
just too blind).


I noticed that whenever --foreround is used, timeout exits with a 124
status (instead of the documented 128+9) regardless of whether the KILL
is sent because of --signal=KILL or --kill-after=n .


Thanks,
Chris.

btw: I'm sill on Debian sid's 8.32 version of coreutils.