[PATCH] a few fixes for tevent

Volker Lendecke Volker.Lendecke at SerNet.DE
Tue Jun 6 08:08:11 UTC 2017


Hi!

Review appreciated!

Thanks, Volker

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de
-------------- next part --------------
From 637ea470b5266a850c8387e1deefb78173ac0f6e Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 5 Jun 2017 07:23:27 +0200
Subject: [PATCH 1/6] tevent: Fix a typo

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 lib/tevent/tevent.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/tevent/tevent.h b/lib/tevent/tevent.h
index ba4bb4d..728cf62 100644
--- a/lib/tevent/tevent.h
+++ b/lib/tevent/tevent.h
@@ -1777,7 +1777,7 @@ void tevent_thread_proxy_schedule(struct tevent_thread_proxy *tp,
  *
  * It is the duty of the caller of tevent_threaded_context_create() to
  * keep the event context around longer than any
- * tevent_threaded_context. tevent will abort if ev is talllc_free'ed
+ * tevent_threaded_context. tevent will abort if ev is talloc_free'ed
  * with an active tevent_threaded_context.
  *
  * If tevent is build without pthread support, this always returns
-- 
2.1.4


From 16a0cf653338799b0b955ae95da0cbfb43f2129a Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 5 Jun 2017 06:58:37 +0200
Subject: [PATCH 2/6] tevent: Factor out context initialization

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 lib/tevent/tevent.c | 59 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 25 deletions(-)

diff --git a/lib/tevent/tevent.c b/lib/tevent/tevent.c
index 65b101f..68b9db6 100644
--- a/lib/tevent/tevent.c
+++ b/lib/tevent/tevent.c
@@ -392,46 +392,26 @@ int tevent_common_context_destructor(struct tevent_context *ev)
 	return 0;
 }
 
-/*
-  create a event_context structure for a specific implemementation.
-  This must be the first events call, and all subsequent calls pass
-  this event_context as the first element. Event handlers also
-  receive this as their first argument.
-
-  This function is for allowing third-party-applications to hook in gluecode
-  to their own event loop code, so that they can make async usage of our client libs
-
-  NOTE: use tevent_context_init() inside of samba!
-*/
-struct tevent_context *tevent_context_init_ops(TALLOC_CTX *mem_ctx,
-					       const struct tevent_ops *ops,
-					       void *additional_data)
+static int tevent_common_context_constructor(struct tevent_context *ev)
 {
-	struct tevent_context *ev;
 	int ret;
 
-	ev = talloc_zero(mem_ctx, struct tevent_context);
-	if (!ev) return NULL;
-
 #ifdef HAVE_PTHREAD
 
 	ret = pthread_once(&tevent_atfork_initialized, tevent_prep_atfork);
 	if (ret != 0) {
-		talloc_free(ev);
-		return NULL;
+		return ret;
 	}
 
 	ret = pthread_mutex_init(&ev->scheduled_mutex, NULL);
 	if (ret != 0) {
-		talloc_free(ev);
-		return NULL;
+		return ret;
 	}
 
 	ret = pthread_mutex_lock(&tevent_contexts_mutex);
 	if (ret != 0) {
 		pthread_mutex_destroy(&ev->scheduled_mutex);
-		talloc_free(ev);
-		return NULL;
+		return ret;
 	}
 
 	DLIST_ADD(tevent_contexts, ev);
@@ -440,11 +420,40 @@ struct tevent_context *tevent_context_init_ops(TALLOC_CTX *mem_ctx,
 	if (ret != 0) {
 		abort();
 	}
-
 #endif
 
 	talloc_set_destructor(ev, tevent_common_context_destructor);
 
+	return 0;
+}
+
+/*
+  create a event_context structure for a specific implemementation.
+  This must be the first events call, and all subsequent calls pass
+  this event_context as the first element. Event handlers also
+  receive this as their first argument.
+
+  This function is for allowing third-party-applications to hook in gluecode
+  to their own event loop code, so that they can make async usage of our client libs
+
+  NOTE: use tevent_context_init() inside of samba!
+*/
+struct tevent_context *tevent_context_init_ops(TALLOC_CTX *mem_ctx,
+					       const struct tevent_ops *ops,
+					       void *additional_data)
+{
+	struct tevent_context *ev;
+	int ret;
+
+	ev = talloc_zero(mem_ctx, struct tevent_context);
+	if (!ev) return NULL;
+
+	ret = tevent_common_context_constructor(ev);
+	if (ret != 0) {
+		talloc_free(ev);
+		return NULL;
+	}
+
 	ev->ops = ops;
 	ev->additional_data = additional_data;
 
-- 
2.1.4


From aaa49690683b586793ab6a4a02d97d1e69ded8c6 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 5 Jun 2017 07:16:17 +0200
Subject: [PATCH 3/6] tevent: Re-init threading in tevent_re_initialise

Without this threading is not usable after that call

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 lib/tevent/tevent.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/tevent/tevent.c b/lib/tevent/tevent.c
index 68b9db6..7d59c8b 100644
--- a/lib/tevent/tevent.c
+++ b/lib/tevent/tevent.c
@@ -884,6 +884,8 @@ int tevent_re_initialise(struct tevent_context *ev)
 {
 	tevent_common_context_destructor(ev);
 
+	tevent_common_context_constructor(ev);
+
 	return ev->ops->context_init(ev);
 }
 
-- 
2.1.4


From aac8ff62759821cc91b042e7be1f5e979d53fbad Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 5 Jun 2017 07:29:11 +0200
Subject: [PATCH 4/6] tevent: Add tevent_re_initialise to threaded test

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 lib/tevent/testsuite.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lib/tevent/testsuite.c b/lib/tevent/testsuite.c
index 4783ab4..ee29e5b 100644
--- a/lib/tevent/testsuite.c
+++ b/lib/tevent/testsuite.c
@@ -1207,6 +1207,14 @@ static bool test_multi_tevent_threaded_2(struct torture_context *test,
 	ev = tevent_context_init(test);
 	torture_assert(test, ev != NULL, "tevent_context_init failed");
 
+	/*
+	 * tevent_re_initialise used to have a bug where it did not
+	 * re-initialise the thread support after taking it
+	 * down. Excercise that code path.
+	 */
+	ret = tevent_re_initialise(ev);
+	torture_assert(test, ret == 0, "tevent_re_initialise failed");
+
 	tctx = tevent_threaded_context_create(ev, ev);
 	torture_assert(test, tctx != NULL,
 		       "tevent_threaded_context_create failed");
-- 
2.1.4


From 6c68c8cd8c2ecb128fe3376d33e2da36f4bea889 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Wed, 24 May 2017 16:21:40 +0200
Subject: [PATCH 5/6] tevent: Fix a memleak on FreeBSD

FreeBSD has malloc'ed memory attached to mutexes. We need to clean this up.

valgrind really helped here

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 lib/tevent/tevent.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/tevent/tevent.c b/lib/tevent/tevent.c
index 7d59c8b..f3b18a1 100644
--- a/lib/tevent/tevent.c
+++ b/lib/tevent/tevent.c
@@ -341,6 +341,11 @@ int tevent_common_context_destructor(struct tevent_context *ev)
 
 		DLIST_REMOVE(ev->threaded_contexts, tctx);
 	}
+
+	ret = pthread_mutex_destroy(&ev->scheduled_mutex);
+	if (ret != 0) {
+		abort();
+	}
 #endif
 
 	tevent_common_wakeup_fini(ev);
-- 
2.1.4


From 0cf3f5c266789640659f751024bfe539bd16a383 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Wed, 24 May 2017 16:22:34 +0200
Subject: [PATCH 6/6] tevent: Fix a race condition in tevent context rundown

We protect setting tctx->event_ctx=NULL with tctx->event_ctx_mutex.
But in _tevent_threaded_schedule_immediate we have the classic
TOCTOU race: After we checked "ev==NULL", looking at
tevent_common_context_destructor the event context can go after
_tevent_threaded_schedule_immediate checked. We need to serialize
things a bit by keeping tctx->event_ctx_mutex locked while we
reference "ev", in particular in the

DLIST_ADD_END(ev->scheduled_immediates,im);

I think the locking hierarchy is still maintained, tevent_atfork_prepare()
first locks all the tctx locks, and then the scheduled_mutex.  Also,
I don't think this will impact parallelism too badly: event_ctx_mutex
is only used to protect setting tctx->ev.

Found by staring at code while fixing the FreeBSD memleak due to
not destroying scheduled_mutex.

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 lib/tevent/tevent_threads.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/lib/tevent/tevent_threads.c b/lib/tevent/tevent_threads.c
index 8197323..8ecda02 100644
--- a/lib/tevent/tevent_threads.c
+++ b/lib/tevent/tevent_threads.c
@@ -443,15 +443,14 @@ void _tevent_threaded_schedule_immediate(struct tevent_threaded_context *tctx,
 
 	ev = tctx->event_ctx;
 
-	ret = pthread_mutex_unlock(&tctx->event_ctx_mutex);
-	if (ret != 0) {
-		abort();
-	}
-
 	if (ev == NULL) {
 		/*
 		 * Our event context is already gone.
 		 */
+		ret = pthread_mutex_unlock(&tctx->event_ctx_mutex);
+		if (ret != 0) {
+			abort();
+		}
 		return;
 	}
 
@@ -479,6 +478,11 @@ void _tevent_threaded_schedule_immediate(struct tevent_threaded_context *tctx,
 		abort();
 	}
 
+	ret = pthread_mutex_unlock(&tctx->event_ctx_mutex);
+	if (ret != 0) {
+		abort();
+	}
+
 	/*
 	 * We might want to wake up the main thread under the lock. We
 	 * had a slightly similar situation in pthreadpool, changed
-- 
2.1.4



More information about the samba-technical mailing list