From 92a359ee3adf4636db508e6c6d7179d4d59eaafc Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Sat, 3 Jan 2015 19:37:15 +0300 Subject: [PATCH 1/5] be_pair: release shared lock with the latest of bufferevent_pair Then next code sample will use free'd lock: evthread_use_pthreads(); ... assert(!bufferevent_pair_new(base, BEV_OPT_THREADSAFE, pair)); ... bufferevent_free(pair[0]); # refcnt == 0 -> unlink bufferevent_free(pair[1]); # refcnt == 0 -> unlink ... event_base_free() -> finalizers -> EVTHREAD_FREE_LOCK(bev1->lock) -> BEV_LOCK(bev2->lock) <-- *already freed* While if you will reverse the order: bufferevent_free(pair[1]); # refcnt == 0 -> unlink bufferevent_free(pair[0]); # refcnt == 0 -> unlink ... event_base_free() -> finalizers -> BEV_LOCK(bev2->lock)/!own_lock/BEV_UNLOCK(bev2->lock) -> EVTHREAD_FREE_LOCK(bev1->lock) (own_lock) It is ok now, but I guess that it will be better to relax order of freeing pairs. --- bufferevent_pair.c | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/bufferevent_pair.c b/bufferevent_pair.c index 4340f237..8154e17b 100644 --- a/bufferevent_pair.c +++ b/bufferevent_pair.c @@ -45,6 +45,8 @@ struct bufferevent_pair { struct bufferevent_private bev; struct bufferevent_pair *partner; + /* For ->destruct() lock checking */ + struct bufferevent_pair *unlinked_partner; }; @@ -265,11 +267,40 @@ be_pair_unlink(struct bufferevent *bev) struct bufferevent_pair *bev_p = upcast(bev); if (bev_p->partner) { + bev_p->unlinked_partner = bev_p->partner; bev_p->partner->partner = NULL; bev_p->partner = NULL; } } +/* Free *shared* lock in the latest be (since we share it between two of them). */ +static void +be_pair_destruct(struct bufferevent *bev) +{ + struct bufferevent_pair *bev_p = upcast(bev); + + /* Transfer ownership of the lock into partner, otherwise we will use + * already free'd lock during freeing second bev, see next example: + * + * bev1->own_lock = 1 + * bev2->own_lock = 0 + * bev2->lock = bev1->lock + * + * bufferevent_free(bev1) # refcnt == 0 -> unlink + * bufferevent_free(bev2) # refcnt == 0 -> unlink + * + * event_base_free() -> finilizers -> EVTHREAD_FREE_LOCK(bev1->lock) + * -> BEV_LOCK(bev2->lock) <-- already freed + * + * Where bev1 == pair[0], bev2 == pair[1]. + */ + if (bev_p->unlinked_partner && bev_p->bev.own_lock) { + bev_p->unlinked_partner->bev.own_lock = 1; + bev_p->bev.own_lock = 0; + } + bev_p->unlinked_partner = NULL; +} + static int be_pair_flush(struct bufferevent *bev, short iotype, enum bufferevent_flush_mode mode) @@ -320,7 +351,7 @@ const struct bufferevent_ops bufferevent_ops_pair = { be_pair_enable, be_pair_disable, be_pair_unlink, - NULL, /* be_pair_destruct, */ + be_pair_destruct, bufferevent_generic_adj_timeouts_, be_pair_flush, NULL, /* ctrl */ From c0b34f6fd5258bd2bb5cb02ed6408b21229a56b7 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 26 Jan 2015 00:27:41 +0300 Subject: [PATCH 2/5] evthread: add evthread_get_{lock,condition}_callbacks() helpers --- evthread-internal.h | 5 +++++ evthread.c | 19 +++++++++++++------ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/evthread-internal.h b/evthread-internal.h index 346b2bb9..45e178f2 100644 --- a/evthread-internal.h +++ b/evthread-internal.h @@ -376,6 +376,11 @@ int evsig_global_setup_locks_(const int enable_locks); int evutil_global_setup_locks_(const int enable_locks); int evutil_secure_rng_global_setup_locks_(const int enable_locks); +/** Return current evthread_lock_callbacks */ +struct evthread_lock_callbacks *evthread_get_lock_callbacks(); +/** Return current evthread_condition_callbacks */ +struct evthread_condition_callbacks *evthread_get_condition_callbacks(); + #endif #ifdef __cplusplus diff --git a/evthread.c b/evthread.c index 4da5d24e..40933626 100644 --- a/evthread.c +++ b/evthread.c @@ -69,12 +69,21 @@ evthread_set_id_callback(unsigned long (*id_fn)(void)) evthread_id_fn_ = id_fn; } +struct evthread_lock_callbacks *evthread_get_lock_callbacks() +{ + return evthread_lock_debugging_enabled_ + ? &original_lock_fns_ : &evthread_lock_fns_; +} +struct evthread_condition_callbacks *evthread_get_condition_callbacks() +{ + return evthread_lock_debugging_enabled_ + ? &original_cond_fns_ : &evthread_cond_fns_; +} + int evthread_set_lock_callbacks(const struct evthread_lock_callbacks *cbs) { - struct evthread_lock_callbacks *target = - evthread_lock_debugging_enabled_ - ? &original_lock_fns_ : &evthread_lock_fns_; + struct evthread_lock_callbacks *target = evthread_get_lock_callbacks(); if (!cbs) { if (target->alloc) @@ -109,9 +118,7 @@ evthread_set_lock_callbacks(const struct evthread_lock_callbacks *cbs) int evthread_set_condition_callbacks(const struct evthread_condition_callbacks *cbs) { - struct evthread_condition_callbacks *target = - evthread_lock_debugging_enabled_ - ? &original_cond_fns_ : &evthread_cond_fns_; + struct evthread_condition_callbacks *target = evthread_get_condition_callbacks(); if (!cbs) { if (target->alloc_condition) From ccc55937cfc23b5ecca7f6b2427f906f8413ca4e Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 26 Jan 2015 00:28:46 +0300 Subject: [PATCH 3/5] evthread: evthreadimpl_disable_lock_debugging_() for libevent_global_shutdown() --- evthread-internal.h | 2 ++ evthread.c | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/evthread-internal.h b/evthread-internal.h index 45e178f2..16adf167 100644 --- a/evthread-internal.h +++ b/evthread-internal.h @@ -380,6 +380,8 @@ int evutil_secure_rng_global_setup_locks_(const int enable_locks); struct evthread_lock_callbacks *evthread_get_lock_callbacks(); /** Return current evthread_condition_callbacks */ struct evthread_condition_callbacks *evthread_get_condition_callbacks(); +/** Disable locking for internal usage (like global shutdown) */ +void evthreadimpl_disable_lock_debugging_(void); #endif diff --git a/evthread.c b/evthread.c index 40933626..02dab7a8 100644 --- a/evthread.c +++ b/evthread.c @@ -79,6 +79,10 @@ struct evthread_condition_callbacks *evthread_get_condition_callbacks() return evthread_lock_debugging_enabled_ ? &original_cond_fns_ : &evthread_cond_fns_; } +void evthreadimpl_disable_lock_debugging_(void) +{ + evthread_lock_debugging_enabled_ = 0; +} int evthread_set_lock_callbacks(const struct evthread_lock_callbacks *cbs) From e5c87d18b0b57bd04f4a7808e277698f6e62cfb2 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 26 Jan 2015 00:29:15 +0300 Subject: [PATCH 4/5] event_free_debug_globals_locks(): disable lock debugging This will allow to use library event after event_free_debug_globals_locks()/libevent_global_shutdown() without invalid read/write's. --- event.c | 1 + 1 file changed, 1 insertion(+) diff --git a/event.c b/event.c index 2e4f64ea..440a90a5 100644 --- a/event.c +++ b/event.c @@ -3763,6 +3763,7 @@ event_free_debug_globals_locks(void) if (event_debug_map_lock_ != NULL) { EVTHREAD_FREE_LOCK(event_debug_map_lock_, 0); event_debug_map_lock_ = NULL; + evthreadimpl_disable_lock_debugging_(); } #endif /* EVENT__DISABLE_DEBUG_MODE */ #endif /* EVENT__DISABLE_THREAD_SUPPORT */ From a558fcdb43f3f403e51a24cb35dc63bb5b1a9287 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 26 Jan 2015 00:31:23 +0300 Subject: [PATCH 5/5] be_pair/regress: cover use of shared lock (lock/unlock/free) For more info look at 92a359ee3adf4636db508e6c6d7179d4d59eaafc ("be_pair: release shared lock with the latest of bufferevent_pair") --- test/regress_bufferevent.c | 147 ++++++++++++++++++++++++++++++++++++- 1 file changed, 145 insertions(+), 2 deletions(-) diff --git a/test/regress_bufferevent.c b/test/regress_bufferevent.c index 9cc453fe..a1998ba6 100644 --- a/test/regress_bufferevent.c +++ b/test/regress_bufferevent.c @@ -75,6 +75,7 @@ #include "event2/util.h" #include "bufferevent-internal.h" +#include "evthread-internal.h" #include "util-internal.h" #ifdef _WIN32 #include "iocp-internal.h" @@ -173,9 +174,9 @@ test_bufferevent_impl(int use_pair) event_dispatch(); - bufferevent_free(bev1); - tt_ptr_op(bufferevent_pair_get_partner(bev2), ==, NULL); bufferevent_free(bev2); + tt_ptr_op(bufferevent_pair_get_partner(bev1), ==, NULL); + bufferevent_free(bev1); if (test_ok != 2) test_ok = 0; @@ -195,6 +196,143 @@ test_bufferevent_pair(void) test_bufferevent_impl(1); } +#if defined(EVTHREAD_USE_PTHREADS_IMPLEMENTED) +/** + * Trace lock/unlock/alloc/free for locks. + * (More heavier then evthread_debug*) + */ +typedef struct +{ + void *lock; + enum { + ALLOC, FREE, + } status; + size_t locked /** allow recursive locking */; +} lock_wrapper; +struct lock_unlock_base +{ + /* Original callbacks */ + struct evthread_lock_callbacks cbs; + /* Map of locks */ + lock_wrapper *locks; + size_t nr_locks; +} lu_base = { + .locks = NULL, +}; + +static lock_wrapper *lu_find(void *lock_) +{ + size_t i; + for (i = 0; i < lu_base.nr_locks; ++i) { + lock_wrapper *lock = &lu_base.locks[i]; + if (lock->lock == lock_) + return lock; + } + return NULL; +} + +static void *trace_lock_alloc(unsigned locktype) +{ + ++lu_base.nr_locks; + lu_base.locks = realloc(lu_base.locks, + sizeof(lock_wrapper) * lu_base.nr_locks); + void *lock = lu_base.cbs.alloc(locktype); + lu_base.locks[lu_base.nr_locks - 1] = (lock_wrapper){ lock, ALLOC, 0 }; + return lock; +} +static void trace_lock_free(void *lock_, unsigned locktype) +{ + lock_wrapper *lock = lu_find(lock_); + if (!lock || lock->status == FREE || lock->locked) { + __asm__("int3"); + TT_FAIL(("lock: free error")); + } else { + lock->status = FREE; + lu_base.cbs.free(lock_, locktype); + } +} +static int trace_lock_lock(unsigned mode, void *lock_) +{ + lock_wrapper *lock = lu_find(lock_); + if (!lock || lock->status == FREE) { + TT_FAIL(("lock: lock error")); + return -1; + } else { + ++lock->locked; + return lu_base.cbs.lock(mode, lock_); + } +} +static int trace_lock_unlock(unsigned mode, void *lock_) +{ + lock_wrapper *lock = lu_find(lock_); + if (!lock || lock->status == FREE || !lock->locked) { + TT_FAIL(("lock: unlock error")); + return -1; + } else { + --lock->locked; + return lu_base.cbs.unlock(mode, lock_); + } +} +static void lock_unlock_free_thread_cbs() +{ + event_base_free(NULL); + + /** drop immutable flag */ + evthread_set_lock_callbacks(NULL); + /** avoid calling of event_global_setup_locks_() for new cbs */ + libevent_global_shutdown(); + /** drop immutable flag for non-debug ops (since called after shutdown) */ + evthread_set_lock_callbacks(NULL); +} + +static int use_lock_unlock_profiler(void) +{ + struct evthread_lock_callbacks cbs = { + EVTHREAD_LOCK_API_VERSION, + EVTHREAD_LOCKTYPE_RECURSIVE, + trace_lock_alloc, + trace_lock_free, + trace_lock_lock, + trace_lock_unlock, + }; + memcpy(&lu_base.cbs, evthread_get_lock_callbacks(), + sizeof(lu_base.cbs)); + { + lock_unlock_free_thread_cbs(); + + evthread_set_lock_callbacks(&cbs); + /** re-create debug locks correctly */ + evthread_enable_lock_debugging(); + + event_init(); + } + return 0; +} +static void free_lock_unlock_profiler(struct basic_test_data *data) +{ + lock_unlock_free_thread_cbs(); + free(lu_base.locks); + data->base = NULL; +} + +static void test_bufferevent_pair_release_lock(void *arg) +{ + struct basic_test_data *data = arg; + use_lock_unlock_profiler(); + { + struct bufferevent *pair[2]; + if (!bufferevent_pair_new(NULL, BEV_OPT_THREADSAFE, pair)) { + bufferevent_free(pair[0]); + bufferevent_free(pair[1]); + } else + tt_abort_perror("bufferevent_pair_new"); + } + free_lock_unlock_profiler(data); +end: + ; +} +#endif + /* * test watermarks and bufferevent */ @@ -949,6 +1087,11 @@ struct testcase_t bufferevent_testcases[] = { LEGACY(bufferevent, TT_ISOLATED), LEGACY(bufferevent_pair, TT_ISOLATED), +#if defined(EVTHREAD_USE_PTHREADS_IMPLEMENTED) + { "bufferevent_pair_release_lock", test_bufferevent_pair_release_lock, + TT_FORK|TT_ISOLATED|TT_NEED_THREADS|TT_NEED_BASE|TT_LEGACY, + &basic_setup, NULL }, +#endif LEGACY(bufferevent_watermarks, TT_ISOLATED), LEGACY(bufferevent_pair_watermarks, TT_ISOLATED), LEGACY(bufferevent_filters, TT_ISOLATED),