summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorKaleb S KEITHLEY <kkeithle@redhat.com>2016-05-06 13:04:38 -0400
committerJeff Darcy <jdarcy@redhat.com>2016-06-01 06:50:13 -0700
commit24dd33929bbbc9a72360793048f17bf4e6cec8a3 (patch)
treecaa5c51a587252ff222ae885514a70a19ca05a1d
parenta04eaf366779a0632e5b9cdd6d63de0eb62f7449 (diff)
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. Change-Id: Ia28454dda0cf0de2fec94d76441d98c3927a906a BUG: 1333925 Signed-off-by: Kaleb S KEITHLEY <kkeithle@redhat.com> Reviewed-on: http://review.gluster.org/14247 NetBSD-regression: NetBSD Build System <jenkins@build.gluster.org> Smoke: Gluster Build System <jenkins@build.gluster.com> CentOS-regression: Gluster Build System <jenkins@build.gluster.com> Reviewed-by: Poornima G <pgurusid@redhat.com> Reviewed-by: Niels de Vos <ndevos@redhat.com> Reviewed-by: Jeff Darcy <jdarcy@redhat.com>
-rw-r--r--api/src/glfs.c8
-rw-r--r--cli/src/cli.c2
-rw-r--r--glusterfsd/src/glusterfsd.c2
-rw-r--r--libglusterfs/src/ctx.c3
-rw-r--r--libglusterfs/src/glusterfs.h3
-rw-r--r--libglusterfs/src/timer.c128
-rw-r--r--libglusterfs/src/timer.h10
-rw-r--r--rpc/rpc-transport/rdma/src/rdma.c4
-rw-r--r--xlators/features/changelog/lib/src/gf-changelog.c2
9 files changed, 86 insertions, 76 deletions
diff --git a/api/src/glfs.c b/api/src/glfs.c
index 0d67ce1a86c..1cb6bf7a3cf 100644
--- a/api/src/glfs.c
+++ b/api/src/glfs.c
@@ -149,7 +149,7 @@ glusterfs_ctx_defaults_init (glusterfs_ctx_t *ctx)
LOCK_INIT (&pool->lock);
ctx->pool = pool;
- pthread_mutex_init (&(ctx->lock), NULL);
+ LOCK_INIT (&ctx->lock);
ret = 0;
err:
@@ -1059,9 +1059,9 @@ glusterfs_ctx_destroy (glusterfs_ctx_t *ctx)
GF_FREE (ctx->process_uuid);
GF_FREE (ctx->cmd_args.volfile_id);
- pthread_mutex_destroy (&(ctx->lock));
- pthread_mutex_destroy (&(ctx->notify_lock));
- pthread_cond_destroy (&(ctx->notify_cond));
+ LOCK_DESTROY (&ctx->lock);
+ pthread_mutex_destroy (&ctx->notify_lock);
+ pthread_cond_destroy (&ctx->notify_cond);
/* Free all the graph structs and its containing xlator_t structs
* from this point there should be no reference to GF_FREE/GF_CALLOC
diff --git a/cli/src/cli.c b/cli/src/cli.c
index 923748b4594..518ae265f22 100644
--- a/cli/src/cli.c
+++ b/cli/src/cli.c
@@ -154,7 +154,7 @@ glusterfs_ctx_defaults_init (glusterfs_ctx_t *ctx)
LOCK_INIT (&pool->lock);
ctx->pool = pool;
- pthread_mutex_init (&(ctx->lock), NULL);
+ LOCK_INIT (&ctx->lock);
cmd_args = &ctx->cmd_args;
diff --git a/glusterfsd/src/glusterfsd.c b/glusterfsd/src/glusterfsd.c
index d8bc01337b7..0cf1763bdf0 100644
--- a/glusterfsd/src/glusterfsd.c
+++ b/glusterfsd/src/glusterfsd.c
@@ -1493,7 +1493,7 @@ glusterfs_ctx_defaults_init (glusterfs_ctx_t *ctx)
if (!ctx->logbuf_pool)
goto out;
- pthread_mutex_init (&(ctx->lock), NULL);
+ LOCK_INIT (&ctx->lock);
pthread_mutex_init (&ctx->notify_lock, NULL);
pthread_cond_init (&ctx->notify_cond, NULL);
diff --git a/libglusterfs/src/ctx.c b/libglusterfs/src/ctx.c
index c70d97bc5d4..2aa14654b9e 100644
--- a/libglusterfs/src/ctx.c
+++ b/libglusterfs/src/ctx.c
@@ -35,7 +35,8 @@ glusterfs_ctx_new ()
ctx->daemon_pipe[0] = -1;
ctx->daemon_pipe[1] = -1;
- ret = pthread_mutex_init (&ctx->lock, NULL);
+ /* lock is never destroyed! */
+ ret = LOCK_INIT (&ctx->lock);
if (ret) {
free (ctx);
ctx = NULL;
diff --git a/libglusterfs/src/glusterfs.h b/libglusterfs/src/glusterfs.h
index d5fde68d76a..610dfc9e7d9 100644
--- a/libglusterfs/src/glusterfs.h
+++ b/libglusterfs/src/glusterfs.h
@@ -32,6 +32,7 @@
#include "glusterfs-fops.h" /* generated XDR values for FOPs */
#include "list.h"
+#include "locking.h"
#include "logging.h"
#include "lkowner.h"
#include "compat-uuid.h"
@@ -428,7 +429,7 @@ struct _glusterfs_ctx {
void *event_pool;
void *iobuf_pool;
void *logbuf_pool;
- pthread_mutex_t lock;
+ gf_lock_t lock;
size_t page_size;
struct list_head graphs; /* double linked list of graphs - one per volfile parse */
glusterfs_graph_t *active; /* the latest graph in use */
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);
}
diff --git a/libglusterfs/src/timer.h b/libglusterfs/src/timer.h
index fff902814ed..0224b0897f9 100644
--- a/libglusterfs/src/timer.h
+++ b/libglusterfs/src/timer.h
@@ -20,7 +20,7 @@ typedef void (*gf_timer_cbk_t) (void *);
struct _gf_timer {
struct _gf_timer *next, *prev;
- struct timespec at;
+ struct timespec at;
gf_timer_cbk_t callbk;
void *data;
xlator_t *xl;
@@ -31,7 +31,7 @@ struct _gf_timer_registry {
pthread_t th;
char fin;
struct _gf_timer active;
- pthread_mutex_t lock;
+ gf_lock_t lock;
};
typedef struct _gf_timer gf_timer_t;
@@ -47,12 +47,6 @@ int32_t
gf_timer_call_cancel (glusterfs_ctx_t *ctx,
gf_timer_t *event);
-void *
-gf_timer_proc (void *data);
-
-gf_timer_registry_t *
-gf_timer_registry_init (glusterfs_ctx_t *ctx);
-
void
gf_timer_registry_destroy (glusterfs_ctx_t *ctx);
#endif /* _TIMER_H */
diff --git a/rpc/rpc-transport/rdma/src/rdma.c b/rpc/rpc-transport/rdma/src/rdma.c
index 1f397e42206..551ac072079 100644
--- a/rpc/rpc-transport/rdma/src/rdma.c
+++ b/rpc/rpc-transport/rdma/src/rdma.c
@@ -4641,7 +4641,7 @@ gf_rdma_init (rpc_transport_t *this)
pthread_mutex_init (&priv->recv_mutex, NULL);
pthread_cond_init (&priv->recv_cond, NULL);
- pthread_mutex_lock (&ctx->lock);
+ LOCK (&ctx->lock);
{
if (ctx->ib == NULL) {
ctx->ib = __gf_rdma_ctx_create ();
@@ -4650,7 +4650,7 @@ gf_rdma_init (rpc_transport_t *this)
}
}
}
- pthread_mutex_unlock (&ctx->lock);
+ UNLOCK (&ctx->lock);
return ret;
}
diff --git a/xlators/features/changelog/lib/src/gf-changelog.c b/xlators/features/changelog/lib/src/gf-changelog.c
index 165703d3aa6..75891635827 100644
--- a/xlators/features/changelog/lib/src/gf-changelog.c
+++ b/xlators/features/changelog/lib/src/gf-changelog.c
@@ -145,7 +145,7 @@ gf_changelog_ctx_defaults_init (glusterfs_ctx_t *ctx)
LOCK_INIT (&pool->lock);
ctx->pool = pool;
- pthread_mutex_init (&(ctx->lock), NULL);
+ LOCK_INIT (&ctx->lock);
cmd_args = &ctx->cmd_args;