Don't use reference counting in isc_timer unit

The reference counting and isc_timer_attach()/isc_timer_detach()
semantic are actually misleading because it cannot be used under normal
conditions.  The usual conditions under which is timer used uses the
object where timer is used as argument to the "timer" itself.  This
means that when the caller is using `isc_timer_detach()` it needs the
timer to stop and the isc_timer_detach() does that only if this would be
the last reference.  Unfortunately, this also means that if the timer is
attached elsewhere and the timer is fired it will most likely be
use-after-free, because the object used in the timer no longer exists.

Remove the reference counting from the isc_timer unit, remove
isc_timer_attach() function and rename isc_timer_detach() to
isc_timer_destroy() to better reflect how the API needs to be used.

The only caveat is that the already executed event must be destroyed
before the isc_timer_destroy() is called because the timer is no longet
attached to .ev_destroy_arg.

diff --git a/bin/named/server.c b/bin/named/server.c
index f720cb0016da7f4a6a2e40f1169f4296d65ea607..fa327e8da01e0d3765f46987c9bc57784e682670 100644 (file)
--- a/bin/named/server.c
+++ b/bin/named/server.c
@@ -9893,10 +9893,10 @@ shutdown_server(isc_task_t *task, isc_event_t *event) {
                isc_mem_put(server->mctx, nsc, sizeof(*nsc));
        }
 
-       isc_timer_detach(&server->interface_timer);
-       isc_timer_detach(&server->heartbeat_timer);
-       isc_timer_detach(&server->pps_timer);
-       isc_timer_detach(&server->tat_timer);
+       isc_timer_destroy(&server->interface_timer);
+       isc_timer_destroy(&server->heartbeat_timer);
+       isc_timer_destroy(&server->pps_timer);
+       isc_timer_destroy(&server->tat_timer);
 
        ns_interfacemgr_detach(&server->interfacemgr);
 

diff --git a/lib/dns/catz.c b/lib/dns/catz.c
index 9be7b91ed8e9fa4c4db53e35f7f79307b2960b1a..bd1e76b1a5b863606dc7d51d070be5fe77c1d9ad 100644 (file)
--- a/lib/dns/catz.c
+++ b/lib/dns/catz.c
@@ -765,7 +765,7 @@ dns_catz_zone_detach(dns_catz_zone_t **zonep) {
                        isc_ht_destroy(&zone->entries);
                }
                zone->magic = 0;
-               isc_timer_detach(&zone->updatetimer);
+               isc_timer_destroy(&zone->updatetimer);
                if (zone->db_registered) {
                        INSIST(dns_db_updatenotify_unregister(
                                       zone->db, dns_catz_dbupdate_callback,

diff --git a/lib/dns/nta.c b/lib/dns/nta.c
index 49f4ddef47d580a44a5b2fb9377bbc8028842e35..c5d3d905a4d6eb329cf51e652517eb935e3e48a2 100644 (file)
--- a/lib/dns/nta.c
+++ b/lib/dns/nta.c
@@ -76,7 +76,7 @@ nta_detach(isc_mem_t *mctx, dns_nta_t **ntap) {
                if (nta->timer != NULL) {
                        (void)isc_timer_reset(
                                nta->timer, isc_timertype_inactive, NULL, true);
-                       isc_timer_detach(&nta->timer);
+                       isc_timer_destroy(&nta->timer);
                }
                if (dns_rdataset_isassociated(&nta->rdataset)) {
                        dns_rdataset_disassociate(&nta->rdataset);
@@ -294,7 +294,7 @@ settimer(dns_ntatable_t *ntatable, dns_nta_t *nta, uint32_t lifetime) {
        result = isc_timer_reset(nta->timer, isc_timertype_ticker, &interval,
                                 false);
        if (result != ISC_R_SUCCESS) {
-               isc_timer_detach(&nta->timer);
+               isc_timer_destroy(&nta->timer);
        }
        return (result);
 }
@@ -481,7 +481,7 @@ again:
                if (nta->timer != NULL) {
                        (void)isc_timer_reset(
                                nta->timer, isc_timertype_inactive, NULL, true);
-                       isc_timer_detach(&nta->timer);
+                       isc_timer_destroy(&nta->timer);
                }
 
                result = deletenode(ntatable, foundname);

diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c
index 53f3c2fa92cac78f7dba689f1ed4fb4ed15b0264..c8724f68ba8954d894f2b6eae5d4dfcecf02f81c 100644 (file)
--- a/lib/dns/resolver.c
+++ b/lib/dns/resolver.c
@@ -4379,7 +4379,7 @@ fctx_destroy(fetchctx_t *fctx, bool exiting) {
        dns_db_detach(&fctx->cache);
        dns_adb_detach(&fctx->adb);
 
-       isc_timer_detach(&fctx->timer);
+       isc_timer_destroy(&fctx->timer);
 
        dns_resolver_detach(&fctx->res);
 
@@ -4927,7 +4927,7 @@ cleanup_mctx:
        dns_db_detach(&fctx->cache);
 
 cleanup_timer:
-       isc_timer_detach(&fctx->timer);
+       isc_timer_destroy(&fctx->timer);
 
 cleanup_qmessage:
        dns_message_detach(&fctx->qmessage);
@@ -10118,7 +10118,7 @@ destroy(dns_resolver_t *res) {
        dns_resolver_reset_ds_digests(res);
        dns_badcache_destroy(&res->badcache);
        dns_resolver_resetmustbesecure(res);
-       isc_timer_detach(&res->spillattimer);
+       isc_timer_destroy(&res->spillattimer);
        res->magic = 0;
        isc_mem_putanddetach(&res->mctx, res, sizeof(*res));
 }

diff --git a/lib/dns/rpz.c b/lib/dns/rpz.c
index d0feec756e89adde611fdfda88faa591f33e929f..b62c20d107ccbd3ed5d287abf8555854fcbc86c2 100644 (file)
--- a/lib/dns/rpz.c
+++ b/lib/dns/rpz.c
@@ -2189,7 +2189,7 @@ rpz_detach(dns_rpz_zone_t **rpzp) {
 
                isc_timer_reset(rpz->updatetimer, isc_timertype_inactive, NULL,
                                true);
-               isc_timer_detach(&rpz->updatetimer);
+               isc_timer_destroy(&rpz->updatetimer);
 
                isc_ht_destroy(&rpz->nodes);
 

diff --git a/lib/dns/zone.c b/lib/dns/zone.c
index ec6c78030ffcc701e4bde93864c4eabcb81893d5..76c2554b1ad3d412426aed877380444cab224b32 100644 (file)
--- a/lib/dns/zone.c
+++ b/lib/dns/zone.c
@@ -15088,8 +15088,7 @@ zone_shutdown(isc_task_t *task, isc_event_t *event) {
        forward_cancel(zone);
 
        if (zone->timer != NULL) {
-               zone_timer_stop(zone);
-               isc_timer_detach(&zone->timer);
+               isc_timer_destroy(&zone->timer);
                isc_refcount_decrement(&zone->irefs);
        }
 

diff --git a/lib/isc/include/isc/timer.h b/lib/isc/include/isc/timer.h
index cbe23728788ff9c435c96bbe6a8880dbc79defd3..132cc67bf64b40e81d908db7397287efa36c2b26 100644 (file)
--- a/lib/isc/include/isc/timer.h
+++ b/lib/isc/include/isc/timer.h
@@ -182,25 +182,9 @@ isc_timer_reset(isc_timer_t *timer, isc_timertype_t type,
  */
 
 void
-isc_timer_attach(isc_timer_t *timer, isc_timer_t **timerp);
+isc_timer_destroy(isc_timer_t **timerp);
 /*%<
- * Attach *timerp to timer.
- *
- * Requires:
- *
- *\li  'timer' is a valid timer.
- *
- *\li  'timerp' points to a NULL timer.
- *
- * Ensures:
- *
- *\li  *timerp is attached to timer.
- */
-
-void
-isc_timer_detach(isc_timer_t **timerp);
-/*%<
- * Detach *timerp from its timer.
+ * Destroy *timerp.
  *
  * Requires:
  *
@@ -210,9 +194,6 @@ isc_timer_detach(isc_timer_t **timerp);
  *
  *\li  *timerp is NULL.
  *
- *\li  If '*timerp' is the last reference to the timer,
- *     then:
- *
  *\code
  *             The timer will be shutdown
  *
@@ -221,9 +202,13 @@ isc_timer_detach(isc_timer_t **timerp);
  *             All resources used by the timer have been freed
  *
  *             Any events already posted by the timer will be purged.
- *             Therefore, if isc_timer_detach() is called in the context
+ *             Therefore, if isc_timer_destroy() is called in the context
  *             of the timer's task, it is guaranteed that no more
  *             timer event callbacks will run after the call.
+ *
+ *             If this function is called from the timer event callback
+ *             the event itself must be destroyed before the timer
+ *             itself.
  *\endcode
  */
 

diff --git a/lib/isc/ratelimiter.c b/lib/isc/ratelimiter.c
index ceab020503db50086cf353105351bcae380afd8e..4a6bc909c74422ca97f1381972bf9c6c96ba92fc 100644 (file)
--- a/lib/isc/ratelimiter.c
+++ b/lib/isc/ratelimiter.c
@@ -229,6 +229,7 @@ void
 isc_ratelimiter_shutdown(isc_ratelimiter_t *rl) {
        isc_event_t *ev;
        isc_task_t *task;
+       isc_result_t result;
 
        REQUIRE(rl != NULL);
 
@@ -243,7 +244,11 @@ isc_ratelimiter_shutdown(isc_ratelimiter_t *rl) {
        }
        task = NULL;
        isc_task_attach(rl->task, &task);
-       isc_timer_detach(&rl->timer);
+
+       result = isc_timer_reset(rl->timer, isc_timertype_inactive, NULL,
+                                false);
+       RUNTIME_CHECK(result == ISC_R_SUCCESS);
+       isc_timer_destroy(&rl->timer);
 
        /*
         * Send an event to our task.  The delivery of this event

diff --git a/lib/isc/tests/task_test.c b/lib/isc/tests/task_test.c
index f71a8ee9c163afc48da45e921692a8a34b7532aa..a7cffbd8bb3d9abad710483d46fc15ae5374ab52 100644 (file)
--- a/lib/isc/tests/task_test.c
+++ b/lib/isc/tests/task_test.c
@@ -304,8 +304,8 @@ basic(void **state) {
        isc_task_detach(&task4);
 
        sleep(10);
-       isc_timer_detach(&ti1);
-       isc_timer_detach(&ti2);
+       isc_timer_destroy(&ti1);
+       isc_timer_destroy(&ti2);
 }
 
 /*

diff --git a/lib/isc/tests/timer_test.c b/lib/isc/tests/timer_test.c
index a41f7b8352347e8269121f7549daa341455579fa..75c30851fbc41f01a070d1c542981b9f7e870585 100644 (file)
--- a/lib/isc/tests/timer_test.c
+++ b/lib/isc/tests/timer_test.c
@@ -241,14 +241,14 @@ ticktock(isc_task_t *task, isc_event_t *event) {
        isc_mutex_unlock(&lasttime_mx);
        subthread_assert_result_equal(result, ISC_R_SUCCESS);
 
+       isc_event_free(&event);
+
        if (atomic_load(&eventcnt) == nevents) {
                result = isc_time_now(&endtime);
                subthread_assert_result_equal(result, ISC_R_SUCCESS);
-               isc_timer_detach(&timer);
+               isc_timer_destroy(&timer);
                isc_task_shutdown(task);
        }
-
-       isc_event_free(&event);
 }
 
 /*
@@ -312,9 +312,10 @@ test_idle(isc_task_t *task, isc_event_t *event) {
 
        subthread_assert_int_equal(event->ev_type, ISC_TIMEREVENT_ONCE);
 
-       isc_timer_detach(&timer);
-       isc_task_shutdown(task);
        isc_event_free(&event);
+
+       isc_timer_destroy(&timer);
+       isc_task_shutdown(task);
 }
 
 /* timer type once idles out */
@@ -387,14 +388,15 @@ test_reset(isc_task_t *task, isc_event_t *event) {
                                                 &interval, false);
                        subthread_assert_result_equal(result, ISC_R_SUCCESS);
                }
+
+               isc_event_free(&event);
        } else {
                subthread_assert_int_equal(event->ev_type, ISC_TIMEREVENT_ONCE);
 
-               isc_timer_detach(&timer);
+               isc_event_free(&event);
+               isc_timer_destroy(&timer);
                isc_task_shutdown(task);
        }
-
-       isc_event_free(&event);
 }
 
 static void
@@ -545,8 +547,8 @@ purge(void **state) {
 
        assert_int_equal(atomic_load(&eventcnt), 1);
 
-       isc_timer_detach(&tickertimer);
-       isc_timer_detach(&oncetimer);
+       isc_timer_destroy(&tickertimer);
+       isc_timer_destroy(&oncetimer);
        isc_task_destroy(&task1);
        isc_task_destroy(&task2);
 }

diff --git a/lib/isc/timer.c b/lib/isc/timer.c
index 6ecb1b4c1bf6b1673a1101f56db537ed778ae2c1..b2f6c0b1c1e492a3a8aa350ceeb5d42cacd02cc9 100644 (file)
--- a/lib/isc/timer.c
+++ b/lib/isc/timer.c
@@ -59,9 +59,9 @@ struct isc_timer {
        unsigned int magic;
        isc_timermgr_t *manager;
        isc_mutex_t lock;
-       isc_refcount_t references;
        /*! Locked by timer lock. */
        isc_time_t idle;
+       ISC_LIST(isc_timerevent_t) active;
        /*! Locked by manager lock. */
        isc_timertype_t type;
        isc_interval_t interval;
@@ -70,7 +70,6 @@ struct isc_timer {
        void *arg;
        unsigned int index;
        isc_time_t due;
-       ISC_LIST(isc_timerevent_t) active;
        LINK(isc_timer_t) link;
 };
 
@@ -201,13 +200,14 @@ timerevent_destroy(isc_event_t *event0) {
        isc_timer_t *timer = event0->ev_destroy_arg;
        isc_timerevent_t *event = (isc_timerevent_t *)event0;
 
+       LOCK(&timer->lock);
        if (ISC_LINK_LINKED(event, ev_timerlink)) {
                /* The event was unlinked via timer_purge() */
                timerevent_unlink(timer, event);
        }
+       UNLOCK(&timer->lock);
 
        isc_mem_put(timer->manager->mctx, event, event0->ev_size);
-       isc_timer_detach(&timer);
 }
 
 static void
@@ -215,8 +215,10 @@ timer_purge(isc_timer_t *timer) {
        isc_timerevent_t *event = NULL;
 
        while ((event = ISC_LIST_HEAD(timer->active)) != NULL) {
+               UNLOCK(&timer->lock);
                bool purged = isc_task_purgeevent(timer->task,
                                                  (isc_event_t *)event);
+               LOCK(&timer->lock);
                if (!purged) {
                        /*
                         * The event has already been executed, but not
@@ -227,25 +229,6 @@ timer_purge(isc_timer_t *timer) {
        }
 }
 
-static void
-destroy(isc_timer_t *timer) {
-       isc_timermgr_t *manager = timer->manager;
-
-       LOCK(&manager->lock);
-
-       timer_purge(timer);
-       deschedule(timer);
-
-       UNLINK(manager->timers, timer, link);
-
-       UNLOCK(&manager->lock);
-
-       isc_task_detach(&timer->task);
-       isc_mutex_destroy(&timer->lock);
-       timer->magic = 0;
-       isc_mem_put(manager->mctx, timer, sizeof(*timer));
-}
-
 void
 isc_timer_create(isc_timermgr_t *manager, isc_task_t *task,
                 isc_taskaction_t action, void *arg, isc_timer_t **timerp) {
@@ -272,8 +255,6 @@ isc_timer_create(isc_timermgr_t *manager, isc_task_t *task,
                .arg = arg,
        };
 
-       isc_refcount_init(&timer->references, 1);
-
        isc_time_settoepoch(&timer->idle);
 
        isc_task_attach(task, &timer->task);
@@ -378,30 +359,32 @@ isc_timer_gettype(isc_timer_t *timer) {
 }
 
 void
-isc_timer_attach(isc_timer_t *timer, isc_timer_t **timerp) {
-       REQUIRE(VALID_TIMER(timer));
-       REQUIRE(timerp != NULL && *timerp == NULL);
-       isc_refcount_increment(&timer->references);
-
-       *timerp = timer;
-}
+isc_timer_destroy(isc_timer_t **timerp) {
+       isc_timer_t *timer = NULL;
+       isc_timermgr_t *manager = NULL;
 
-void
-isc_timer_detach(isc_timer_t **timerp) {
-       isc_timer_t *timer;
-       /*
-        * Detach *timerp from its timer.
-        */
+       REQUIRE(timerp != NULL && VALID_TIMER(*timerp));
 
-       REQUIRE(timerp != NULL);
        timer = *timerp;
-       REQUIRE(VALID_TIMER(timer));
+       *timerp = NULL;
 
-       if (isc_refcount_decrement(&timer->references) == 1) {
-               destroy(timer);
-       }
+       manager = timer->manager;
 
-       *timerp = NULL;
+       LOCK(&manager->lock);
+
+       LOCK(&timer->lock);
+       timer_purge(timer);
+       deschedule(timer);
+       UNLOCK(&timer->lock);
+
+       UNLINK(manager->timers, timer, link);
+
+       UNLOCK(&manager->lock);
+
+       isc_task_detach(&timer->task);
+       isc_mutex_destroy(&timer->lock);
+       timer->magic = 0;
+       isc_mem_put(manager->mctx, timer, sizeof(*timer));
 }
 
 static void
@@ -415,13 +398,13 @@ post_event(isc_timermgr_t *manager, isc_timer_t *timer, isc_eventtype_t type) {
 
        ISC_LINK_INIT(event, ev_timerlink);
        ((isc_event_t *)event)->ev_destroy = timerevent_destroy;
-
-       isc_timer_attach(timer, &(isc_timer_t *){ NULL });
        ((isc_event_t *)event)->ev_destroy_arg = timer;
 
        event->due = timer->due;
 
+       LOCK(&timer->lock);
        ISC_LIST_APPEND(timer->active, event, ev_timerlink);
+       UNLOCK(&timer->lock);
 
        isc_task_send(timer->task, ISC_EVENT_PTR(&event));
 }