Inconsistent signal handling?

Inconsistent signal handling?

[Philippe Troin]

[-- Attachment #1: Type: text/plain, Size: 2508 bytes --]

In init.c, init_signals() says:

    void
    init_signals(void)
    {
        sigchld_mask = signal_mask(SIGCHLD);

        intr();

    #ifndef QDEBUG
        signal_ignore(SIGQUIT);
    #endif

        install_handler(SIGHUP);
        install_handler(SIGCHLD);
    #ifdef SIGWINCH
        install_handler(SIGWINCH);
    #endif
        if (interact) {
            install_handler(SIGALRM);
[1]         signal_ignore(SIGTERM);
        }
        if (jobbing) {
[2]         long ttypgrp;
[2]
[2]         while ((ttypgrp = gettygrp()) != -1 && ttypgrp != mypgrp)
[2]             kill(0, SIGTTIN);
[2]         if (ttypgrp == -1) {
[2]             opts[MONITOR] = 0;
[2]         } else {
[2]             signal_ignore(SIGTTOU);
[2]             signal_ignore(SIGTSTP);
[2]             signal_ignore(SIGTTIN);
[2]             attachtty(mypgrp);
[2]         }
        }
[3]     if (islogin) {
[4]         signal_setmask(signal_mask(0));
        } else if (interact) {
            sigset_t set;

            sigemptyset(&set);
            sigaddset(&set, SIGINT);
            sigaddset(&set, SIGQUIT);
            signal_unblock(set);
        }
    }

My remarks:

 [1] Why do we ignore SIGTERM on interactive sessions? That sounds
     like a bad idea to me.

 [2] See my previous mail <87bs5hc31h.fsf@ceramic.fifi.org>

 [3] Why do we only clean-up the signal mask on login shells? It
     should happen on interactive shells (there can be login
     non-interactive shells, eg. for X11 startup).

 [4] This is not enough to clean-up all inherited signal
     properties. If some signals have been ignored (with
     signal(SIG_IGN)) before zsh was exec()ed, then they are still
     ignored by zsh. exec*() only resets the disposition of signals
     which have a signal handler to SIG_DFL, but leaves SIG_IGNored
     signals as is (this is a POSIX requirement).

I'd suggest changing init_signals to:

void
init_signals(void)
{
    if (interact) {
        int i;
        signal_setmask(signal_mask(0));
        for (i=0; i<NSIG; ++i)
            signal_default(i);
    }
    sigchld_mask = signal_mask(SIGCHLD);

    intr();

#ifndef QDEBUG
    signal_ignore(SIGQUIT);
#endif

    install_handler(SIGHUP);
    install_handler(SIGCHLD);
#ifdef SIGWINCH
    install_handler(SIGWINCH);
#endif
    if (interact) {
        install_handler(SIGALRM);
    }
    if (jobbing) {
        signal_ignore(SIGTTOU);
        signal_ignore(SIGTSTP);
        signal_ignore(SIGTTIN);
    }
}

Patch against 4.0.6 attached.

Phil.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: zsh-4.0.6-signals.patch --]
[-- Type: text/x-patch, Size: 1304 bytes --]

diff -rc zsh-4.0.6.orig/Src/init.c zsh-4.0.6/Src/init.c
*** zsh-4.0.6.orig/Src/init.c	Fri Oct 25 19:43:15 2002
--- zsh-4.0.6/Src/init.c	Fri Oct 25 20:19:02 2002
***************
*** 842,847 ****
--- 842,853 ----
  void
  init_signals(void)
  {
+     if (interact) {
+ 	int i;
+ 	signal_setmask(signal_mask(0));
+ 	for (i=0; i<NSIG; ++i)
+ 	    signal_default(i);
+     }
      sigchld_mask = signal_mask(SIGCHLD);
  
      intr();
***************
*** 857,887 ****
  #endif
      if (interact) {
  	install_handler(SIGALRM);
- 	signal_ignore(SIGTERM);
      }
      if (jobbing) {
! 	long ttypgrp;
! 
! 	while ((ttypgrp = gettygrp()) != -1 && ttypgrp != mypgrp)
! 	    kill(0, SIGTTIN);
! 	if (ttypgrp == -1) {
! 	    opts[MONITOR] = 0;
! 	} else {
! 	    signal_ignore(SIGTTOU);
! 	    signal_ignore(SIGTSTP);
! 	    signal_ignore(SIGTTIN);
! 	    attachtty(mypgrp);
! 	}
!     }
!     if (islogin) {
! 	signal_setmask(signal_mask(0));
!     } else if (interact) {
! 	sigset_t set;
! 
! 	sigemptyset(&set);
! 	sigaddset(&set, SIGINT);
! 	sigaddset(&set, SIGQUIT);
! 	signal_unblock(set);
      }
  }
  
--- 863,873 ----
  #endif
      if (interact) {
  	install_handler(SIGALRM);
      }
      if (jobbing) {
! 	signal_ignore(SIGTTOU);
! 	signal_ignore(SIGTSTP);
! 	signal_ignore(SIGTTIN);
      }
  }
  

Re: Inconsistent signal handling?

[Peter Stephenson]

Philippe Troin wrote:
>  [1] Why do we ignore SIGTERM on interactive sessions? That sounds
>      like a bad idea to me.

We restore SIGTERM to the default in entersubsh(), i.e. for all spawned
programmes.  So the intent seems to be that only the parent shell
ignores it.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

Re: Inconsistent signal handling?

[Philippe Troin]

Peter Stephenson <pws@csr.com> writes:

> Philippe Troin wrote:
> >  [1] Why do we ignore SIGTERM on interactive sessions? That sounds
> >      like a bad idea to me.
> 
> We restore SIGTERM to the default in entersubsh(), i.e. for all spawned
> programmes.  So the intent seems to be that only the parent shell
> ignores it.

Yes, I've never said that programs launched by zsh had SIGTERM blocked
or ignored. But I was wondering what was the rationale behind ignoring
SIGTERM within zsh? I've always been annoyed at not being able to kill
zsh with SIGTERM and having to send SIGHUP (or whatever-signal-of-the-day).

BTW what is your opinion about the rest of the patch?

Phil.

Re: Inconsistent signal handling?

[Peter Stephenson]

Philippe Troin wrote:
> Peter Stephenson <pws@csr.com> writes:
> 
> > Philippe Troin wrote:
> > >  [1] Why do we ignore SIGTERM on interactive sessions? That sounds
> > >      like a bad idea to me.
> > 
> > We restore SIGTERM to the default in entersubsh(), i.e. for all spawned
> > programmes.  So the intent seems to be that only the parent shell
> > ignores it.
> 
> Yes, I've never said that programs launched by zsh had SIGTERM blocked
> or ignored. But I was wondering what was the rationale behind ignoring
> SIGTERM within zsh? I've always been annoyed at not being able to kill
> zsh with SIGTERM and having to send SIGHUP (or whatever-signal-of-the-day).

I suspect it's too old to have a rationale.  I've no personal attachment
to it.

> BTW what is your opinion about the rest of the patch?

As far as I can tell, looks OK.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

Re: Inconsistent signal handling?

[Philippe Troin]

Peter Stephenson <pws@csr.com> writes:

> > BTW what is your opinion about the rest of the patch?
> 
> As far as I can tell, looks OK.

So has it been applied ? :-) 

Phil.

Re: Inconsistent signal handling?

[Peter Stephenson]

Philippe Troin wrote:
> So has it been applied ? :-) 

Just committed it on the main trunk.  We can look for unexpected changes
of behaviour.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

Re: Inconsistent signal handling?

[Philippe Troin]

Peter Stephenson <pws@csr.com> writes:

> Philippe Troin wrote:
> > So has it been applied ? :-) 
> 
> Just committed it on the main trunk.  We can look for unexpected changes
> of behaviour.

Thanks.

I believe that the terminal I/O handling patch is also on the main
trunk. Then there should be no change in behavior (except for the
problem fixed by those two patches, namely: 1. the signal mask is
cleared and all signals are reset to their default disposition on all
interactive sessions; 2. zsh makes sure it is a process group leader
(by creating its own process group if necessary) on interactive
sessions; and 3. zsh no longer ignores SIGTERM).

Phil.

Re: Inconsistent signal handling?

[Zefram]

Philippe Troin wrote:
>3. zsh no longer ignores SIGTERM).

Ignoring SIGTERM by default in interactive shells is long-established
consistent behaviour of all Bourne- and C-shell derivatives.  This should
be retained.  (It means that an errant kill(1), particularly a "kill 0",
is unlikely to accidentally terminate the shell.)

-zefram

Re: Inconsistent signal handling?

[Philippe Troin]

[-- Attachment #1: Type: text/plain, Size: 919 bytes --]

Zefram <zefram@fysh.org> writes:

> Philippe Troin wrote:
> >3. zsh no longer ignores SIGTERM).
> 
> Ignoring SIGTERM by default in interactive shells is long-established
> consistent behaviour of all Bourne- and C-shell derivatives.  This should
> be retained.  (It means that an errant kill(1), particularly a "kill 0",
> is unlikely to accidentally terminate the shell.)

Zefram, you are right. Thanks for pointing this out.

Peter, here is a trivial patch that puts this behavior back.

Index: Src/init.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/init.c,v
retrieving revision 1.28
diff -b -u -r1.28 init.c
--- Src/init.c	8 Nov 2002 10:43:49 -0000	1.28
+++ Src/init.c	12 Nov 2002 23:28:27 -0000
@@ -904,6 +904,7 @@
 #endif
     if (interact) {
 	install_handler(SIGALRM);
+	signal_ignore(SIGTERM);
     }
     if (jobbing) {
 	signal_ignore(SIGTTOU);


Phil.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: zsh-ignore-SIGTERM-on-interactive.patch --]
[-- Type: text/x-patch, Size: 420 bytes --]

Index: Src/init.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/init.c,v
retrieving revision 1.28
diff -b -u -r1.28 init.c
--- Src/init.c	8 Nov 2002 10:43:49 -0000	1.28
+++ Src/init.c	12 Nov 2002 23:28:27 -0000
@@ -904,6 +904,7 @@
 #endif
     if (interact) {
 	install_handler(SIGALRM);
+	signal_ignore(SIGTERM);
     }
     if (jobbing) {
 	signal_ignore(SIGTTOU);