Re: PATCH: Re: Allowing traps

Re: PATCH: Re: Allowing traps

[Sven Wischnowsky]

Bart Schaefer wrote:

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

Of course I have no objections to use a cleaner solution, but will the 
signal-blocking really allow us to execute more trap handlers
immediately? Considering the many places where concurrent execution is 
unsafe...

And who is going to find all the places where we have to block/unblock 
signals? (To keep the sections with signals blocked short.) Somehow I
envision many extra system calls, but I really haven't tried to make a 
list where we need to keep trap handlers from running, so I may be
wrong...


Bye
 Sven


--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: PATCH: Re: Allowing traps

[Bart Schaefer]

On Nov 23,  9:11am, Sven Wischnowsky wrote:
} 
} Bart Schaefer wrote:
} > 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
} 
} Of course I have no objections to use a cleaner solution, but will the 
} signal-blocking really allow us to execute more trap handlers
} immediately?

The signal queueing mechanism doesn't actually block the signals, it just
adds them to a queue which is processed later.  It never grows the array
used for the queue and it maintains the front and rear as pointers so it
can cycle; thus it never has to realloc() or memcpy(), both of which are
unsafe.

On the other hand, it assumes signals are only queued briefly, so that it
would be extremely unlikely for a large number of them to arrive at once.
You could probably overwhelm it by deliberately bombarding zsh with a lot
of different signals in a tight loop.  We should increase the queue size
if we start using it a lot more widely.

} Considering the many places where concurrent execution is unsafe...

Well, that's a design flaw that we should be addressing, even if only
gradually, rather than something we sweep under the rug by making some
other part of the execution incorrect.

} And who is going to find all the places where we have to block/unblock 
} signals? (To keep the sections with signals blocked short.)

We could start by wrapping every function in mem.c with queue_signals()/
unqueue_signals().  That'd probably take care of a lot of the problems.

} Somehow I envision many extra system calls

The signal queueing mechanism only makes system calls if a signal actually
gets queued; then it emulates the signal masking that the OS would set up
during the signal handler call, when it finally does call the handler.

We only need to block signals when OS-level side-effects [such as read()
giving EINTR] must be prevented.

} but I really haven't tried to make a list where we need to keep trap
} handlers from running, so I may be wrong...

The short answer would be "everywhere that we manipulate global variables
that are pointers."  That's a bit strong, as simple dereferencing should
be OK.  Based on a grep of *.(c|h|e#pro) files, there appear to be at least
234 such variables (including structs that may contain pointers, though I
may have missed some of those), of which 44 are static and 57 are part of
the completion system.  Another 35 are part of ZLE.

So 25% of the global pointers are in the completion system, and I'd be
willing to bet that it modifies its globals a lot more heavily than just
about anything except the memory allocators.  That makes the completion
system an awfully large target for an interactive shell that's receiving
SIGALRM once every second; no wonder Thomas is seeing problems.

The other pesky bits are the parameter tables, which could mostly be
covered by doctoring params.c.

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

[Peter Stephenson]

Bart wrote:
> We could start by wrapping every function in mem.c with queue_signals()/
> unqueue_signals().  That'd probably take care of a lot of the problems.

We did something pretty similar some time ago (1992?) --- the performance
hit was ghastly, so we removed it.

-- 
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: PATCH: Re: Allowing traps

[Bart Schaefer]

On Nov 23,  9:42pm, Peter Stephenson wrote:
} Subject: Re: PATCH: Re: Allowing traps
}
} Bart wrote:
} > We could start by wrapping every function in mem.c with queue_signals()/
} > unqueue_signals().  That'd probably take care of a lot of the problems.
} 
} We did something pretty similar some time ago (1992?) --- the performance
} hit was ghastly, so we removed it.

The something similar was to actually block and unblock the signals, which
meant two system calls on every operation; queue_signals() does nothing but
increment/decrement a counter unless a signal actually arrives.  

I definitely am not recommending blocking and unblocking signals here.

-- 
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 23,  9:42pm, Peter Stephenson wrote:
}
} Bart wrote:
} > We could start by wrapping every function in mem.c with queue_signals()/
} > unqueue_signals().
} 
} We did something pretty similar some time ago (1992?) --- the performance
} hit was ghastly, so we removed it.

You'll note that the ZSH_MEM malloc() and zhalloc() both already have some
sections in queue_signals().  What I'm suggesting is that other operations
that manipulate the heap pointers should be similarly protected.


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

[Sven Wischnowsky]

Bart Schaefer wrote:

> ...
> 
> The short answer would be "everywhere that we manipulate global variables
> that are pointers."  That's a bit strong, as simple dereferencing should
> be OK.  Based on a grep of *.(c|h|e#pro) files, there appear to be at least
> 234 such variables (including structs that may contain pointers, though I
> may have missed some of those), of which 44 are static and 57 are part of
> the completion system.  Another 35 are part of ZLE.
> 
> So 25% of the global pointers are in the completion system, and I'd be
> willing to bet that it modifies its globals a lot more heavily than just
> about anything except the memory allocators.  That makes the completion
> system an awfully large target for an interactive shell that's receiving
> SIGALRM once every second; no wonder Thomas is seeing problems.
> 
> The other pesky bits are the parameter tables, which could mostly be
> covered by doctoring params.c.

Judging from the debugging output I got from Thomas I think it was
this part that finally caused the SEGVs (if I remember correctly).

And hence I'm not convinced that the completion code (note: `code',
not `system') is really a problem. In fact, it should be easy to make
safe once the other code is safe. Because nowadays we have only very
few entry points to the completion code that can be guarded to
disallow invoking the code while it is still being executed (i.e. make 
sure that trap handlers can't call it -- that wouldn't make much sense 
anyway, I think).

And later we can put all the completion state in a struct and use that 
to make it reentrant. Something I've been wishing to have for the
lexing, parsing and execution code for years (there we use structs to
safe and restore the state, copying it from/to global variables).

Bye
 Sven


--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

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