Re: [PATCH] Set SA_RESTART flag on SIGCHLD handler

[Jilles Tjoelker <jilles@xxxxxxxx>]

On Thu, Jun 21, 2012 at 07:36:28PM -0500, Jonathan Nieder wrote:
> Anders Kaseorg wrote:
> > On Sun, 18 Mar 2012, Jonathan Nieder wrote:

> >> --- i/src/trap.c
> >> +++ w/src/trap.c
> >> @@ -259,7 +259,7 @@ setsignal(int signo)
> >>  		act.sa_handler = SIG_DFL;
> >>  	}
> >>  	*t = action;
> >> -	act.sa_flags = 0;
> >> +	act.sa_flags = SA_RESTART;
> >>  	sigfillset(&act.sa_mask);
> >>  	sigaction(signo, &act, 0);
> >>  }
> [...]
> > It looks like this was never forwarded to the dash mailing list.  Any 
> > thoughts from upstream?

> > Context: http://thread.gmane.org/gmane.comp.version-control.git.debian/147
> > (FTR, it did happen again.)

> Thanks.  Here's a less invasive patch to experiment with.

> POSIX (XCU §2.11 "Signals and Error Handling") requires that the "wait"
> utility must return immediately when interrupted by a trapped signal,
> so SA_RESTART is not appropriate for those.

Same thing for "read", even though that does not make much sense (the
only thing the trap handler can do is abandon the read, since bytes
already read but not part of a complete line may be lost).

On the other hand, this interruption is not specified to happen with
signals whose default action is to ignore. Many shells don't do this,
though (perhaps because of the difficulty of finding what the default
action is).

> It does not seem to give any advice on whether signals interrupt
> redirection.

For certain situations it certainly does. For simple commands without
command name, it says any redirections shall be performed in a subshell
environment; hence, trapped signals are reset to the default action.
This is what most shells appear to do in all cases. Again, this may not
be the most useful behaviour but it matches what the Bourne shell did.

> diff --git i/src/redir.c w/src/redir.c
> index f96a76bc..7fb7748f 100644
> --- i/src/redir.c
> +++ w/src/redir.c
> @@ -206,8 +206,11 @@ openredirect(union node *redir)
>  		/* FALLTHROUGH */
>  	case NCLOBBER:
>  		fname = redir->nfile.expfname;
> -		if ((f = open64(fname, O_WRONLY|O_CREAT|O_TRUNC, 0666)) < 0)
> +		while ((f = open64(fname, O_WRONLY|O_CREAT|O_TRUNC, 0666)) < 0) {
> +			if (errno == EINTR)
> +				continue;
>  			goto ecreate;
> +		}
>  		break;
>  	case NAPPEND:
>  		fname = redir->nfile.expfname;

It looks like this patch breaks interrupting (via SIGINT) a blocking
open, for example of a fifo. Although the race condition associated with
relying on [EINTR] is very hard to close, I don't think that's reason to
give up on it entirely.

I think it is better to rethink commit
3800d4934391b144fd261a7957aea72ced7d47ea that caused this bug (and some
others that have been fixed). A signal handler for SIGCHLD was added to
allow waiting for a trapped signal or a child process at the same time
(using sigsuspend, as was done, or sigwait). However, leaving the signal
handler installed all the time causes unexpected [EINTR] errors, so I
suggest only installing it during the "wait" builtin or specifying
SA_RESTART only for this special SIGCHLD handler (while leaving all
other signal handlers interrupting). In either case, behaviour for
SIGCHLD handlers installed because of CHLD traps should remain
unchanged. The former seems safest because SA_RESTART handlers still
cause visible effects such as short writes.

In case of a script doing 'trap "" CHLD', it is perhaps undesirable for
"wait" to block forever, given that wait(2) doesn't do that either.

-- 
Jilles Tjoelker
--
To unsubscribe from this list: send the line "unsubscribe dash" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html