[PATCH] [EVAL] Force fork if any trap is set, not just on EXIT.

[PATCH] [EVAL] Force fork if any trap is set, not just on EXIT.

[Jilles Tjoelker]

In some cases the shell executes a subshell or an external command in
the current process. This is not done if a trap on EXIT has been set, so
that that trap can execute after the subshell or external command has
finished. Extend that check to all traps. (A trap is "set" if a
non-empty command string has been attached to it.)

Improve encapsulation by exporting an accessor function for this and
making the trap array static again.

This is much like FreeBSD SVN r194127, enhanced to apply to subshells
also (see FreeBSD SVN r194774).

Example:
  dash -c '{ trap "echo moo" TERM; sleep 3; }& sleep 1; kill $!;wait'
This should print "moo" after 3 seconds.

Example:
  dash -c '{ trap "echo moo" TERM; (sleep 3) }& sleep 1; kill $!;wait'
The same.

Example:
  dash -c '{ trap "echo moo" TERM; sleep 3; :; }& sleep 1; kill $!;wait'
This works correctly even without this patch.

Signed-off-by: Jilles Tjoelker <jilles@stack.nl>
---
 src/eval.c |    4 ++--
 src/trap.c |   16 +++++++++++++++-
 src/trap.h |    2 +-
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/src/eval.c b/src/eval.c
index 62d9d5d..e7c95cc 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -448,7 +448,7 @@ evalsubshell(union node *n, int flags)
 	int status;
 
 	expredir(n->nredir.redirect);
-	if (!backgnd && flags & EV_EXIT && !trap[0])
+	if (!backgnd && flags & EV_EXIT && !have_traps())
 		goto nofork;
 	INTOFF;
 	jp = makejob(n, 1);
@@ -829,7 +829,7 @@ bail:
 	switch (cmdentry.cmdtype) {
 	default:
 		/* Fork off a child process if necessary. */
-		if (!(flags & EV_EXIT) || trap[0]) {
+		if (!(flags & EV_EXIT) || have_traps()) {
 			INTOFF;
 			jp = makejob(cmd, 1);
 			if (forkshell(jp, cmd, FORK_FG) != 0) {
diff --git a/src/trap.c b/src/trap.c
index 3f93c46..439b8d2 100644
--- a/src/trap.c
+++ b/src/trap.c
@@ -69,7 +69,7 @@
 
 
 /* trap handler commands */
-char *trap[NSIG];
+static char *trap[NSIG];
 /* current value of signal */
 char sigmode[NSIG - 1];
 /* indicates specified signal received */
@@ -163,6 +163,20 @@ clear_traps(void)
 }
 
 
+/*
+ * Check if we have any traps enabled.
+ */
+int
+have_traps(void)
+{
+	char *volatile *tp;
+
+	for (tp = trap ; tp <= &trap[NSIG - 1] ; tp++) {
+		if (*tp && **tp)	/* trap not NULL or SIG_IGN */
+			return 1;
+	}
+	return 0;
+}
 
 /*
  * Set the signal handler for the specified signal.  The routine figures
diff --git a/src/trap.h b/src/trap.h
index f19a66b..4e897a9 100644
--- a/src/trap.h
+++ b/src/trap.h
@@ -36,12 +36,12 @@
 
 #include <signal.h>
 
-extern char *trap[];
 extern char sigmode[];
 extern volatile sig_atomic_t pendingsigs;
 
 int trapcmd(int, char **);
 void clear_traps(void);
+int have_traps(void);
 void setsignal(int);
 void ignoresig(int);
 void onsig(int);
-- 
1.7.1

Re: [PATCH] [EVAL] Force fork if any trap is set, not just on EXIT.

[Herbert Xu]

Jilles Tjoelker <jilles@stack.nl> wrote:
> In some cases the shell executes a subshell or an external command in
> the current process. This is not done if a trap on EXIT has been set, so
> that that trap can execute after the subshell or external command has
> finished. Extend that check to all traps. (A trap is "set" if a
> non-empty command string has been attached to it.)
> 
> Improve encapsulation by exporting an accessor function for this and
> making the trap array static again.
> 
> This is much like FreeBSD SVN r194127, enhanced to apply to subshells
> also (see FreeBSD SVN r194774).
> 
> Example:
>  dash -c '{ trap "echo moo" TERM; sleep 3; }& sleep 1; kill $!;wait'
> This should print "moo" after 3 seconds.
> 
> Example:
>  dash -c '{ trap "echo moo" TERM; (sleep 3) }& sleep 1; kill $!;wait'
> The same.
> 
> Example:
>  dash -c '{ trap "echo moo" TERM; sleep 3; :; }& sleep 1; kill $!;wait'
> This works correctly even without this patch.
> 
> Signed-off-by: Jilles Tjoelker <jilles@stack.nl>

I've applied it with some changes.  In particuar, I've made the
have_traps code done using a count kept up-to-date by trapcmd
and clear_traps.  Let me know if you see any problems with this.

diff --git a/ChangeLog b/ChangeLog
index 650899a..969d6be 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2010-05-27  Jilles Tjoelker <jilles@stack.nl>
+
+	* Force fork if any trap is set, not just on EXIT.
+
 2010-05-27  Herbert Xu <herbert@gondor.apana.org.au>
 
 	* Fix poplocalvar on abnormal exit from function.
diff --git a/src/eval.c b/src/eval.c
index d5c1e6c..439f881 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -449,7 +449,7 @@ evalsubshell(union node *n, int flags)
 	int status;
 
 	expredir(n->nredir.redirect);
-	if (!backgnd && flags & EV_EXIT && !trap[0])
+	if (!backgnd && flags & EV_EXIT && !have_traps())
 		goto nofork;
 	INTOFF;
 	jp = makejob(n, 1);
@@ -836,7 +836,7 @@ bail:
 	switch (cmdentry.cmdtype) {
 	default:
 		/* Fork off a child process if necessary. */
-		if (!(flags & EV_EXIT) || trap[0]) {
+		if (!(flags & EV_EXIT) || have_traps()) {
 			INTOFF;
 			jp = makejob(cmd, 1);
 			if (forkshell(jp, cmd, FORK_FG) != 0) {
diff --git a/src/trap.c b/src/trap.c
index 3f93c46..ba32da6 100644
--- a/src/trap.c
+++ b/src/trap.c
@@ -69,7 +69,9 @@
 
 
 /* trap handler commands */
-char *trap[NSIG];
+static char *trap[NSIG];
+/* number of non-null traps */
+int trapcnt;
 /* current value of signal */
 char sigmode[NSIG - 1];
 /* indicates specified signal received */
@@ -125,11 +127,17 @@ trapcmd(int argc, char **argv)
 		if (action) {
 			if (action[0] == '-' && action[1] == '\0')
 				action = NULL;
-			else
+			else {
+				if (*action)
+					trapcnt++;
 				action = savestr(action);
+			}
 		}
-		if (trap[signo])
+		if (trap[signo]) {
+			if (*trap[signo])
+				trapcnt--;
 			ckfree(trap[signo]);
+		}
 		trap[signo] = action;
 		if (signo != 0)
 			setsignal(signo);
@@ -150,16 +158,17 @@ clear_traps(void)
 {
 	char **tp;
 
+	INTOFF;
 	for (tp = trap ; tp < &trap[NSIG] ; tp++) {
 		if (*tp && **tp) {	/* trap not NULL or SIG_IGN */
-			INTOFF;
 			ckfree(*tp);
 			*tp = NULL;
 			if (tp != &trap[0])
 				setsignal(tp - trap);
-			INTON;
 		}
 	}
+	trapcnt = 0;
+	INTON;
 }
 
 
diff --git a/src/trap.h b/src/trap.h
index f19a66b..50fc3ed 100644
--- a/src/trap.h
+++ b/src/trap.h
@@ -36,7 +36,7 @@
 
 #include <signal.h>
 
-extern char *trap[];
+extern int trapcnt;
 extern char sigmode[];
 extern volatile sig_atomic_t pendingsigs;
 
@@ -49,3 +49,8 @@ int dotrap(void);
 void setinteractive(int);
 void exitshell(void) __attribute__((__noreturn__));
 int decode_signal(const char *, int);
+
+static inline int have_traps(void)
+{
+	return trapcnt;
+}

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt