From owner-freebsd-bugs@FreeBSD.ORG Thu Apr 9 20:50:05 2009 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 8E297106566B for ; Thu, 9 Apr 2009 20:50:05 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 768248FC18 for ; Thu, 9 Apr 2009 20:50:05 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.14.3/8.14.3) with ESMTP id n39Ko51d030338 for ; Thu, 9 Apr 2009 20:50:05 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.3/8.14.3/Submit) id n39Ko5Zl030337; Thu, 9 Apr 2009 20:50:05 GMT (envelope-from gnats) Date: Thu, 9 Apr 2009 20:50:05 GMT Message-Id: <200904092050.n39Ko5Zl030337@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org From: Jilles Tjoelker Cc: Subject: Re: bin/74404: sh(1) does not handle signals to subshells properly and/or $! is broken X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Jilles Tjoelker List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Apr 2009 20:50:08 -0000 The following reply was made to PR bin/74404; it has been noted by GNATS. From: Jilles Tjoelker To: Mike Silbersack 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--