From e26aa4cbb184d56db5ae64524b31c02b2d2d8790 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Sat, 2 Apr 2022 00:42:20 +0200 Subject: [PATCH] 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. (cherry picked from commit ae01ec282343d4231167beb19f2ee83c99d95c0c) --- bin/named/server.c | 8 ++-- lib/dns/catz.c | 2 +- lib/dns/nta.c | 7 +++- lib/dns/resolver.c | 6 +-- lib/dns/rpz.c | 2 +- lib/dns/zone.c | 2 +- lib/isc/include/isc/timer.h | 29 ++++----------- lib/isc/ratelimiter.c | 7 +++- lib/isc/timer.c | 74 +++++++++++++++---------------------- tests/isc/task_test.c | 4 +- tests/isc/timer_test.c | 22 ++++++----- 11 files changed, 71 insertions(+), 92 deletions(-) diff --git a/bin/named/server.c b/bin/named/server.c index 1114882ba9..fd6b11e7a4 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -10119,10 +10119,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 ed50dea053..1c0dfc1621 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -978,7 +978,7 @@ dns_catz_zone_detach(dns_catz_zone_t **zonep) { isc_ht_destroy(&zone->coos); } zone->magic = 0; - isc_timer_detach(&zone->updatetimer); + isc_timer_destroy(&zone->updatetimer); if (zone->db_registered) { dns_db_updatenotify_unregister( zone->db, dns_catz_dbupdate_callback, diff --git a/lib/dns/nta.c b/lib/dns/nta.c index 1c7a68dc54..5614eb8aeb 100644 --- a/lib/dns/nta.c +++ b/lib/dns/nta.c @@ -77,7 +77,7 @@ nta_detach(isc_mem_t *mctx, dns_nta_t **ntap) { (void)isc_timer_reset(nta->timer, isc_timertype_inactive, NULL, NULL, true); - isc_timer_detach(&nta->timer); + isc_timer_destroy(&nta->timer); } if (dns_rdataset_isassociated(&nta->rdataset)) { dns_rdataset_disassociate(&nta->rdataset); @@ -293,6 +293,9 @@ settimer(dns_ntatable_t *ntatable, dns_nta_t *nta, uint32_t lifetime) { result = isc_timer_create(ntatable->timermgr, isc_timertype_ticker, NULL, &interval, ntatable->task, checkbogus, nta, &nta->timer); + if (result != ISC_R_SUCCESS) { + isc_timer_destroy(&nta->timer); + } return (result); } @@ -479,7 +482,7 @@ again: (void)isc_timer_reset(nta->timer, isc_timertype_inactive, NULL, 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 880d33b4d3..2cf8a22210 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -4479,7 +4479,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); @@ -5050,7 +5050,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); @@ -10242,7 +10242,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 71340785d6..4ddc6f00fb 100644 --- a/lib/dns/rpz.c +++ b/lib/dns/rpz.c @@ -2231,7 +2231,7 @@ rpz_detach(dns_rpz_zone_t **rpzp) { isc_timer_reset(rpz->updatetimer, isc_timertype_inactive, NULL, 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 556bd65b20..2ee0249b03 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -15065,7 +15065,7 @@ zone_shutdown(isc_task_t *task, isc_event_t *event) { forward_cancel(zone); if (zone->timer != NULL) { - 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 1e828d6bc3..815cf42dbe 100644 --- a/lib/isc/include/isc/timer.h +++ b/lib/isc/include/isc/timer.h @@ -225,25 +225,9 @@ isc_timer_touch(isc_timer_t *timer); */ 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: * @@ -253,9 +237,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 * @@ -264,9 +245,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 601c133a9e..84a848a8a3 100644 --- a/lib/isc/ratelimiter.c +++ b/lib/isc/ratelimiter.c @@ -242,6 +242,7 @@ void isc_ratelimiter_shutdown(isc_ratelimiter_t *rl) { isc_event_t *ev; isc_task_t *task; + isc_result_t result; REQUIRE(rl != NULL); @@ -257,7 +258,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, NULL, + true); + 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/timer.c b/lib/isc/timer.c index 33daa554cd..11bf197a29 100644 --- a/lib/isc/timer.c +++ b/lib/isc/timer.c @@ -60,9 +60,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_time_t expires; @@ -72,7 +72,6 @@ struct isc_timer { void *arg; unsigned int index; isc_time_t due; - ISC_LIST(isc_timerevent_t) active; LINK(isc_timer_t) link; }; @@ -216,13 +215,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 @@ -230,8 +230,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 @@ -242,25 +244,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)); -} - isc_result_t isc_timer_create(isc_timermgr_t *manager, isc_timertype_t type, const isc_time_t *expires, const isc_interval_t *interval, @@ -310,7 +293,6 @@ isc_timer_create(isc_timermgr_t *manager, isc_timertype_t type, timer = isc_mem_get(manager->mctx, sizeof(*timer)); timer->manager = manager; - isc_refcount_init(&timer->references, 1); if (type == isc_timertype_once && !isc_interval_iszero(interval)) { result = isc_time_add(&now, interval, &timer->idle); @@ -495,30 +477,32 @@ isc_timer_touch(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); +isc_timer_destroy(isc_timer_t **timerp) { + isc_timer_t *timer = NULL; + isc_timermgr_t *manager = NULL; - *timerp = timer; -} + REQUIRE(timerp != NULL && VALID_TIMER(*timerp)); -void -isc_timer_detach(isc_timer_t **timerp) { - isc_timer_t *timer; - /* - * Detach *timerp from its timer. - */ - - REQUIRE(timerp != NULL); timer = *timerp; - REQUIRE(VALID_TIMER(timer)); - - if (isc_refcount_decrement(&timer->references) == 1) { - destroy(timer); - } - *timerp = NULL; + + manager = timer->manager; + + 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 @@ -533,13 +517,13 @@ timer_post_event(isc_timermgr_t *manager, isc_timer_t *timer, 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)); } diff --git a/tests/isc/task_test.c b/tests/isc/task_test.c index e31b54c77f..973ecc9a99 100644 --- a/tests/isc/task_test.c +++ b/tests/isc/task_test.c @@ -525,8 +525,8 @@ ISC_RUN_TEST_IMPL(basic) { 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/tests/isc/timer_test.c b/tests/isc/timer_test.c index 099a3523be..644018aacb 100644 --- a/tests/isc/timer_test.c +++ b/tests/isc/timer_test.c @@ -236,14 +236,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); } /* @@ -329,9 +329,10 @@ test_idle(isc_task_t *task, isc_event_t *event) { subthread_assert_int_equal(event->ev_type, ISC_TIMEREVENT_IDLE); - 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 */ @@ -415,14 +416,15 @@ test_reset(isc_task_t *task, isc_event_t *event) { &expires, &interval, false); subthread_assert_result_equal(result, ISC_R_SUCCESS); } + + isc_event_free(&event); } else { subthread_assert_int_equal(event->ev_type, ISC_TIMEREVENT_LIFE); - isc_timer_detach(&timer); + isc_event_free(&event); + isc_timer_destroy(&timer); isc_task_shutdown(task); } - - isc_event_free(&event); } ISC_RUN_TEST_IMPL(reset) { @@ -578,8 +580,8 @@ ISC_RUN_TEST_IMPL(purge) { 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); }