GNU bug report logs - #5995
[PATCH] Fix exit status of signal handlers in shell scripts

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 5995 in the body.
You can then email your comments to 5995 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: "Dmitry V. Levin" <ldv <at> altlinux.org>
To: bug-coreutils <at> gnu.org
Subject: Re: [PATCH] Fix exit status of signal handlers in shell scripts
Date: Wed, 21 Apr 2010 13:59:08 +0400

[Message part 1 (text/plain, inline)]

On Sat, Jan 30, 2010 at 11:52:31PM +0300, Dmitry V. Levin wrote:
> On Sat, Jan 30, 2010 at 12:28:50PM -0700, Eric Blake wrote:
> > According to Dmitry V. Levin on 1/30/2010 12:18 PM:
> > > The value of `$?' on entrance to signal handlers in shell scripts
> > > cannot be relied upon, so set the exit code explicitly to
> > > 128 + SIGTERM == 143.
> > > * src/Makefile.am (sc_tight_scope): Use `exit 143' in signal handler.
> > 
> > I'm not sure I like the direction this is headed in.  Exiting with 143
> > when a trap is known to be caused by SIGTERM might be okay, but it would
> > be even better to reraise the signal and make the shell also exit by
> > SIGTERM (in case the caller can distinguish between exit by signal and
> > normal exit by status > 128).  But blindly giving status 143 for other
> > signals, like SIGHUP, is just wrong.  If you are going to munge trap
> > handlers to account for races, then you need one trap handler per signal
> > with an appropriate exit status for each.
> 
> One trap handler per signal is overkill in most cases.
> I think that any non-zero exit status would be sufficient.

I just want to remind you that the undefined behaviour still hasn't been
fixed.  Please make a decision what kind of fix seems to be more suitable.


-- 
ldv

[Message part 2 (application/pgp-signature, inline)]

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

From: Jim Meyering <jim <at> meyering.net>
To: bug-coreutils <at> gnu.org
Subject: Re: bug#5995: [PATCH] Fix exit status of signal handlers in shell
	scripts
Date: Wed, 21 Apr 2010 16:04:29 +0200

Dmitry V. Levin wrote:
> On Sat, Jan 30, 2010 at 11:52:31PM +0300, Dmitry V. Levin wrote:
>> On Sat, Jan 30, 2010 at 12:28:50PM -0700, Eric Blake wrote:
>> > According to Dmitry V. Levin on 1/30/2010 12:18 PM:
>> > > The value of `$?' on entrance to signal handlers in shell scripts
>> > > cannot be relied upon, so set the exit code explicitly to
>> > > 128 + SIGTERM == 143.
>> > > * src/Makefile.am (sc_tight_scope): Use `exit 143' in signal handler.
>> >
>> > I'm not sure I like the direction this is headed in.  Exiting with 143
>> > when a trap is known to be caused by SIGTERM might be okay, but it would
>> > be even better to reraise the signal and make the shell also exit by
>> > SIGTERM (in case the caller can distinguish between exit by signal and
>> > normal exit by status > 128).  But blindly giving status 143 for other
>> > signals, like SIGHUP, is just wrong.  If you are going to munge trap
>> > handlers to account for races, then you need one trap handler per signal
>> > with an appropriate exit status for each.
>>
>> One trap handler per signal is overkill in most cases.
>> I think that any non-zero exit status would be sufficient.
>
> I just want to remind you that the undefined behaviour still hasn't been
> fixed.  Please make a decision what kind of fix seems to be more suitable.

Thanks for the reminder.
I'd prefer a solution like the one used in automake:

http://git.savannah.gnu.org/cgit/automake.git/commit/?id=dbfabdfc6521979

    +am__trap='rm -f '\''$(abs_builddir)/$@-t'\''; (exit $$st); exit $$st'; \
    +trap "st=129; $$am__trap" 1; trap "st=130; $$am__trap" 2;      \
    +trap "st=141; $$am__trap" 13; trap "st=143; $$am__trap" 15; \

since it preserves signals, rather than mapping all to one,
which would inevitably produce misleading results some of the time.

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

From: "Dmitry V. Levin" <ldv <at> altlinux.org>
To: bug-coreutils <at> gnu.org
Subject: Re: bug#5995: [PATCH] Fix exit status of signal handlers in shell
	scripts
Date: Thu, 22 Apr 2010 15:53:03 +0400

[Message part 1 (text/plain, inline)]

On Wed, Apr 21, 2010 at 04:04:29PM +0200, Jim Meyering wrote:
[...]
> I'd prefer a solution like the one used in automake:
> 
> http://git.savannah.gnu.org/cgit/automake.git/commit/?id=dbfabdfc6521979
> 
>     +am__trap='rm -f '\''$(abs_builddir)/$@-t'\''; (exit $$st); exit $$st'; \
>     +trap "st=129; $$am__trap" 1; trap "st=130; $$am__trap" 2;      \
>     +trap "st=141; $$am__trap" 13; trap "st=143; $$am__trap" 15; \
> 
> since it preserves signals, rather than mapping all to one,
> which would inevitably produce misleading results some of the time.

Something like this?

From 5c36d056cbd46c43cb0bb54175fcc06b1b6069be Mon Sep 17 00:00:00 2001
From: Dmitry V. Levin <ldv <at> altlinux.org>
Date: Sat, 30 Jan 2010 16:02:36 +0000
Subject: [PATCH] Fix exit status of signal handlers in shell scripts

The value of `$?' on entrance to signal handlers in shell scripts
cannot be relied upon, so set the exit code explicitly.

* cfg.mk (sc_always_defined_macros, sc_system_h_headers): Set
the exit code in signal handler explicitly to 128 + SIG<SIGNAL>.
* src/Makefile.am (sc_tight_scope): Likewise.
* tests/test-lib.sh: Likewise.
---
 cfg.mk            |   10 ++++++++--
 src/Makefile.am   |    5 ++++-
 tests/test-lib.sh |    5 ++++-
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index 7533930..71bcb55 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -134,7 +134,10 @@ headers_with_interesting_macro_defs = \
 # Don't define macros that we already get from gnulib header files.
 sc_always_defined_macros: .re-defmac
 	@if test -f $(srcdir)/src/system.h; then			\
-	  trap 'rc=$$?; rm -f .re-defmac; exit $$rc' 0 1 2 3 15;	\
+	  trap 'rc=$$?; rm -f .re-defmac; exit $$rc' 0;			\
+	  am__exit='(exit $rc); exit $rc';				\
+	  trap "rc=129; $$am__exit" 1; trap "rc=130; $$am__exit" 2;	\
+	  trap "rc=131; $$am__exit" 3; trap "rc=143; $$am__exit" 15;	\
 	  grep -f .re-defmac $$($(VC_LIST))				\
 	    && { echo '$(ME): define the above via some gnulib .h file'	\
 		  1>&2;  exit 1; } || :;				\
@@ -153,7 +156,10 @@ sc_always_defined_macros: .re-defmac
 # the headers already included via system.h.
 sc_system_h_headers: .re-list
 	@if test -f $(srcdir)/src/system.h; then			\
-	  trap 'rc=$$?; rm -f .re-list; exit $$rc' 0 1 2 3 15;		\
+	  trap 'rc=$$?; rm -f .re-list; exit $$rc' 0;			\
+	  am__exit='(exit $rc); exit $rc';				\
+	  trap "rc=129; $$am__exit" 1; trap "rc=130; $$am__exit" 2;	\
+	  trap "rc=131; $$am__exit" 3; trap "rc=143; $$am__exit" 15;	\
 	  grep -nE -f .re-list						\
 	      $$($(VC_LIST_EXCEPT) | grep '^src/')			\
 	    && { echo '$(ME): the above are already included via system.h'\
diff --git a/src/Makefile.am b/src/Makefile.am
index 20b306d..db5359b 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -721,7 +721,10 @@ sc_check-AUTHORS: $(all_programs)
 .PHONY: sc_tight_scope
 sc_tight_scope: $(bin_PROGRAMS)
 	@t=exceptions-$$$$;						\
-	trap "s=$$?; rm -f $$t; exit $$s" 0 1 2 13 15;			\
+	trap 's=$$?; rm -f $$t; exit $$s' 0;				\
+	am__exit='(exit $s); exit $s';					\
+	trap "s=129; $$am__exit" 1; trap "s=130; $$am__exit" 2;		\
+	trap "s=141; $$am__exit" 13; trap "s=143; $$am__exit" 15;	\
 	src=`for f in $(SOURCES); do					\
 	       test -f $$f && d= || d=$(srcdir)/; echo $$d$$f; done`;	\
 	hdr=`for f in $(noinst_HEADERS); do				\
diff --git a/tests/test-lib.sh b/tests/test-lib.sh
index 7ad4331..8bf5601 100644
--- a/tests/test-lib.sh
+++ b/tests/test-lib.sh
@@ -408,7 +408,10 @@ remove_tmp_()
 # Run each test from within a temporary sub-directory named after the
 # test itself, and arrange to remove it upon exception or normal exit.
 trap remove_tmp_ 0
-trap 'Exit $?' 1 2 13 15
+trap 'Exit 129' 1
+trap 'Exit 130' 2
+trap 'Exit 141' 13
+trap 'Exit 143' 15
 
 cd "$t_" || error_ "failed to cd to $t_"

P.S.  I wonder why cfg.mk installs a signal handler for SIGQUIT while
src/Makefile.am installs a signal handler for SIGPIPE instead.

 
-- 
ldv

[Message part 2 (application/pgp-signature, inline)]

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

From: Jim Meyering <jim <at> meyering.net>
To: bug-coreutils <at> gnu.org
Subject: Re: bug#5995: [PATCH] Fix exit status of signal handlers in shell
	scripts
Date: Fri, 23 Apr 2010 16:22:36 +0200

Dmitry V. Levin wrote:
> On Wed, Apr 21, 2010 at 04:04:29PM +0200, Jim Meyering wrote:
> [...]
>> I'd prefer a solution like the one used in automake:
>>
>> http://git.savannah.gnu.org/cgit/automake.git/commit/?id=dbfabdfc6521979
>>
>>     +am__trap='rm -f '\''$(abs_builddir)/$@-t'\''; (exit $$st); exit $$st'; \
>>     +trap "st=129; $$am__trap" 1; trap "st=130; $$am__trap" 2;      \
>>     +trap "st=141; $$am__trap" 13; trap "st=143; $$am__trap" 15; \
>>
>> since it preserves signals, rather than mapping all to one,
>> which would inevitably produce misleading results some of the time.
>
> Something like this?
>
> From 5c36d056cbd46c43cb0bb54175fcc06b1b6069be Mon Sep 17 00:00:00 2001
> From: Dmitry V. Levin <ldv <at> altlinux.org>
> Date: Sat, 30 Jan 2010 16:02:36 +0000
> Subject: [PATCH] Fix exit status of signal handlers in shell scripts

Thank you!  That looks like just what I wanted.

> The value of `$?' on entrance to signal handlers in shell scripts
> cannot be relied upon, so set the exit code explicitly.
>
...
> -	  trap 'rc=$$?; rm -f .re-list; exit $$rc' 0 1 2 3 15;		\
...
> -trap 'Exit $?' 1 2 13 15
> +trap 'Exit 129' 1
> +trap 'Exit 130' 2
> +trap 'Exit 141' 13
> +trap 'Exit 143' 15
...
> P.S.  I wonder why cfg.mk installs a signal handler for SIGQUIT while
> src/Makefile.am installs a signal handler for SIGPIPE instead.

Well caught.
I don't recall when I first added "13" (SIGPIPE), but do
see that its use dates back to 2000, where it was used in the
original tests/sample-test:
  http://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=10d2bd9fe1475b
among other test scripts.

I do recall explicitly adding "3" (SIGQUIT), since that can
certainly be used to interrupt a test, and we should clean up
after it just like we do for other catchable signals.

I'll add the "3" to test-lib.sh and the 13 to the others after
coreutils-8.5.  For now I've applied your patch, as-is.

Previous Next

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