timer: fix race between gf_timer_call_cancel() and gf_timer_proc()

Change-Id: Ie264d3d591352e4a8ddaa90ae2174d9c552396f1
BUG: 1243187
Signed-off-by: Anand Avati <avati@redhat.com>
Reviewed-on: http://review.gluster.org/6459
Reviewed-by: Krutika Dhananjay <kdhananj@redhat.com>
Reviewed-by: Poornima G <pgurusid@redhat.com>
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Niels de Vos <ndevos@redhat.com>

2 files changed, 18 insertions, 37 deletions

diff --git a/libglusterfs/src/timer.c b/libglusterfs/src/timer.c
index b406ef613d6..c3069d6fe18 100644
--- a/libglusterfs/src/timer.c
+++ b/libglusterfs/src/timer.c
@@ -80,31 +80,11 @@ gf_timer_call_after (glusterfs_ctx_t *ctx,
 }
 
 int32_t
-gf_timer_call_stale (gf_timer_registry_t *reg,
-                     gf_timer_t *event)
-{
-        if (reg == NULL || event == NULL)
-        {
-                gf_msg_callingfn ("timer", GF_LOG_ERROR, EINVAL,
-                                  LG_MSG_INVALID_ARG, "invalid argument");
-                return 0;
-        }
-
-        event->next->prev = event->prev;
-        event->prev->next = event->next;
-        event->next = &reg->stale;
-        event->prev = event->next->prev;
-        event->next->prev = event;
-        event->prev->next = event;
-
-        return 0;
-}
-
-int32_t
 gf_timer_call_cancel (glusterfs_ctx_t *ctx,
                       gf_timer_t *event)
 {
         gf_timer_registry_t *reg = NULL;
+        gf_boolean_t fired = _gf_false;
 
         if (ctx == NULL || event == NULL)
         {
@@ -123,13 +103,21 @@ gf_timer_call_cancel (glusterfs_ctx_t *ctx,
 
         pthread_mutex_lock (&reg->lock);
         {
+		fired = event->fired;
+		if (fired)
+			goto unlock;
+
                 event->next->prev = event->prev;
                 event->prev->next = event->next;
         }
+unlock:
         pthread_mutex_unlock (&reg->lock);
 
-        GF_FREE (event);
-        return 0;
+	if (!fired) {
+		GF_FREE (event);
+		return 0;
+        }
+        return -1;
 }
 
 static inline void __delete_entry (gf_timer_t *event) {
@@ -176,7 +164,9 @@ gf_timer_proc (void *ctx)
                                 at = TS (event->at);
                                 if (event != &reg->active && now >= at) {
                                         need_cbk = 1;
-                                        gf_timer_call_stale (reg, event);
+                                        event->next->prev = event->prev;
+                                        event->prev->next = event->next;
+                                        event->fired = _gf_true;
                                 }
                         }
                         pthread_mutex_unlock (&reg->lock);
@@ -187,15 +177,13 @@ gf_timer_proc (void *ctx)
                                         THIS = event->xl;
                                 }
                                 event->callbk (event->data);
-                                /*This callbk above would have freed the event
-                                 * by calling timer_cancel, don't ever touch it
-                                 * again*/
+                                GF_FREE (event);
                                 if (old_THIS) {
                                         THIS = old_THIS;
                                 }
-                        }
-                        else
+                        } else {
                                 break;
+                        }
                 }
                 nanosleep (&sleepts, NULL);
         }
@@ -211,11 +199,6 @@ gf_timer_proc (void *ctx)
                          * list_head*/
                         __delete_entry (event);
                 }
-
-                while (reg->stale.next != &reg->stale) {
-                        event = reg->stale.next;
-                        __delete_entry (event);
-                }
         }
         pthread_mutex_unlock (&reg->lock);
         pthread_mutex_destroy (&reg->lock);
@@ -244,8 +227,6 @@ gf_timer_registry_init (glusterfs_ctx_t *ctx)
                 pthread_mutex_init (&reg->lock, NULL);
                 reg->active.next = &reg->active;
                 reg->active.prev = &reg->active;
-                reg->stale.next = &reg->stale;
-                reg->stale.prev = &reg->stale;
 
                 ctx->timer = reg;
                 gf_thread_create (&reg->th, NULL, gf_timer_proc, ctx);
diff --git a/libglusterfs/src/timer.h b/libglusterfs/src/timer.h
index 220a280c705..fff902814ed 100644
--- a/libglusterfs/src/timer.h
+++ b/libglusterfs/src/timer.h
@@ -24,12 +24,12 @@ struct _gf_timer {
         gf_timer_cbk_t    callbk;
         void             *data;
         xlator_t         *xl;
+	gf_boolean_t      fired;
 };
 
 struct _gf_timer_registry {
         pthread_t        th;
         char             fin;
-        struct _gf_timer stale;
         struct _gf_timer active;
         pthread_mutex_t  lock;
 };