From: Azat Khuzhin <a3at.mail@gmail.com>
Date: Sat, 3 Jan 2015 16:37:15 +0000 (+0300)
Subject: be_pair: release shared lock with the latest of bufferevent_pair
X-Git-Tag: release-2.1.6-beta~97^2~4
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=92a359ee3adf4636db508e6c6d7179d4d59eaafc;p=libevent

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.
---

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 */