From be995074882d198c2a416db5d8a196e585ca3512 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 31 Mar 2022 21:55:15 +0200 Subject: [PATCH 1/8] Repair isc_task_purgeevent(), clean isc_task_unsend{,range}() The isc_task_purgerange() was walking through all events on the task to find a matching task. Instead use the ISC_LINK_LINKED to find whether the event is active. Cleanup the related isc_task_unsend() and isc_task_unsendrange() functions that were not used anywhere. (cherry picked from commit 17aed2f8951d75bfb4ea8ff5fcfdb7f13dd0ce2d) --- lib/isc/include/isc/task.h | 56 ++++++-------------------------------- lib/isc/task.c | 34 +++++------------------ lib/isc/tests | 1 - tests/isc/task_test.c | 14 ---------- 4 files changed, 15 insertions(+), 90 deletions(-) delete mode 120000 lib/isc/tests diff --git a/lib/isc/include/isc/task.h b/lib/isc/include/isc/task.h index 4d5f0080e5..973cb30284 100644 --- a/lib/isc/include/isc/task.h +++ b/lib/isc/include/isc/task.h @@ -54,18 +54,6 @@ * * Purging calls isc_event_free() on the matching events. * - * Unsending returns a list of events that matched the pattern. - * The caller is then responsible for them. - * - * Consumers of events should purge, not unsend. - * - * Producers of events often want to remove events when the caller indicates - * it is no longer interested in the object, e.g. by canceling a timer. - * Sometimes this can be done by purging, but for some event types, the - * calls to isc_event_free() cause deadlock because the event free routine - * wants to acquire a lock the caller is already holding. Unsending instead - * of purging solves this problem. As a general rule, producers should only - * unsend events which they have sent. */ /*** @@ -337,34 +325,6 @@ isc_task_purgeevent(isc_task_t *task, isc_event_t *event); * or was marked unpurgeable. */ -unsigned int -isc_task_unsendrange(isc_task_t *task, void *sender, isc_eventtype_t first, - isc_eventtype_t last, void *tag, isc_eventlist_t *events); -/*%< - * Remove events from a task's event queue. - * - * Requires: - * - *\li 'task' is a valid task. - * - *\li last >= first. - * - *\li *events is a valid list. - * - * Ensures: - * - *\li Events in the event queue of 'task' whose sender is 'sender', whose - * type is >= first and <= last, and whose tag is 'tag' will be dequeued - * and appended to *events. - * - *\li A sender of NULL will match any sender. A NULL tag matches any - * tag. - * - * Returns: - * - *\li The number of events unsent. - */ - unsigned int isc_task_unsend(isc_task_t *task, void *sender, isc_eventtype_t type, void *tag, isc_eventlist_t *events); @@ -373,27 +333,27 @@ isc_task_unsend(isc_task_t *task, void *sender, isc_eventtype_t type, void *tag, * * Notes: * - *\li This function is equivalent to + *\li This function is equivalent to * *\code - * isc_task_unsendrange(task, sender, type, type, tag, events); + * isc_task_unsendrange(task, sender, type, type, tag, events); *\endcode * * Requires: * - *\li 'task' is a valid task. + *\li 'task' is a valid task. * - *\li *events is a valid list. + *\li *events is a valid list. * * Ensures: * - *\li Events in the event queue of 'task' whose sender is 'sender', whose - * type is 'type', and whose tag is 'tag' will be dequeued and appended - * to *events. + *\li Events in the event queue of 'task' whose sender is 'sender', whose + * type is 'type', and whose tag is 'tag' will be dequeued and appended + * to *events. * * Returns: * - *\li The number of events unsent. + *\li The number of events unsent. */ isc_result_t diff --git a/lib/isc/task.c b/lib/isc/task.c index 81909c8c36..7698c84492 100644 --- a/lib/isc/task.c +++ b/lib/isc/task.c @@ -614,12 +614,10 @@ isc_task_purge(isc_task_t *task, void *sender, isc_eventtype_t type, bool isc_task_purgeevent(isc_task_t *task, isc_event_t *event) { - isc_event_t *curr_event, *next_event; + bool found = false; /* * Purge 'event' from a task's event queue. - * - * XXXRTH: WARNING: This method may be removed before beta. */ REQUIRE(VALID_TASK(task)); @@ -635,40 +633,22 @@ isc_task_purgeevent(isc_task_t *task, isc_event_t *event) { */ LOCK(&task->lock); - for (curr_event = HEAD(task->events); curr_event != NULL; - curr_event = next_event) - { - next_event = NEXT(curr_event, ev_link); - if (curr_event == event && PURGE_OK(event)) { - DEQUEUE(task->events, curr_event, ev_link); - task->nevents--; - break; - } + if (ISC_LINK_LINKED(event, ev_link)) { + DEQUEUE(task->events, event, ev_link); + task->nevents--; + found = true; } UNLOCK(&task->lock); - if (curr_event == NULL) { + if (!found) { return (false); } - isc_event_free(&curr_event); + isc_event_free(&event); return (true); } -unsigned int -isc_task_unsendrange(isc_task_t *task, void *sender, isc_eventtype_t first, - isc_eventtype_t last, void *tag, isc_eventlist_t *events) { - /* - * Remove events from a task's event queue. - */ - REQUIRE(VALID_TASK(task)); - - XTRACE("isc_task_unsendrange"); - - return (dequeue_events(task, sender, first, last, tag, events, false)); -} - unsigned int isc_task_unsend(isc_task_t *task, void *sender, isc_eventtype_t type, void *tag, isc_eventlist_t *events) { diff --git a/lib/isc/tests b/lib/isc/tests deleted file mode 120000 index 9c8d7ce1d0..0000000000 --- a/lib/isc/tests +++ /dev/null @@ -1 +0,0 @@ -../../tests/isc \ No newline at end of file diff --git a/tests/isc/task_test.c b/tests/isc/task_test.c index 69492db851..e31b54c77f 100644 --- a/tests/isc/task_test.c +++ b/tests/isc/task_test.c @@ -1389,19 +1389,6 @@ ISC_RUN_TEST_IMPL(purgeevent) { try_purgeevent(true); } -/* - * Purge event not purgeable test: - * When the event is not marked as purgable, a call to - * isc_task_purgeevent(task, event) does not purge the event - * 'event' from the task's queue and returns false. - */ - -ISC_RUN_TEST_IMPL(purgeevent_notpurge) { - UNUSED(state); - - try_purgeevent(false); -} - ISC_TEST_LIST_START ISC_TEST_ENTRY_CUSTOM(manytasks, _setup4, _teardown) @@ -1413,7 +1400,6 @@ ISC_TEST_ENTRY_CUSTOM(privilege_drop, _setup, _teardown) ISC_TEST_ENTRY_CUSTOM(privileged_events, _setup, _teardown) ISC_TEST_ENTRY_CUSTOM(purge, _setup2, _teardown) ISC_TEST_ENTRY_CUSTOM(purgeevent, _setup2, _teardown) -ISC_TEST_ENTRY_CUSTOM(purgeevent_notpurge, _setup2, _teardown) ISC_TEST_ENTRY_CUSTOM(task_shutdown, _setup4, _teardown) ISC_TEST_ENTRY_CUSTOM(task_exclusive, _setup4, _teardown) From 5f141e2c7f576950e58955bd3635f79105c4a0c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 31 Mar 2022 21:59:57 +0200 Subject: [PATCH 2/8] Keep the list of scheduled events on the timer Instead of searching for the events to purge, keep the list of scheduled events on the timer list and purge the events that we have scheduled. (cherry picked from commit 3f8024b4a2f12fcd28a9dd813b6f1f3f11d506f2) --- lib/isc/include/isc/timer.h | 7 +++++-- lib/isc/timer.c | 33 ++++++++++++++++++++++++--------- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/lib/isc/include/isc/timer.h b/lib/isc/include/isc/timer.h index b86d74e4aa..1e828d6bc3 100644 --- a/lib/isc/include/isc/timer.h +++ b/lib/isc/include/isc/timer.h @@ -87,10 +87,13 @@ typedef enum { isc_timertype_inactive = 3 /*%< Inactive */ } isc_timertype_t; -typedef struct isc_timerevent { +typedef struct isc_timerevent isc_timerevent_t; + +struct isc_timerevent { struct isc_event common; isc_time_t due; -} isc_timerevent_t; + ISC_LINK(isc_timerevent_t) ev_timerlink; +}; #define ISC_TIMEREVENT_FIRSTEVENT (ISC_EVENTCLASS_TIMER + 0) #define ISC_TIMEREVENT_TICK (ISC_EVENTCLASS_TIMER + 1) diff --git a/lib/isc/timer.c b/lib/isc/timer.c index 1b278eff30..7bebc7c7a9 100644 --- a/lib/isc/timer.c +++ b/lib/isc/timer.c @@ -72,6 +72,7 @@ struct isc_timer { void *arg; unsigned int index; isc_time_t due; + ISC_LIST(isc_timerevent_t) active; LINK(isc_timer_t) link; }; @@ -204,18 +205,31 @@ deschedule(isc_timer_t *timer) { } } +static void +timerevent_unlink(isc_timer_t *timer, isc_timerevent_t *event) { + fprintf(stderr, "unlinking %p from %p\n", event, &timer->active); + + REQUIRE(ISC_LINK_LINKED(event, ev_timerlink)); + ISC_LIST_UNLINK(timer->active, event, ev_timerlink); +} + +static void +timer_purge(isc_timer_t *timer) { + isc_timerevent_t *event = NULL; + + while ((event = ISC_LIST_HEAD(timer->active)) != NULL) { + (void)isc_task_purgeevent(timer->task, (isc_event_t *)event); + timerevent_unlink(timer, event); + } +} + static void destroy(isc_timer_t *timer) { isc_timermgr_t *manager = timer->manager; - /* - * The caller must ensure it is safe to destroy the timer. - */ - LOCK(&manager->lock); - (void)isc_task_purgerange(timer->task, timer, ISC_TIMEREVENT_FIRSTEVENT, - ISC_TIMEREVENT_LASTEVENT, NULL); + timer_purge(timer); deschedule(timer); UNLINK(manager->timers, timer, link); @@ -309,6 +323,9 @@ isc_timer_create(isc_timermgr_t *manager, isc_timertype_t type, timer->index = 0; isc_mutex_init(&timer->lock); ISC_LINK_INIT(timer, link); + + ISC_LIST_INIT(timer->active); + timer->magic = TIMER_MAGIC; LOCK(&manager->lock); @@ -388,9 +405,7 @@ isc_timer_reset(isc_timer_t *timer, isc_timertype_t type, LOCK(&timer->lock); if (purge) { - (void)isc_task_purgerange(timer->task, timer, - ISC_TIMEREVENT_FIRSTEVENT, - ISC_TIMEREVENT_LASTEVENT, NULL); + timer_purge(timer); } timer->type = type; timer->expires = *expires; From 68abe3fa06a8ba2ff00320a3e9bf381de82c1aa2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 1 Apr 2022 10:40:37 +0200 Subject: [PATCH 3/8] Add isc_task_setquantum() and use it for post-init zone loading Add isc_task_setquantum() function that modifies quantum for the future isc_task_run() invocations. NOTE: The current isc_task_run() caches the task->quantum into a local variable and therefore the current event loop is not affected by any quantum change. (cherry picked from commit 15ea6f002ffbe1f3463639d3fe8c0bc07526a6d8) --- lib/isc/include/isc/task.h | 9 +++++++++ lib/isc/task.c | 16 ++++++++++++++-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/lib/isc/include/isc/task.h b/lib/isc/include/isc/task.h index 973cb30284..75b7cdb9d1 100644 --- a/lib/isc/include/isc/task.h +++ b/lib/isc/include/isc/task.h @@ -488,6 +488,15 @@ isc_task_gettag(isc_task_t *task); *\li 'task' is a valid task. */ +void +isc_task_setquantum(isc_task_t *task, unsigned int quantum); +/*%< + * Set future 'task' quantum to 'quantum'. The current 'task' quantum will be + * kept for the current isc_task_run() loop, and will be changed for the next + * run. Therefore, the function is save to use from the event callback as it + * will not affect the current event loop processing. + */ + isc_result_t isc_task_beginexclusive(isc_task_t *task); /*%< diff --git a/lib/isc/task.c b/lib/isc/task.c index 7698c84492..439d430790 100644 --- a/lib/isc/task.c +++ b/lib/isc/task.c @@ -761,6 +761,16 @@ isc_task_getnetmgr(isc_task_t *task) { return (task->manager->netmgr); } +void +isc_task_setquantum(isc_task_t *task, unsigned int quantum) { + REQUIRE(VALID_TASK(task)); + + LOCK(&task->lock); + task->quantum = (quantum > 0) ? quantum + : task->manager->default_quantum; + UNLOCK(&task->lock); +} + /*** *** Task Manager. ***/ @@ -771,11 +781,13 @@ task_run(isc_task_t *task) { bool finished = false; isc_event_t *event = NULL; isc_result_t result = ISC_R_SUCCESS; + uint32_t quantum; REQUIRE(VALID_TASK(task)); LOCK(&task->lock); - /* FIXME */ + quantum = task->quantum; + if (task->state != task_state_ready) { goto done; } @@ -849,7 +861,7 @@ task_run(isc_task_t *task) { task->state = task_state_idle; } break; - } else if (dispatch_count >= task->quantum) { + } else if (dispatch_count >= quantum) { /* * Our quantum has expired, but there is more work to be * done. We'll requeue it to the ready queue later. From a7055b01afcc1c484e434316cf8d4518a10f3374 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 31 Mar 2022 22:15:49 +0200 Subject: [PATCH 4/8] Set quantum to infinity for the zone loading task When we are loading the zones, set the quantum to UINT_MAX, which makes task_run process all tasks at once. After the zone loading is finished the quantum will be dropped to 1 to not block server when we are loading new zones after reconfiguration. (cherry picked from commit 87c4c24cdeb858decaf11232bd413a8a815b4732) --- lib/dns/zone.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 50e2e6ef1d..556bd65b20 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -2414,6 +2414,9 @@ zone_asyncload(isc_task_t *task, isc_event_t *event) { (asl->loaded)(asl->loaded_arg, zone, task); } + /* Reduce the quantum */ + isc_task_setquantum(zone->loadtask, 1); + isc_mem_put(zone->mctx, asl, sizeof(*asl)); dns_zone_idetach(&zone); } @@ -19120,7 +19123,7 @@ dns_zonemgr_setsize(dns_zonemgr_t *zmgr, int num_zones) { pool = NULL; if (zmgr->loadtasks == NULL) { result = isc_taskpool_create(zmgr->taskmgr, zmgr->mctx, ntasks, - 2, true, &pool); + UINT_MAX, true, &pool); } else { result = isc_taskpool_expand(&zmgr->loadtasks, ntasks, true, &pool); From 7197cf2b7e0329dd8503dc0ec9459a15bd7274ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 31 Mar 2022 22:14:14 +0200 Subject: [PATCH 5/8] Remove isc_task_purge() and isc_task_purgerange() The isc_task_purge() and isc_task_purgerange() were now unused, so sweep the task.c file. Additionally remove unused ISC_EVENTATTR_NOPURGE event attribute. (cherry picked from commit c17eee034be9c21c4e49a4b4ab338754d1d9b1b5) --- lib/isc/timer.c | 70 +++++++++++++++++++++++++++++++++---------------- 1 file changed, 47 insertions(+), 23 deletions(-) diff --git a/lib/isc/timer.c b/lib/isc/timer.c index 7bebc7c7a9..33daa554cd 100644 --- a/lib/isc/timer.c +++ b/lib/isc/timer.c @@ -207,19 +207,38 @@ deschedule(isc_timer_t *timer) { static void timerevent_unlink(isc_timer_t *timer, isc_timerevent_t *event) { - fprintf(stderr, "unlinking %p from %p\n", event, &timer->active); - REQUIRE(ISC_LINK_LINKED(event, ev_timerlink)); ISC_LIST_UNLINK(timer->active, event, ev_timerlink); } +static void +timerevent_destroy(isc_event_t *event0) { + isc_timer_t *timer = event0->ev_destroy_arg; + isc_timerevent_t *event = (isc_timerevent_t *)event0; + + if (ISC_LINK_LINKED(event, ev_timerlink)) { + /* The event was unlinked via timer_purge() */ + timerevent_unlink(timer, event); + } + + isc_mem_put(timer->manager->mctx, event, event0->ev_size); + isc_timer_detach(&timer); +} + static void timer_purge(isc_timer_t *timer) { isc_timerevent_t *event = NULL; while ((event = ISC_LIST_HEAD(timer->active)) != NULL) { - (void)isc_task_purgeevent(timer->task, (isc_event_t *)event); - timerevent_unlink(timer, event); + bool purged = isc_task_purgeevent(timer->task, + (isc_event_t *)event); + if (!purged) { + /* + * The event has already been executed, but not + * yet destroyed. + */ + timerevent_unlink(timer, event); + } } } @@ -487,7 +506,6 @@ isc_timer_attach(isc_timer_t *timer, isc_timer_t **timerp) { void isc_timer_detach(isc_timer_t **timerp) { isc_timer_t *timer; - /* * Detach *timerp from its timer. */ @@ -503,10 +521,32 @@ isc_timer_detach(isc_timer_t **timerp) { *timerp = NULL; } +static void +timer_post_event(isc_timermgr_t *manager, isc_timer_t *timer, + isc_eventtype_t type) { + isc_timerevent_t *event; + XTRACEID("posting", timer); + + event = (isc_timerevent_t *)isc_event_allocate( + manager->mctx, timer, type, timer->action, timer->arg, + sizeof(*event)); + + 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; + + ISC_LIST_APPEND(timer->active, event, ev_timerlink); + + isc_task_send(timer->task, ISC_EVENT_PTR(&event)); +} + static void dispatch(isc_timermgr_t *manager, isc_time_t *now) { bool done = false, post_event, need_schedule; - isc_timerevent_t *event; isc_eventtype_t type = 0; isc_timer_t *timer; isc_result_t result; @@ -568,23 +608,7 @@ dispatch(isc_timermgr_t *manager, isc_time_t *now) { } if (post_event) { - XTRACEID("posting", timer); - /* - * XXX We could preallocate this event. - */ - event = (isc_timerevent_t *)isc_event_allocate( - manager->mctx, timer, type, - timer->action, timer->arg, - sizeof(*event)); - - if (event != NULL) { - event->due = timer->due; - isc_task_send(timer->task, - ISC_EVENT_PTR(&event)); - } else { - UNEXPECTED_ERROR("couldn't allocate " - "event"); - } + timer_post_event(manager, timer, type); } timer->index = 0; 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 6/8] 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); } From 80a052aaf68896979f1597113af6729c933a5464 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Wed, 6 Apr 2022 15:52:24 +1000 Subject: [PATCH 7/8] Unlink the timer event before trying to purge it as far as I can determine the order of operations is not important. *** CID 351372: Concurrent data access violations (ATOMICITY) /lib/isc/timer.c: 227 in timer_purge() 221 LOCK(&timer->lock); 222 if (!purged) { 223 /* 224 * The event has already been executed, but not 225 * yet destroyed. 226 */ >>> CID 351372: Concurrent data access violations (ATOMICITY) >>> Using an unreliable value of "event" inside the second locked section. If the data that "event" depends on was changed by another thread, this use might be incorrect. 227 timerevent_unlink(timer, event); 228 } 229 } 230 } 231 232 void (cherry picked from commit 98718b3b4b1604935aca952477b7ac97dc32557d) --- lib/isc/timer.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/lib/isc/timer.c b/lib/isc/timer.c index 11bf197a29..40ae929ce8 100644 --- a/lib/isc/timer.c +++ b/lib/isc/timer.c @@ -230,17 +230,10 @@ timer_purge(isc_timer_t *timer) { isc_timerevent_t *event = NULL; while ((event = ISC_LIST_HEAD(timer->active)) != NULL) { + timerevent_unlink(timer, event); UNLOCK(&timer->lock); - bool purged = isc_task_purgeevent(timer->task, - (isc_event_t *)event); + (void)isc_task_purgeevent(timer->task, (isc_event_t *)event); LOCK(&timer->lock); - if (!purged) { - /* - * The event has already been executed, but not - * yet destroyed. - */ - timerevent_unlink(timer, event); - } } } From a7e8e4382936114516fa336b08f9df2da55876ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 1 Apr 2022 10:59:28 +0200 Subject: [PATCH 8/8] Add CHANGES note for [GL #3252] (cherry picked from commit a7cd0868a249d10918a225aee8a6c5e208d7187b) --- CHANGES | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGES b/CHANGES index 20fe610301..929d9c9155 100644 --- a/CHANGES +++ b/CHANGES @@ -66,6 +66,13 @@ 6044. [bug] There was an "RSASHA236" typo in a log message. [GL !7206] +5845. [bug] Refactor the timer to keep track of posted events + as to use isc_task_purgeevent() instead of using + isc_task_purgerange(). The isc_task_purgeevent() + has been refactored to purge a single event instead + of walking through the list of posted events. + [GL #3252] + 5830. [func] Implement incremental resizing of isc_ht hash tables to perform the rehashing gradually. [GL #3212]