Date: Thu, 9 Apr 2009 20:50:05 GMT From: Jilles Tjoelker <jilles@stack.nl> To: freebsd-bugs@FreeBSD.org Subject: Re: bin/74404: sh(1) does not handle signals to subshells properly and/or $! is broken Message-ID: <200904092050.n39Ko5Zl030337@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR bin/74404; it has been noted by GNATS.
From: Jilles Tjoelker <jilles@stack.nl>
To: Mike Silbersack <silby@silby.com>
Cc: bug-followup@FreeBSD.org
Subject: Re: bin/74404: sh(1) does not handle signals to subshells properly
and/or $! is broken
Date: Thu, 9 Apr 2009 22:45:52 +0200
--x+6KMIRAuhnl3hBn
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
On Mon, Apr 06, 2009 at 10:11:08PM -0500, Mike Silbersack wrote:
> On Sun, 5 Apr 2009, Jilles Tjoelker wrote:
> >> [ sh forks twice for (somecommand) & ]
> > It seems reasonable for sh to fork twice here. You can avoid it by doing
> > { somecommand; } &.
> I honestly don't remember how I came up with the script in question, but I
> suspect that I picked it up from an example bash script. Based on that,
> it may be worth making the script function as is.
After investigating some more, I agree. I found an elegant way to treat
(CMD)& as { CMD; }&, and it turns out that many other shells already do
something like it, so it is likely problematic use of $! occurs more
often. The point is that it is not necessary to fork for a subshell if
this is the last thing the shell process has to do (and there are no
traps), just like already is implemented for external commands (to some
degree).
Patches for src/bin/sh, also on web space in case gnats mangles them
http://www.stack.nl/~jilles/unix/sh-forkiftrapped.patch adds the code to
check if there are traps and ensures the shell stays around waiting for
an external command if there are traps. Formerly this was tied to -T.
(This only happens with traps set in subshells. With the old code, a
workaround is to place ;: after the external command.)
http://www.stack.nl/~jilles/unix/sh-subshell-noforkifunneeded.patch
avoids forking for subshells if not necessary, and depends on the first
patch.
--
Jilles Tjoelker
--x+6KMIRAuhnl3hBn
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="sh-forkiftrapped.patch"
Don't skip forking for an external command if any traps are active.
Example:
sh -c '(trap "echo trapped" EXIT; sleep 3)'
now correctly prints "trapped".
With this check, it is no longer necessary to check for -T
explicitly in that case.
diff --git a/eval.c b/eval.c
--- a/eval.c
+++ b/eval.c
@@ -730,7 +730,7 @@ evalcommand(union node *cmd, int flags,
/* Fork off a child process if necessary. */
if (cmd->ncmd.backgnd
|| (cmdentry.cmdtype == CMDNORMAL
- && ((flags & EV_EXIT) == 0 || Tflag))
+ && ((flags & EV_EXIT) == 0 || have_traps()))
|| ((flags & EV_BACKCMD) != 0
&& (cmdentry.cmdtype != CMDBUILTIN
|| cmdentry.u.index == CDCMD
diff --git a/trap.c b/trap.c
--- a/trap.c
+++ b/trap.c
@@ -222,6 +222,21 @@ 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
* out what it should be set to.
*/
diff --git a/trap.h b/trap.h
--- a/trap.h
+++ b/trap.h
@@ -39,6 +39,7 @@ extern volatile sig_atomic_t gotwinch;
int trapcmd(int, char **);
void clear_traps(void);
+int have_traps(void);
void setsignal(int);
void ignoresig(int);
void onsig(int);
--x+6KMIRAuhnl3hBn
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="sh-subshell-noforkifunneeded.patch"
Do not fork for a subshell if it is the last thing this shell is doing
(EV_EXIT). The fork is still done as normal if any traps
are active.
Example:
sh -c '(/bin/sleep 10)& sleep 1;ps -p $! -o comm='
Now prints "sleep" instead of "sh". $! is more useful this way.
Most shells (dash, bash, pdksh, ksh93, zsh) seem to print "sleep" for this.
Example:
sh -c '( ( ( (ps jT))))'
Now shows only one waiting shell process instead of four.
Most shells (dash, bash, pdksh, ksh93, zsh) seem to show zero or one.
diff --git a/eval.c b/eval.c
--- a/eval.c
+++ b/eval.c
@@ -397,8 +397,8 @@ evalsubshell(union node *n, int flags)
int backgnd = (n->type == NBACKGND);
expredir(n->nredir.redirect);
- jp = makejob(n, 1);
- if (forkshell(jp, n, backgnd) == 0) {
+ if ((!backgnd && flags & EV_EXIT && !have_traps()) ||
+ forkshell(jp = makejob(n, 1), n, backgnd) == 0) {
if (backgnd)
flags &=~ EV_TESTED;
redirect(n->nredir.redirect, 0);
--x+6KMIRAuhnl3hBn--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200904092050.n39Ko5Zl030337>