[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