signal weirdness fix

signal weirdness fix

[Zefram]

-----BEGIN PGP SIGNED MESSAGE-----

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.  Unfortunately the semantic we actually want (did the signal
actually originate at the tty) is impossible to implement, but I think
this is a close approximation.

 -zefram

      Index: Src/jobs.c
      ===================================================================
      RCS file: /home/zefram/usr/cvsroot/zsh/Src/jobs.c,v
      retrieving revision 1.10
      diff -c -r1.10 jobs.c
      *** Src/jobs.c	1996/11/03 00:48:18	1.10
      --- Src/jobs.c	1996/11/26 00:01:11
      ***************
      *** 180,194 ****
        
            /* 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.
      --- 180,198 ----
        
            /* 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 && interact && WIFSIGNALED(status)) {
      ! 	int sig = WTERMSIG(status);
      ! 
      ! 	if(sig != SIGHUP && 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.
      ***************
      *** 198,205 ****
        	     */
        	    if (errflag)
        		breaks = loops;
      ! 	} else if (WTERMSIG(status) == SIGINT ||
      ! 		   WTERMSIG(status) == SIGQUIT) {
        	    breaks = loops;
        	    errflag = 1;
        	}
      --- 202,208 ----
        	     */
        	    if (errflag)
        		breaks = loops;
      ! 	} else if (sig == SIGINT || sig == SIGQUIT) {
        	    breaks = loops;
        	    errflag = 1;
        	}

-----BEGIN PGP SIGNATURE-----
Version: 2.6.2

iQCVAwUBMpo0q3D/+HJTpU/hAQFClQP/XBLb4IYMjcCmXc+7jneECAAcHJZvNs22
Yoa2gxrdSM78jHj1NUHN3V78LAqF6wITB5bspgT5sJ1hfyBlR+eQSvKd0imyDi9M
4YxtkUCaDX/GZLU+wiws4GSD7DsCD4c2yiAC+B8WlZ4WXGCKy8r++9h5DVIn4K1p
HoA+C4q4IR4=
=QpBl
-----END PGP SIGNATURE-----

Re: signal weirdness fix

[Bart Schaefer]

On Nov 26,  8:46am, Zefram wrote:
> Subject: signal weirdness fix
> -----BEGIN PGP SIGNED MESSAGE-----
> 
> 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.

Re: signal weirdness fix

[Zefram]

>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.

Hmm.  I have no strong feeling about this, but it *would* be nice to
make sure there's a tty around when processing these signals.
isatty(0) is obviously not sufficient test, so maybe that "&& interact"
should just be removed.

-zefram

Re: signal weirdness fix

[Zoltan Hidvegi]

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.  Unfortunately the semantic we actually want (did the signal
> actually originate at the tty) is impossible to implement, but I think
> this is a close approximation.

The right semantic is not impossible to implement.  Actually it would
probably be quite easy for someone who fully understands zsh's process and
signal handling mechanism (unfortunately I do not understand it).  Terminal
signals are sent to all processes whose controlling terminal is the
originating tty.  The problem is that zsh waits for its child using
sigsuspend which all signals bug HUP and CHLD blocked.

There are other things which can be improved.  There is the bug described
in Etc/BUGS interrupting zsh -c 'cat a_long_file | less ; :'.  Also I think
that the pipe synchronization code can also be removed.

Zoltan

Re: signal weirdness fix

[Zoltan Hidvegi]

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;

Re: signal weirdness fix

[Bart Schaefer]

On Jan 11,  6:27pm, Zoltan Hidvegi wrote:
} Subject: Re: signal weirdness fix
}
} 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.
[...]
} 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.

Yes, all of this is exactly right.  For some reason I assumed that this
was common knowledge, or I would have explained it before.

} 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.

Now that part *I* hadn't realized.  Having different pgrp behavior when
monitor is off seems a "why bother?" to me.

} Examining the Linux kernel, and the behaviour of ksh and pdksh it seems
} that only SIGINT, SIGQUIT and SIGTSTP needs this special treatment.

Sounds fine.

} 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.

Yes, that *is* the way it happens on all systems (unless the system itself
is buggy).  That's why zsh is using sigblock or sighold rather than using a
SIG_IGN handler, when it wants to hold some signals.  Once again, I thought
this was common knowledge (well, common to anyone who knew how to block
signals) and that you had empirical evidence that zsh dropped some signals
in spite of it.

I'm now a bit less puzzled by your long message about how buggy zsh's
signal handling is:  It's not quite as buggy as you thought, just badly
organized and under-commented.

} 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 is all absolutely correct.

-- 
Bart Schaefer                             Brass Lantern Enterprises
http://www.well.com/user/barts            http://www.nbn.com/people/lantern

Re: signal weirdness fix

[Richard Coleman]

> I'm now a bit less puzzled by your long message about how buggy zsh's
> signal handling is:  It's not quite as buggy as you thought, just badly
> organized and under-commented.
> 
> } 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.

I agree with Zoltan here.  Zsh executes way too much code in its
signal handlers.  But fixing it is non-trivial.  And it is complicated
by the way we handle `errflag'.  I think it might be easier to change
the way errors are handled first (using setjmp, longjmp), and then
work on the signal handling code.

rc