Re: [gpsd-dev] [PATCH] Use pselect() to reduce wakeups and avoid race conditions

[Mike Frysinger]

On Friday 13 January 2012 16:06:52 Eckhart Wörner wrote:
> attached patch replaces select() by pselect() in the gpsd main loop. This
> should make gpsd behaving nice when idle and avoids a race condition when
> multiple signals arrive shortly one after another.
> if COMPAT_SELECT is defined, the code should behave exactly as it did
> before, for systems where pselect() isn't available. COMPAT_SELECT needs a
> configuration variable.

the idle issue doesn't require pselect() to address.  you can do that already 
by replacing "tv" with NULL in the select() call.  but the current main logic 
seems to imply that this behavior is somewhat desirable (at least when 
debugging).  perhaps the tv argument should be modified that way ?  if people 
are running in debug mode, then poll every second, otherwise wait in 
definitely.  note: i'm certainly not a gpsd expert, so my quick analysis might 
be off ...

as for the race condition, a little explanation might be in order for people 
not versed on esoteric signal behavior.  signal() has the property that when 
the user specified handler is called, the handler is automatically reset to 
SIG_DFL.  thus there is a race condition from the time someone does `killall -
HUP gpsd` to the time where gpsd's main loop notices and actually processes 
the request.  this window cannot be fully closed even if the user signal 
handler itself calls signal() to setup the handler for future signals.  if, in 
that window of time where the handler has been reset to SIG_DFL, the user 
resends with `killall -HUP gpsd`, then the kernel will go ahead and terminate 
gpsd.  there is no race issue between different signals (e.g. `killall -HUP 
gpsd` && `killall -INT gpsd`) since each signal has its own state.

but fixing this issue doesn't require pselect() either.  simply use sigaction() 
and don't use the SA_RESETHAND flag when registering the handlers.  this should 
be portable enough for the code to not worry about compat defines.

> --- a/gpsd.c
> +++ b/gpsd.c
> 
> +    sigprocmask(SIG_BLOCK, &blockset, &emptyset);

the 3rd arg to sigprocmask() is not an emptyset, it's the old sigset, so the 
variable name needs tweaking.
-mike

signature.asc
Description: This is a digitally signed message part.