libglusterfs (timer): race conditions, illegal mem access, mem leak

While investigating gfapi memory consumption with valgrind, valgrind
reported several memory access issues.

Also see the timer 'registry' being recreated (shortly) after being
freed during teardown due to the way it's currently written.

Passing ctx as data to gf_timer_proc() is prone to memory access
issues if ctx is freed before gf_timer_proc() terminates. (And in
fact this does happen, at least in valgrind.) gf_timer_proc() doesn't
need ctx for anything, it only needs ctx->timer, so just pass that.

Nothing ever calls gf_timer_registry_init(). Nothing outside of
timer.c that is. Making it and gf_timer_proc() static.

backport mainline:
> http://review.gluster.org/14247
> BUG: 1333925

Change-Id: Ia28454dda0cf0de2fec94d76441d98c3927a906a
BUG: 1342620
Signed-off-by: Kaleb S KEITHLEY <kkeithle@redhat.com>
Reviewed-on: http://review.gluster.org/14644
Smoke: Gluster Build System <jenkins@build.gluster.com>
NetBSD-regression: NetBSD Build System <jenkins@build.gluster.org>
CentOS-regression: Gluster Build System <jenkins@build.gluster.com>

1 files changed, 71 insertions, 57 deletions

diff --git a/libglusterfs/src/timer.c b/libglusterfs/src/timer.c
index a072985acce..bcdc5da5571 100644
--- a/libglusterfs/src/timer.c
+++ b/libglusterfs/src/timer.c
@@ -15,6 +15,10 @@
 #include "timespec.h"
 #include "libglusterfs-messages.h"
 
+/* fwd decl */
+static gf_timer_registry_t *
+gf_timer_registry_init (glusterfs_ctx_t *);
+
 gf_timer_t *
 gf_timer_call_after (glusterfs_ctx_t *ctx,
                      struct timespec delta,
@@ -33,17 +37,6 @@ gf_timer_call_after (glusterfs_ctx_t *ctx,
                 return NULL;
         }
 
-        /* ctx and its fields are not accessed inside mutex!?
-         * TODO: Even with this there is a possiblity of race
-         * when cleanup_started is set after checking for it
-         */
-        if (ctx->cleanup_started) {
-                gf_msg_callingfn ("timer", GF_LOG_INFO, 0,
-                                  LG_MSG_CTX_CLEANUP_STARTED, "ctx cleanup "
-                                  "started");
-                return NULL;
-        }
-
         reg = gf_timer_registry_init (ctx);
 
         if (!reg) {
@@ -62,7 +55,7 @@ gf_timer_call_after (glusterfs_ctx_t *ctx,
         event->callbk = callbk;
         event->data = data;
         event->xl = THIS;
-        pthread_mutex_lock (&reg->lock);
+        LOCK (&reg->lock);
         {
                 trav = reg->active.prev;
                 while (trav != &reg->active) {
@@ -75,10 +68,11 @@ gf_timer_call_after (glusterfs_ctx_t *ctx,
                 event->prev->next = event;
                 event->next->prev = event;
         }
-        pthread_mutex_unlock (&reg->lock);
+        UNLOCK (&reg->lock);
         return event;
 }
 
+
 int32_t
 gf_timer_call_cancel (glusterfs_ctx_t *ctx,
                       gf_timer_t *event)
@@ -93,7 +87,12 @@ gf_timer_call_cancel (glusterfs_ctx_t *ctx,
                 return 0;
         }
 
-        reg = gf_timer_registry_init (ctx);
+        LOCK (&ctx->lock);
+        {
+                reg = ctx->timer;
+        }
+        UNLOCK (&ctx->lock);
+
         if (!reg) {
                 gf_msg ("timer", GF_LOG_ERROR, 0, LG_MSG_INIT_TIMER_FAILED,
                         "!reg");
@@ -101,53 +100,42 @@ gf_timer_call_cancel (glusterfs_ctx_t *ctx,
                 return 0;
         }
 
-        pthread_mutex_lock (&reg->lock);
+        LOCK (&reg->lock);
         {
-		fired = event->fired;
-		if (fired)
-			goto unlock;
+                fired = event->fired;
+                if (fired)
+                        goto unlock;
 
                 event->next->prev = event->prev;
                 event->prev->next = event->next;
         }
 unlock:
-        pthread_mutex_unlock (&reg->lock);
+        UNLOCK (&reg->lock);
 
-	if (!fired) {
-		GF_FREE (event);
-		return 0;
+        if (!fired) {
+                GF_FREE (event);
+                return 0;
         }
         return -1;
 }
 
-static void __delete_entry (gf_timer_t *event) {
+
+static void
+__delete_entry (gf_timer_t *event) {
         event->next->prev = event->prev;
         event->prev->next = event->next;
         GF_FREE (event);
 }
 
-void *
-gf_timer_proc (void *ctx)
+
+static void *
+gf_timer_proc (void *data)
 {
-        gf_timer_registry_t *reg = NULL;
+        gf_timer_registry_t *reg = data;
         const struct timespec sleepts = {.tv_sec = 1, .tv_nsec = 0, };
         gf_timer_t *event = NULL;
         xlator_t   *old_THIS = NULL;
 
-        if (ctx == NULL)
-        {
-                gf_msg_callingfn ("timer", GF_LOG_ERROR, EINVAL,
-                                  LG_MSG_INVALID_ARG, "invalid argument");
-                return NULL;
-        }
-
-        reg = gf_timer_registry_init (ctx);
-        if (!reg) {
-                gf_msg ("timer", GF_LOG_ERROR, 0, LG_MSG_INIT_TIMER_FAILED,
-                        "!reg");
-                return NULL;
-        }
-
         while (!reg->fin) {
                 uint64_t now;
                 struct timespec now_ts;
@@ -158,7 +146,7 @@ gf_timer_proc (void *ctx)
                         uint64_t at;
                         char need_cbk = 0;
 
-                        pthread_mutex_lock (&reg->lock);
+                        LOCK (&reg->lock);
                         {
                                 event = reg->active.next;
                                 at = TS (event->at);
@@ -169,7 +157,7 @@ gf_timer_proc (void *ctx)
                                         event->fired = _gf_true;
                                 }
                         }
-                        pthread_mutex_unlock (&reg->lock);
+                        UNLOCK (&reg->lock);
                         if (need_cbk) {
                                 old_THIS = NULL;
                                 if (event->xl) {
@@ -188,7 +176,7 @@ gf_timer_proc (void *ctx)
                 nanosleep (&sleepts, NULL);
         }
 
-        pthread_mutex_lock (&reg->lock);
+        LOCK (&reg->lock);
         {
                 /* Do not call gf_timer_call_cancel(),
                  * it will lead to deadlock
@@ -200,42 +188,58 @@ gf_timer_proc (void *ctx)
                         __delete_entry (event);
                 }
         }
-        pthread_mutex_unlock (&reg->lock);
-        pthread_mutex_destroy (&reg->lock);
-        GF_FREE (((glusterfs_ctx_t *)ctx)->timer);
+        UNLOCK (&reg->lock);
+        LOCK_DESTROY (&reg->lock);
 
         return NULL;
 }
 
-gf_timer_registry_t *
+
+static gf_timer_registry_t *
 gf_timer_registry_init (glusterfs_ctx_t *ctx)
 {
+        gf_timer_registry_t *reg = NULL;
+
         if (ctx == NULL) {
                 gf_msg_callingfn ("timer", GF_LOG_ERROR, EINVAL,
                                   LG_MSG_INVALID_ARG, "invalid argument");
                 return NULL;
         }
 
-        if (!ctx->timer) {
-                gf_timer_registry_t *reg = NULL;
+        if (ctx->cleanup_started) {
+                gf_msg_callingfn ("timer", GF_LOG_INFO, 0,
+                                  LG_MSG_CTX_CLEANUP_STARTED,
+                                  "ctx cleanup started");
+                return NULL;
+        }
 
+        LOCK (&ctx->lock);
+        {
+                reg = ctx->timer;
+        }
+        UNLOCK (&ctx->lock);
+        if (!reg) {
                 reg = GF_CALLOC (1, sizeof (*reg),
                                  gf_common_mt_gf_timer_registry_t);
                 if (!reg)
-                        goto out;
+                        return NULL;
 
-                pthread_mutex_init (&reg->lock, NULL);
+                LOCK_INIT (&reg->lock);
                 reg->active.next = &reg->active;
                 reg->active.prev = &reg->active;
 
-                ctx->timer = reg;
-                gf_thread_create (&reg->th, NULL, gf_timer_proc, ctx);
-
+                LOCK (&ctx->lock);
+                {
+                        ctx->timer = reg;
+                }
+                UNLOCK (&ctx->lock);
+                gf_thread_create (&reg->th, NULL, gf_timer_proc, reg);
         }
-out:
-        return ctx->timer;
+
+        return reg;
 }
 
+
 void
 gf_timer_registry_destroy (glusterfs_ctx_t *ctx)
 {
@@ -245,8 +249,18 @@ gf_timer_registry_destroy (glusterfs_ctx_t *ctx)
         if (ctx == NULL)
                 return;
 
-        reg = ctx->timer;
+        LOCK (&ctx->lock);
+        {
+                reg = ctx->timer;
+                ctx->timer = NULL;
+        }
+        UNLOCK (&ctx->lock);
+
+        if (!reg)
+                return;
+
         thr_id = reg->th;
         reg->fin = 1;
         pthread_join (thr_id, NULL);
+        GF_FREE (reg);
 }