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 ae01ec2823)

bin/dig/dighost.c

@@ -1584,7 +1584,7 @@ clear_query(dig_query_t *query) {
 	debug("clear_query(%p)", query);
 
 	if (query->timer != NULL) {
-		isc_timer_detach(&query->timer);
+		isc_timer_destroy(&query->timer);
 	}
 	lookup = query->lookup;
 
@@ -2700,7 +2700,7 @@ bringup_timer(dig_query_t *query, unsigned int default_timeout) {
 	debug("have local timeout of %d", local_timeout);
 	isc_interval_set(&l->interval, local_timeout, 0);
 	if (query->timer != NULL) {
-		isc_timer_detach(&query->timer);
+		isc_timer_destroy(&query->timer);
 	}
 	result = isc_timer_create(timermgr, isc_timertype_once, NULL,
 				  &l->interval, global_task, connect_timeout,
@@ -2724,7 +2724,7 @@ force_timeout(dig_query_t *query) {
 	 * ourselves due to the duplicate events.
 	 */
 	if (query->timer != NULL) {
-		isc_timer_detach(&query->timer);
+		isc_timer_destroy(&query->timer);
 	}
 }
 

bin/named/controlconf.c

@@ -177,7 +177,7 @@ maybe_free_connection(controlconnection_t *conn) {
 	}
 
 	if (conn->timer != NULL) {
-		isc_timer_detach(&conn->timer);
+		isc_timer_destroy(&conn->timer);
 	}
 
 	if (conn->ccmsg_valid) {
@@ -570,10 +570,10 @@ control_timeout(isc_task_t *task, isc_event_t *event) {
 
 	UNUSED(task);
 
-	isc_timer_detach(&conn->timer);
-	maybe_free_connection(conn);
-
 	isc_event_free(&event);
+
+	isc_timer_destroy(&conn->timer);
+	maybe_free_connection(conn);
 }
 
 static isc_result_t
@@ -621,7 +621,7 @@ cleanup:
 	}
 	isccc_ccmsg_invalidate(&conn->ccmsg);
 	if (conn->timer != NULL) {
-		isc_timer_detach(&conn->timer);
+		isc_timer_destroy(&conn->timer);
 	}
 	isc_mem_put(listener->mctx, conn, sizeof(*conn));
 #ifdef ENABLE_AFL

bin/named/server.c

@@ -9925,10 +9925,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);
 

bin/tests/optional/ratelimiter_test.c

@@ -86,7 +86,7 @@ shutdown_all(isc_task_t *task, isc_event_t *event) {
 	UNUSED(event);
 	printf("shutdown all\n");
 	for (i = 0; i < NEVENTS; i++) {
-		isc_timer_detach(&timers[i]);
+		isc_timer_destroy(&timers[i]);
 	}
 
 	isc_app_shutdown();

bin/tests/optional/shutdown_test.c

@@ -73,7 +73,7 @@ shutdown_action(isc_task_t *task, isc_event_t *event) {
 
 	printf("task %s (%p) shutdown\n", info->name, task);
 	if (strcmp(info->name, "0") == 0) {
-		isc_timer_detach(&info->timer);
+		isc_timer_destroy(&info->timer);
 		nevent = isc_event_allocate(info->mctx, info, T2_SHUTDOWNOK,
 					    t2_shutdown, &tasks[1],
 					    sizeof(*event));
@@ -104,7 +104,7 @@ tick(isc_task_t *task, isc_event_t *event) {
 		if (info->ticks == 10) {
 			isc_app_shutdown();
 		} else if (info->ticks >= 15 && info->exiting) {
-			isc_timer_detach(&info->timer);
+			isc_timer_destroy(&info->timer);
 			isc_task_detach(&info->task);
 			nevent = isc_event_allocate(
 				info->mctx, info, T2_SHUTDOWNDONE, t1_shutdown,
@@ -114,7 +114,7 @@ tick(isc_task_t *task, isc_event_t *event) {
 			isc_task_detach(&info->peer);
 		}
 	} else if (strcmp(info->name, "foo") == 0) {
-		isc_timer_detach(&info->timer);
+		isc_timer_destroy(&info->timer);
 		nevent = isc_event_allocate(info->mctx, info, FOO_EVENT,
 					    foo_event, task, sizeof(*event));
 		RUNTIME_CHECK(nevent != NULL);

bin/tests/optional/sock_test.c

@@ -245,7 +245,7 @@ timeout(isc_task_t *task, isc_event_t *event) {
 	printf("Timeout, canceling IO on socket %p (task %p)\n", sock, task);
 
 	isc_socket_cancel(sock, NULL, ISC_SOCKCANCEL_ALL);
-	isc_timer_detach((isc_timer_t **)&event->ev_sender);
+	isc_timer_destroy((isc_timer_t **)&event->ev_sender);
 	isc_event_free(&event);
 }
 

bin/tests/optional/task_test.c

@@ -194,8 +194,8 @@ main(int argc, char *argv[]) {
 	Sleep(10000);
 #endif /* ifndef WIN32 */
 	printf("destroy\n");
-	isc_timer_detach(&ti1);
-	isc_timer_detach(&ti2);
+	isc_timer_destroy(&ti1);
+	isc_timer_destroy(&ti2);
 	isc_timermgr_destroy(&timgr);
 	isc_managers_destroy(&netmgr, &taskmgr);
 	printf("destroyed\n");

bin/tests/optional/timer_test.c

@@ -164,9 +164,9 @@ main(int argc, char *argv[]) {
 	Sleep(15000);
 #endif /* ifndef WIN32 */
 	printf("destroy\n");
-	isc_timer_detach(&ti1);
-	isc_timer_detach(&ti2);
-	isc_timer_detach(&ti3);
+	isc_timer_destroy(&ti1);
+	isc_timer_destroy(&ti2);
+	isc_timer_destroy(&ti3);
 #ifndef WIN32
 	sleep(2);
 #else  /* ifndef WIN32 */

lib/dns/catz.c

@@ -808,7 +808,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) {
 			dns_db_updatenotify_unregister(
 				zone->db, dns_catz_dbupdate_callback,

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);

lib/dns/request.c

@@ -1298,6 +1298,9 @@ req_connected(isc_task_t *task, isc_event_t *event) {
 
 	req_log(ISC_LOG_DEBUG(3), "req_connected: request %p", request);
 
+	result = sevent->result;
+	isc_event_free(&event);
+
 	LOCK(&request->requestmgr->locks[request->hash]);
 	request->flags &= ~DNS_REQUEST_F_CONNECTING;
 
@@ -1312,7 +1315,6 @@ req_connected(isc_task_t *task, isc_event_t *event) {
 		}
 	} else {
 		dns_dispatch_starttcp(request->dispatch);
-		result = sevent->result;
 		if (result == ISC_R_SUCCESS) {
 			result = req_send(request, task, NULL);
 		}
@@ -1323,13 +1325,13 @@ req_connected(isc_task_t *task, isc_event_t *event) {
 		}
 	}
 	UNLOCK(&request->requestmgr->locks[request->hash]);
-	isc_event_free(&event);
 }
 
 static void
 req_senddone(isc_task_t *task, isc_event_t *event) {
 	isc_socketevent_t *sevent = (isc_socketevent_t *)event;
 	dns_request_t *request = event->ev_arg;
+	isc_result_t result = sevent->result;
 
 	REQUIRE(event->ev_type == ISC_SOCKEVENT_SENDDONE);
 	REQUIRE(VALID_REQUEST(request));
@@ -1339,6 +1341,8 @@ req_senddone(isc_task_t *task, isc_event_t *event) {
 
 	UNUSED(task);
 
+	isc_event_free(&event);
+
 	LOCK(&request->requestmgr->locks[request->hash]);
 	request->flags &= ~DNS_REQUEST_F_SENDING;
 
@@ -1351,13 +1355,11 @@ req_senddone(isc_task_t *task, isc_event_t *event) {
 		} else {
 			send_if_done(request, ISC_R_CANCELED);
 		}
-	} else if (sevent->result != ISC_R_SUCCESS) {
+	} else if (result != ISC_R_SUCCESS) {
 		req_cancel(request);
 		send_if_done(request, ISC_R_CANCELED);
 	}
 	UNLOCK(&request->requestmgr->locks[request->hash]);
-
-	isc_event_free(&event);
 }
 
 static void
@@ -1407,14 +1409,18 @@ static void
 req_timeout(isc_task_t *task, isc_event_t *event) {
 	dns_request_t *request = event->ev_arg;
 	isc_result_t result;
+	isc_eventtype_t ev_type = event->ev_type;
 
 	REQUIRE(VALID_REQUEST(request));
 
 	req_log(ISC_LOG_DEBUG(3), "req_timeout: request %p", request);
 
 	UNUSED(task);
+
+	isc_event_free(&event);
+
 	LOCK(&request->requestmgr->locks[request->hash]);
-	if (event->ev_type == ISC_TIMEREVENT_TICK && request->udpcount-- != 0) {
+	if (ev_type == ISC_TIMEREVENT_TICK && request->udpcount-- != 0) {
 		if (!DNS_REQUEST_SENDING(request)) {
 			result = req_send(request, task, &request->destaddr);
 			if (result != ISC_R_SUCCESS) {
@@ -1428,7 +1434,6 @@ req_timeout(isc_task_t *task, isc_event_t *event) {
 		send_if_done(request, ISC_R_TIMEDOUT);
 	}
 	UNLOCK(&request->requestmgr->locks[request->hash]);
-	isc_event_free(&event);
 }
 
 static void
@@ -1471,7 +1476,7 @@ req_destroy(dns_request_t *request) {
 		dns_dispatch_detach(&request->dispatch);
 	}
 	if (request->timer != NULL) {
-		isc_timer_detach(&request->timer);
+		isc_timer_destroy(&request->timer);
 	}
 	if (request->tsig != NULL) {
 		isc_buffer_free(&request->tsig);
@@ -1503,7 +1508,7 @@ req_cancel(dns_request_t *request) {
 	request->flags |= DNS_REQUEST_F_CANCELED;
 
 	if (request->timer != NULL) {
-		isc_timer_detach(&request->timer);
+		isc_timer_destroy(&request->timer);
 	}
 	dispattr = dns_dispatch_getattributes(request->dispatch);
 	sock = NULL;

lib/dns/resolver.c

@@ -4706,9 +4706,9 @@ fctx_destroy(fetchctx_t *fctx) {
 
 	isc_counter_detach(&fctx->qc);
 	fcount_decr(fctx);
-	isc_timer_detach(&fctx->timer);
+	isc_timer_destroy(&fctx->timer);
 	if (fctx->timer_try_stale != NULL) {
-		isc_timer_detach(&fctx->timer_try_stale);
+		isc_timer_destroy(&fctx->timer_try_stale);
 	}
 	dns_message_detach(&fctx->qmessage);
 	if (dns_name_countlabels(&fctx->domain) > 0) {
@@ -5483,8 +5483,8 @@ cleanup_mctx:
 	isc_mem_detach(&fctx->mctx);
 	dns_adb_detach(&fctx->adb);
 	dns_db_detach(&fctx->cache);
-	isc_timer_detach(&fctx->timer);
-	isc_timer_detach(&fctx->timer_try_stale);
+	isc_timer_destroy(&fctx->timer);
+	isc_timer_destroy(&fctx->timer_try_stale);
 
 cleanup_qmessage:
 	dns_message_detach(&fctx->qmessage);
@@ -10486,7 +10486,7 @@ destroy(dns_resolver_t *res) {
 #if USE_MBSLOCK
 	isc_rwlock_destroy(&res->mbslock);
 #endif /* if USE_MBSLOCK */
-	isc_timer_detach(&res->spillattimer);
+	isc_timer_destroy(&res->spillattimer);
 	res->magic = 0;
 	isc_mem_put(res->mctx, res, sizeof(*res));
 }

lib/dns/rpz.c

@@ -2230,7 +2230,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);
 

lib/dns/xfrin.c

@@ -950,7 +950,7 @@ xfrin_create(isc_mem_t *mctx, dns_zone_t *zone, dns_db_t *db, isc_task_t *task,
 
 failure:
 	if (xfr->timer != NULL) {
-		isc_timer_detach(&xfr->timer);
+		isc_timer_destroy(&xfr->timer);
 	}
 	if (dns_name_dynamic(&xfr->name)) {
 		dns_name_free(&xfr->name, xfr->mctx);
@@ -1580,7 +1580,7 @@ maybe_free(dns_xfrin_ctx_t *xfr) {
 	}
 
 	if (xfr->timer != NULL) {
-		isc_timer_detach(&xfr->timer);
+		isc_timer_destroy(&xfr->timer);
 	}
 
 	if (xfr->task != NULL) {

lib/dns/zone.c

@@ -15016,7 +15016,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);
 	}
 

lib/isc/include/isc/timer.h

@@ -226,25 +226,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:
  *
@@ -254,9 +238,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
  *
@@ -265,9 +246,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
  */
 

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,
+				 false);
+	RUNTIME_CHECK(result == ISC_R_SUCCESS);
+	isc_timer_destroy(&rl->timer);
 
 	/*
 	 * Send an event to our task.  The delivery of this event

lib/isc/tests/task_test.c

@@ -631,8 +631,8 @@ basic(void **state) {
 #else  /* ifndef WIN32 */
 	Sleep(10000);
 #endif /* ifndef WIN32 */
-	isc_timer_detach(&ti1);
-	isc_timer_detach(&ti2);
+	isc_timer_destroy(&ti1);
+	isc_timer_destroy(&ti2);
 }
 
 /*

lib/isc/tests/timer_test.c

@@ -244,14 +244,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);
 }
 
 /*
@@ -339,9 +339,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 */
@@ -426,14 +427,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);
 }
 
 static void
@@ -591,8 +593,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);
 }

lib/isc/timer.c

@@ -62,9 +62,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;
@@ -74,7 +74,6 @@ struct isc_timer {
 	void *arg;
 	unsigned int index;
 	isc_time_t due;
-	ISC_LIST(isc_timerevent_t) active;
 	LINK(isc_timer_t) link;
 };
 
@@ -219,13 +218,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
@@ -233,8 +233,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
@@ -245,24 +247,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,
@@ -312,7 +296,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);
@@ -497,34 +480,32 @@ isc_timer_touch(isc_timer_t *timer) {
 }
 
 void
-isc_timer_attach(isc_timer_t *timer, isc_timer_t **timerp) {
-	/*
-	 * Attach *timerp to timer.
-	 */
+isc_timer_destroy(isc_timer_t **timerp) {
+	isc_timer_t *timer = NULL;
+	isc_timermgr_t *manager = NULL;
 
-	REQUIRE(VALID_TIMER(timer));
-	REQUIRE(timerp != NULL && *timerp == NULL);
-	isc_refcount_increment(&timer->references);
+	REQUIRE(timerp != NULL && VALID_TIMER(*timerp));
 
-	*timerp = timer;
-}
-
-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
@@ -539,13 +520,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));
 }

lib/isc/win32/libisc.def.in

@@ -700,9 +700,8 @@ isc_time_seconds
 isc_time_set
 isc_time_settoepoch
 isc_time_subtract
-isc_timer_attach
 isc_timer_create
-isc_timer_detach
+isc_timer_destroy
 isc_timer_gettype
 isc_timer_reset
 isc_timer_touch