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

From: Holger Hans Peter Freyther <[hidden email]>

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.

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  <[hidden email]>
+
+ * 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  <[hidden email]>
 
  * 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
--
2.1.0


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

Il 26/09/2014 16:21, Holger Hans Peter Freyther ha scritto:
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

Paolo

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

On Fri, Sep 26, 2014 at 04:52:41PM +0200, Paolo Bonzini wrote:

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

do you know that posix make any guarantee that either the signal of
the previous timer has been delivered or the old timer has been
canceled?

For clock monotinic I did find common_timer_set inide the kernel
and it seems that it tries to prevent such thing?



_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

Il 26/09/2014 20:26, Holger Hans Peter Freyther ha scritto:
> do you know that posix make any guarantee that either the signal of
> the previous timer has been delivered or the old timer has been
> canceled?

Yes, definitely.  The old timer must be delivered before
timer_settime/setitimer returns.

Paolo

> For clock monotinic I did find common_timer_set inide the kernel
> and it seems that it tries to prevent such thing?
>
>
>


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk