Re: [Help-smalltalk] [PATCH] libgst: Fix SIGALRM race and cancel a timer before starting a new one

[Paolo Bonzini]

Il 26/09/2014 16:21, Holger Hans Peter Freyther ha scritto:
> From: Holger Hans Peter Freyther <address@hidden>
> 
> The below Smalltalk code could trigger a race condition. We would program
> a timer and then program another one before the first has expired. If the
> original timer expires after we have set signal handler but before the
> syscall to set the new timer was called. We could receive SIGALARM and
> our signal handler would set SIG_DFL as the handler for SIGALARM. When
> the new expiration is set and expires the default handler will terminate
> the process.
> 
> Setting the application to SIG_IGN the handled signal could lead to a
> deadlock as the real expiration is never signalled to the Smalltalk
> side.
> 
> The best approach seems to cancel the timer. Before we have canceled
> the timer we might run through the signal handler and signal the
> original sempahore but that appears to be fine as we will not miss
> an event or revert the signal handler too early. It might be best to
> combine "TimeoutSem initialize" of Delay class>>#handleDelayRequstor
> with the cancelation of the timer.

Thanks, this is okay.  It is racy anyway whether the
previously-registered timer would happen anyway; canceling the timer
solves the race in favor of the new timer.

Paolo

> I have only verified the working of the posix timer (timer_settime)
> and not the old interface.
> 
> Eval [
>         | sem a |
>         sem := Semaphore new.
> 
>         a := [
>                 [
>                 (Delay forMilliseconds: 250) wait.
>                 sem signal.
>                 ] repeat.
>         ] fork.
> 
>         b := [
>                 [
>                 (Delay forMilliseconds: 125) timedWaitOn: sem.
>                 ] repeat.
>         ] fork.
> 
>         c := [
>                 [
>                 (Delay forMilliseconds: 125) timedWaitOn: sem.
>                 ] repeat.
>         ] fork.
> 
>         stdin next.
> ]
> ---
>  libgst/ChangeLog             |  6 ++++++
>  libgst/sysdep.h              |  4 ++++
>  libgst/sysdep/posix/events.c |  1 +
>  libgst/sysdep/posix/timer.c  | 19 +++++++++++++++++++
>  4 files changed, 30 insertions(+)
> 
> diff --git a/libgst/ChangeLog b/libgst/ChangeLog
> index a390754..ce90b25 100644
> --- a/libgst/ChangeLog
> +++ b/libgst/ChangeLog
> @@ -1,3 +1,9 @@
> +2014-09-26  Holger Hans Peter Freyther  <address@hidden>
> +
> +     * sysdep.h: Declare _gst_sigalrm_cancel.
> +     * sysdep/posix/events.c: Call _gst_sigalrm_cancel.
> +     * sysdep/posix/timer.c: Implement _gst_sigalrm_cancel.
> +
>  2014-08-02  Holger Hans Peter Freyther  <address@hidden>
>  
>       * prims.def: Introduce VMpr_Behavior_newInitialize and
> diff --git a/libgst/sysdep.h b/libgst/sysdep.h
> index d872c32..5648ef2 100644
> --- a/libgst/sysdep.h
> +++ b/libgst/sysdep.h
> @@ -108,6 +108,10 @@ extern void _gst_sigvtalrm_every (int deltaMilli,
>                              SigHandler func)
>    ATTRIBUTE_HIDDEN;
>  
> +/* Cancel an established SIGALRM */
> +extern void _gst_sigalrm_cancel (void)
> +  ATTRIBUTE_HIDDEN;
> +
>  /* Establish SIGALRM to be called when the nanosecond clock reaches NSTIME
>     nanoseconds.  */
>  extern void _gst_sigalrm_at (int64_t nsTime)
> diff --git a/libgst/sysdep/posix/events.c b/libgst/sysdep/posix/events.c
> index 2525b37..39d6ea9 100644
> --- a/libgst/sysdep/posix/events.c
> +++ b/libgst/sysdep/posix/events.c
> @@ -200,6 +200,7 @@ void
>  _gst_async_timed_wait (OOP semaphoreOOP,
>                      int64_t milliTime)
>  {
> +  _gst_sigalrm_cancel ();
>    _gst_async_interrupt_wait (semaphoreOOP, SIGALRM);
>    _gst_sigalrm_at (milliTime);
>  }
> diff --git a/libgst/sysdep/posix/timer.c b/libgst/sysdep/posix/timer.c
> index 9c8fe28..c47ae91 100644
> --- a/libgst/sysdep/posix/timer.c
> +++ b/libgst/sysdep/posix/timer.c
> @@ -83,6 +83,25 @@ static mst_Boolean have_timer;
>  #endif
>  
>  void
> +_gst_sigalrm_cancel ()
> +{
> +#ifdef HAVE_TIMER_CREATE
> +  if (have_timer)
> +    {
> +      struct itimerspec value;
> +      memset(&value, 0, sizeof(value));
> +      timer_settime (timer, TIMER_ABSTIME, &value, NULL);
> +    }
> +  else
> +#endif
> +    {
> +      struct itimerval value;
> +      memset(&value, 0, sizeof(value));
> +      setitimer (ITIMER_REAL, &value, (struct itimerval *) 0);
> +    }
> +}
> +
> +void
>  _gst_sigalrm_at (int64_t nsTime)
>  {
>  #ifdef HAVE_TIMER_CREATE
> 

Paolo