bin/74404: sh(1) does not handle signals to subshells properly and/or $! is broken

Jilles Tjoelker jilles at stack.nl
Thu Apr 9 13:50:08 PDT 2009


The following reply was made to PR bin/74404; it has been noted by GNATS.

From: Jilles Tjoelker <jilles at stack.nl>
To: Mike Silbersack <silby at silby.com>
Cc: bug-followup at 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--


More information about the freebsd-bugs mailing list