From f1bc125eb453af61cbb750652103532b77c271b0 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 12 Mar 2010 23:00:49 -0500 Subject: [PATCH] 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. --- buffer.c | 2 ++ bufferevent-internal.h | 9 +++++---- bufferevent.c | 40 +++++++++++++++++++++++++++++++++------- bufferevent_filter.c | 16 ++++++++++++++-- bufferevent_openssl.c | 9 +++++++-- 5 files changed, 61 insertions(+), 15 deletions(-) diff --git a/buffer.c b/buffer.c index 20454197..731a0064 100644 --- 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 c83bf014..cd50856e 100644 --- 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 7fef8bb6..958fff90 100644 --- 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 45f9c791..7acbc520 100644 --- 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 2ccd4343..a5aee02d 100644 --- 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); } -- 2.40.0