[PATCH] a few fixes for tevent
Jeremy Allison
jra at samba.org
Tue Jun 6 23:47:43 UTC 2017
On Tue, Jun 06, 2017 at 10:08:11AM +0200, Volker Lendecke via samba-technical wrote:
> Hi!
>
> Review appreciated!
FYI, Reviewing these - but threaded code changes are tricky and I
don't understand the changes yet. I'll try again tomorrow :-).
Cheers,
Jeremy.
> --
> 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
> 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