Improve robustness for refcounting

Document that we do intend to double-decref underlying bufferevents under
some circumstances.  Check to make sure that we don't decref past 0.

diff --git a/buffer.c b/buffer.c
index 20454197c6eb05ece4b0220092c27cbaac49fe2d..731a006477356e10062aa6d64b4c82f4a5bd9d77 100644 (file)
--- a/buffer.c
+++ b/buffer.c
@@ -435,6 +435,8 @@ _evbuffer_decref_and_unlock(struct evbuffer *buffer)
        struct evbuffer_chain *chain, *next;
        ASSERT_EVBUFFER_LOCKED(buffer);
 
+       EVUTIL_ASSERT(buffer->refcnt > 0);
+
        if (--buffer->refcnt > 0) {
                EVBUFFER_UNLOCK(buffer);
                return;

diff --git a/bufferevent-internal.h b/bufferevent-internal.h
index c83bf0140bbb7ab1b8ac3b8ce77ea2972a8ca49f..cd50856e52429ca0131c29a2ae492a5896d0e7e7 100644 (file)
--- a/bufferevent-internal.h
+++ b/bufferevent-internal.h
@@ -280,11 +280,12 @@ void bufferevent_incref(struct bufferevent *bufev);
 /** Internal: Lock bufev and increase its reference count.
  * unlocking it otherwise. */
 void _bufferevent_incref_and_lock(struct bufferevent *bufev);
-/** Internal: Increment the reference count on bufev. */
-void bufferevent_decref(struct bufferevent *bufev);
+/** Internal: Decrement the reference count on bufev.  Returns 1 if it freed
+ * the bufferevent.*/
+int bufferevent_decref(struct bufferevent *bufev);
 /** Internal: Drop the reference count on bufev, freeing as necessary, and
- * unlocking it otherwise. */
-void _bufferevent_decref_and_unlock(struct bufferevent *bufev);
+ * unlocking it otherwise.  Returns 1 if it freed the bufferevent. */
+int _bufferevent_decref_and_unlock(struct bufferevent *bufev);
 
 /** Internal: If callbacks are deferred and we have a read callback, schedule
  * a readcb.  Otherwise just run the readcb. */

diff --git a/bufferevent.c b/bufferevent.c
index 7fef8bb650e5187bb38621257f735a6c62a073f2..958fff9002d0034cd5941d23cf4bf3898f1fe9a2 100644 (file)
--- a/bufferevent.c
+++ b/bufferevent.c
@@ -504,16 +504,36 @@ _bufferevent_incref_and_lock(struct bufferevent *bufev)
        ++bufev_private->refcnt;
 }
 
-void
+#if 0
+static void
+_bufferevent_transfer_lock_ownership(struct bufferevent *donor,
+    struct bufferevent *recipient)
+{
+       struct bufferevent_private *d = BEV_UPCAST(donor);
+       struct bufferevent_private *r = BEV_UPCAST(recipient);
+       if (d->lock != r->lock)
+               return;
+       if (r->own_lock)
+               return;
+       if (d->own_lock) {
+               d->own_lock = 0;
+               r->own_lock = 1;
+       }
+}
+#endif
+
+int
 _bufferevent_decref_and_unlock(struct bufferevent *bufev)
 {
        struct bufferevent_private *bufev_private =
            EVUTIL_UPCAST(bufev, struct bufferevent_private, bev);
        struct bufferevent *underlying;
 
+       EVUTIL_ASSERT(bufev_private->refcnt > 0);
+
        if (--bufev_private->refcnt) {
                BEV_UNLOCK(bufev);
-               return;
+               return 0;
        }
 
        underlying = bufferevent_get_underlying(bufev);
@@ -550,21 +570,27 @@ _bufferevent_decref_and_unlock(struct bufferevent *bufev)
        /* Free the actual allocated memory. */
        mm_free(bufev - bufev->be_ops->mem_offset);
 
-       /* release the reference to underlying now that we no longer need
-        * the reference to it.  This is mainly in case our lock is shared
-        * with underlying.
+       /* Release the reference to underlying now that we no longer need the
+        * reference to it.  We wait this long mainly in case our lock is
+        * shared with underlying.
+        *
+        * The 'destruct' function will also drop a reference to underlying
+        * if BEV_OPT_CLOSE_ON_FREE is set.
+        *
         * XXX Should we/can we just refcount evbuffer/bufferevent locks?
         * It would probably save us some headaches.
         */
        if (underlying)
                bufferevent_decref(underlying);
+
+       return 1;
 }
 
-void
+int
 bufferevent_decref(struct bufferevent *bufev)
 {
        BEV_LOCK(bufev);
-       _bufferevent_decref_and_unlock(bufev);
+       return _bufferevent_decref_and_unlock(bufev);
 }
 
 void

diff --git a/bufferevent_filter.c b/bufferevent_filter.c
index 45f9c791af234332e710da0dcad8d8db2528c401..7acbc52019443edd899ab331b3d2f2f0d6bae2a4 100644 (file)
--- a/bufferevent_filter.c
+++ b/bufferevent_filter.c
@@ -187,6 +187,7 @@ bufferevent_filter_new(struct bufferevent *underlying,
        }
 
        bufev_f->underlying = underlying;
+
        bufev_f->process_in = input_filter;
        bufev_f->process_out = output_filter;
        bufev_f->free_context = free_context;
@@ -212,8 +213,19 @@ be_filter_destruct(struct bufferevent *bev)
        if (bevf->free_context)
                bevf->free_context(bevf->context);
 
-       if (bevf->bev.options & BEV_OPT_CLOSE_ON_FREE)
-               bufferevent_free(bevf->underlying);
+       if (bevf->bev.options & BEV_OPT_CLOSE_ON_FREE) {
+               /* Yes, there is also a decref in bufferevent_decref.
+                * That decref corresponds to the incref when we set
+                * underlying for the first time.  This decref is an
+                * extra one to remove the last reference.
+                */
+               if (BEV_UPCAST(bevf->underlying)->refcnt < 2) {
+                       event_warnx("BEV_OPT_CLOSE_ON_FREE set on an "
+                           "bufferevent with too few references");
+               } else {
+                       bufferevent_free(bevf->underlying);
+               }
+       }
 
        _bufferevent_del_generic_timeout_cbs(bev);
 }

diff --git a/bufferevent_openssl.c b/bufferevent_openssl.c
index 2ccd4343fa2e767667095897d10ebabbb4039961..a5aee02d5d4c8b842979ae9416a19299132d8736 100644 (file)
--- a/bufferevent_openssl.c
+++ b/bufferevent_openssl.c
@@ -1030,8 +1030,13 @@ be_openssl_destruct(struct bufferevent *bev)
 
        if (bev_ssl->bev.options & BEV_OPT_CLOSE_ON_FREE) {
                if (bev_ssl->underlying) {
-                       bufferevent_free(bev_ssl->underlying);
-                       bev_ssl->underlying = NULL;
+                       if (BEV_UPCAST(bev_ssl->underlying)->refcnt < 2) {
+                               event_warnx("BEV_OPT_CLOSE_ON_FREE set on an "
+                                   "bufferevent with too few references");
+                       } else {
+                               bufferevent_free(bev_ssl->underlying);
+                               bev_ssl->underlying = NULL;
+                       }
                }
                SSL_free(bev_ssl->ssl);
        }