Allowing traps

Allowing traps

[Peter Stephenson]

It's currently possible, even with the rewritten trap code, for a trap to
be called inside a trap.  For example, if a trap includes a `read', another
trap could be called at that point (whether it is already in the queue, or
the signal has arrived since the trap started).  This is presumably
unhealthy --- at least, I have some shell code using the new trap behaviour
which gets upset if instances of it are not called sequentially.  Should we
add an extra flag to block traps explicitly from running when we are inside
doqueuedtraps()?

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
Cambridge Silicon Radio, Unit 300, Science Park, Milton Road,
Cambridge, CB4 0XL, UK                          Tel: +44 (0)1223 392070

Re: Allowing traps

[Bart Schaefer]

On Nov 20,  4:09pm, Peter Stephenson wrote:
}
} I have some shell code using the new trap behaviour which gets upset
} if instances of it are not called sequentially. Should we add an
} extra flag to block traps explicitly from running when we are inside
} doqueuedtraps()?

We don't need a flag ... the behavior should be that for all xxx, SIGxxx is
blocked when running the trap for that signal.  The OS will enforce that if
the trap is run directly from the signal handler, so we should emulate it
when running the traps in deferred fashion.

This could still cause signals to be received in a different order than
they would otherwise have been, which is one reason I would have preferred
to have reentrancy or limited critical sections rather than deferrals.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: Allowing traps

[Peter Stephenson]

Bart wrote:
> We don't need a flag ... the behavior should be that for all xxx, SIGxxx is
> blocked when running the trap for that signal.  The OS will enforce that if
> the trap is run directly from the signal handler, so we should emulate it
> when running the traps in deferred fashion.

That doesn't fix the problem that the shell will try and run traps already
in the queue from within shell code called from the trap handler --- which
may be the trap for another signal.  I don't see a way of stopping that
without a flag.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
Cambridge Silicon Radio, Unit 300, Science Park, Milton Road,
Cambridge, CB4 0XL, UK                          Tel: +44 (0)1223 392070

PATCH: Re: Allowing traps

[Bart Schaefer]

On Nov 20,  4:54pm, Peter Stephenson wrote:
} Subject: Re: Allowing traps
}
} Bart wrote:
} > We don't need a flag ... the behavior should be that for all xxx, SIGxxx is
} > blocked when running the trap for that signal.
} 
} That doesn't fix the problem that the shell will try and run traps already
} in the queue from within shell code called from the trap handler

Ahh, I see.  Yes, that is a problem.

} I don't see a way of stopping that without a flag.

It looks to me like there's already a bad feedback loop between dotrap()
and doqueuedtraps().

  doqueuedtraps() --> dotrap(sig, -1) --> RUNTRAPS() --> doqueuedtraps()

That's going to cause the queue to be unwound as a stack, isn't it?  What
am I missing?

The patch below should fix both of those problems, I think.

There's still one more problem, which is that it might be possible for a
trap to get queued while it's not ignored, but then become ignored before
the queue runs.  I think any trap that makes it into the queue ought to
get executed, regardless of whether it's ignored at the time the queue is
processed.  We could probably fix this by inserting a call to RUNTRAPS()
in unsettrap().

I'm still a bit concerned that there's going to be a bad interaction between
queued signals and queued traps.  Isn't there some way we could use the
signal queuing mechanism to deal with the trap issue, so that there's only
one queue?

Index: Src/signals.c
===================================================================
@@ -1010,12 +1010,12 @@
 {
     int sig, ota = trapsallowed;
 
-    trapsallowed = 1;
+    trapsallowed = 0;
     while (trapqused) {
 	trapqused--;
 	sig = *trapqueue;
 	memcpy(trapqueue, trapqueue + 1, trapqused * sizeof(int));
-	dotrap(sig, -1);
+	dotrap(sig, 1);
     }
     trapsallowed = ota;
 }

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: PATCH: Re: Allowing traps

[Bart Schaefer]

On Nov 20,  5:39pm, Bart Schaefer wrote:
} Subject: PATCH: Re: Allowing traps
}
} On Nov 20,  4:54pm, Peter Stephenson wrote:
} } That doesn't fix the problem that the shell will try and run traps already
} } in the queue from within shell code called from the trap handler
} 
} Yes, that is a problem.

I was about to commit the patch when I began to get really worried about
the whole trap-queuing idea.

My biggest concern is that a trap that calls `exit' or `return' will be
queued, thereby allowing code that should not have run to execute before
the trap queue is processed.  (My patch would actually make this more
likely, which is what stopped me committing it.)

Traps are being queued during most of the time the shell is running, and
for builtins they're not processed until after each command finishes
executing [unless it does I/O with ztrapread/ztrapwrite].  For signals
like SIGINT, that when not trapped set flags which may cause commands to
be interrupted early, the behavior may become *very* different if the
trap is queued; consider:

	trap 'trap - 2; kill -2 $$' 2

This is especially worrisome in non-interactive shells, which potentially
call ztrapread/ztrapwrite much less often.  Consider too the number of
places where zsh does I/O with stdio functions, during which traps are
now (as of 13108) not executed.

} There's still one more problem, which is that it might be possible for a
} trap to get queued while it's not ignored, but then become ignored before
} the queue runs.
} 
} I'm still a bit concerned that there's going to be a bad interaction between
} queued signals and queued traps.

This is what it comes down to:  The problem only occurs with signals that
can arrive asynchronously.  We already have the signal queueing code to
handle that case; if it needs to be applied more widely, we should do that,
but I no longer believe that a blanket trap-handler-queue is a good idea.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net