Re: signal weirdness fix

[Zoltan Hidvegi <hzoli@xxxxxxxxxx>]

Bart Schaefer wrote:
> On Nov 26,  8:46am, Zefram wrote:
> > Remember that odd behaviour I reported, that zsh thought it received a
> > signal actually sent to its foreground job?
> > 
> > This patch limits it to SIGHUP, SIGINT and SIGQUIT, and disables this
> > behaviour completely in non-interactive shells.  I think this is a
> good
> > semantic.
> 
> Actually, I think this is almost exactly the wrong semantic.  The whole
> point was so that zsh scripts (which are pretty much by definition not
> "interactive shells" even though the script may be doing terminal I/O)
> could have their HUP/INT/QUIT traps tripped even when not executing a
> builtin command at the time the signal came in.
> 
> If you must limit this to some subset of shells (and not just to some
> subset of signals), I beg you to choose some other criteria.  I have no
> problem with limiting it to HUP/INT/QUIT, but requiring interact is too
> much.

I investigated this problem a little more and it turns out that Zefram's
patch in article 2480 was almost the right thing to do.  A terminal signal
is sent to all processes whose process group is the same as the terminal's
process group.  When the MONITOR option is set, foreground processes have a
different process group from the shell so in that case the shell itself
could not see terminal signals.  Examining the Linux kernel, and the
behaviour of ksh and pdksh it seems that only SIGINT, SIGQUIT and SIGTSTP
needs this special treatment.  SIGTSTP is not handled by this patch as it
requires a different treatment.

When MONITOR is not set the shell and the foreground process runs in the
same process group so both receive the signal.  As a result zsh executed
the handler twice before this patch.

There was an other bug here: when SIGINT was received errflag was set to 1
in printjob() called from update_job() called from handler().  I do not
completely understand this but errflag should only be set if SIGINT is not
trapped.  Originally it was always set.  Peter sent a patch to the old
zsh-list in article 6200 to set it only when INT is not ignored and now I
only set it when INT is neither ignored nor trapped.

I also noticed that on Linux signals received while they are blocked are
not dropped they are just delayed until they are unblocked.  If that
happens on all systems we may even leave the current zsh behaviour which
blocks all signals while it is waiting for foreground process.

Now the answer to the `WHY DO WE USE THE RETURN STATUS OF PROCESS GROUP
LEADER HERE?' question in update_job is clear: a foreground job runs in a
separate process group when monitor is set so it is quite natural to use
the status of the leader.  Although this patch fixes some bad signal
problems in zsh I still think that it is a big mess and should be ceaned
up.  The most important is to move trap execution out of the signal
handler.  We should only use libc calls which are guaranteed to be
reentrant on all systems.  This probably means that we cannot use malloc()
and stdio functions.  Preferably we should use system calls only.

This patch should be applied to both zsh-3.0.x and zsh-3.1.x.  If you
applied patch 2480 from Zefram back it out before applying this patch.

Zoltan


*** Src/jobs.c	1997/01/11 00:45:31	3.1.1.3
--- Src/jobs.c	1997/01/11 16:54:48
***************
*** 177,193 ****
      if (sigtrapped[SIGCHLD] && job != thisjob)
  	dotrap(SIGCHLD);
  
!     /* WHY DO WE USE THE RETURN STATUS OF PROCESS GROUP LEADER HERE? */
!     /* If the foreground job got a signal, pretend we got it, too.   */
!     if (inforeground && WIFSIGNALED(status)) {
! 	if (sigtrapped[WTERMSIG(status)]) {
  	    /* Run the trap with the error flag unset.
  	     * Errflag is set in printjobs if the jobs terminated
  	     * with SIGINT.  I don't know why it's done there and
  	     * not here.   (PWS 1995/06/08)
  	     */
  	    errflag = 0;
! 	    dotrap(WTERMSIG(status));
  	    /* We keep the errflag as set or not by dotrap.
  	     * This is to fulfil the promise to carry on
  	     * with the jobs if trap returns zero.
--- 177,199 ----
      if (sigtrapped[SIGCHLD] && job != thisjob)
  	dotrap(SIGCHLD);
  
!     /* When MONITOR is set, the foreground process runs in a different *
!      * process group from the shell, so the shell will not receive     *
!      * terminal signals, therefore we we pretend that the shell got    *
!      * the signal too.                                                 */
!     if (inforeground && isset(MONITOR) && WIFSIGNALED(status)) {
! 	int sig = WTERMSIG(status);
! 
! 	if(sig != SIGINT && sig != SIGQUIT)
! 	    ;
! 	else if (sigtrapped[sig]) {
  	    /* Run the trap with the error flag unset.
  	     * Errflag is set in printjobs if the jobs terminated
  	     * with SIGINT.  I don't know why it's done there and
  	     * not here.   (PWS 1995/06/08)
  	     */
  	    errflag = 0;
! 	    dotrap(sig);
  	    /* We keep the errflag as set or not by dotrap.
  	     * This is to fulfil the promise to carry on
  	     * with the jobs if trap returns zero.
***************
*** 197,204 ****
  	     */
  	    if (errflag)
  		breaks = loops;
! 	} else if (WTERMSIG(status) == SIGINT ||
! 		   WTERMSIG(status) == SIGQUIT) {
  	    breaks = loops;
  	    errflag = 1;
  	}
--- 203,209 ----
  	     */
  	    if (errflag)
  		breaks = loops;
! 	} else if (sig == SIGINT || sig == SIGQUIT) {
  	    breaks = loops;
  	    errflag = 1;
  	}
***************
*** 424,431 ****
  		    len = llen;
  		if (sig != SIGINT && sig != SIGPIPE)
  		    sflag = 1;
! 		else if (sig == SIGINT && !(sigtrapped[SIGINT] & ZSIG_IGNORED))
! 		    /* PWS 1995/05/16 added test for ignoring SIGINT */
  		    errflag = 1;
  		if (job == thisjob && sig == SIGINT)
  		    doputnl = 1;
--- 429,435 ----
  		    len = llen;
  		if (sig != SIGINT && sig != SIGPIPE)
  		    sflag = 1;
! 		else if (sig == SIGINT && !sigtrapped[SIGINT])
  		    errflag = 1;
  		if (job == thisjob && sig == SIGINT)
  		    doputnl = 1;