From 0a48252b53c93eb241108192b386b908e0d73474 Mon Sep 17 00:00:00 2001
From: Aram Sargsyan <aram@isc.org>
Date: Thu, 4 Apr 2024 15:28:47 +0000
Subject: [PATCH] Fix a data race in isc_task_purgeevent()

When isc_task_purgeevent() is called for and 'event', the event, in
the meanwhile, could in theory get processed, unlinked, and freed.
So when the function then operates on the 'event', it causes a
segmentation fault.

The only place where isc_task_purgeevent() is called is from
timer_purge().

In order to resolve the data race, call isc_task_purgeevent() inside
the 'timer->lock' locked block, so that timerevent_destroy() won't
be able to destroy the event if it was processed in the meanwhile,
before isc_task_purgeevent() had a chance to purge it.

In order to be able to do that, move the responsibility of calling
isc_event_free() (upon a successful purge) out from the
isc_task_purgeevent() function to its caller instead, so that it can
be called outside of the timer->lock locked block.
---
 lib/isc/task.c        | 15 ++++++++++-----
 lib/isc/timer.c       | 14 +++++++++++++-
 tests/isc/task_test.c |  3 +++
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/lib/isc/task.c b/lib/isc/task.c
index 439d430790..48c3e790f4 100644
--- a/lib/isc/task.c
+++ b/lib/isc/task.c
@@ -612,6 +612,15 @@ isc_task_purge(isc_task_t *task, void *sender, isc_eventtype_t type,
 	return (isc_task_purgerange(task, sender, type, type, tag));
 }
 
+/*
+ * The caller is responsible for freeing the event if this function returns
+ * true. If it returns false, then the event was already processed before it
+ * could be purged, so the event's action is responsible for freeing the event.
+ * The caller however must make sure that the event's destroy function, called
+ * when the event's action calls isc_event_destroy(), doesn't free the event
+ * while this function is still running. That is, 'event' must remain valid
+ * throughout the whole execution of this function.
+ */
 bool
 isc_task_purgeevent(isc_task_t *task, isc_event_t *event) {
 	bool found = false;
@@ -625,9 +634,7 @@ isc_task_purgeevent(isc_task_t *task, isc_event_t *event) {
 	/*
 	 * If 'event' is on the task's event queue, it will be purged,
 	 * unless it is marked as unpurgeable.  'event' does not have to be
-	 * on the task's event queue; in fact, it can even be an invalid
-	 * pointer.  Purging only occurs if the event is actually on the task's
-	 * event queue.
+	 * on the task's event queue.
 	 *
 	 * Purging never changes the state of the task.
 	 */
@@ -644,8 +651,6 @@ isc_task_purgeevent(isc_task_t *task, isc_event_t *event) {
 		return (false);
 	}
 
-	isc_event_free(&event);
-
 	return (true);
 }
 
diff --git a/lib/isc/timer.c b/lib/isc/timer.c
index 53a820c2c3..5d3d3b627a 100644
--- a/lib/isc/timer.c
+++ b/lib/isc/timer.c
@@ -231,11 +231,23 @@ timer_purge(isc_timer_t *timer) {
 
 	while ((event = ISC_LIST_HEAD(timer->active)) != NULL) {
 		timerevent_unlink(timer, event);
+		bool purged = isc_task_purgeevent(timer->task,
+						  (isc_event_t *)event);
 		UNLOCK(&timer->lock);
 #if defined(UNIT_TESTING)
 		usleep(100);
 #endif
-		(void)isc_task_purgeevent(timer->task, (isc_event_t *)event);
+		if (purged) {
+			isc_event_free((isc_event_t **)&event);
+		} else {
+			/*
+			 * The event was processed while we were trying to
+			 * purge it. The event's action is responsible for
+			 * calling isc_event_free(), which in turn will call
+			 * event->ev_destroy() (timerevent_destroy() here),
+			 * which will unlink and destroy it.
+			 */
+		}
 		LOCK(&timer->lock);
 	}
 }
diff --git a/tests/isc/task_test.c b/tests/isc/task_test.c
index d1b0c89133..d718adcff2 100644
--- a/tests/isc/task_test.c
+++ b/tests/isc/task_test.c
@@ -1398,6 +1398,9 @@ try_purgeevent(bool purgeable) {
 
 	purged = isc_task_purgeevent(task, event2_clone);
 	assert_int_equal(purgeable, purged);
+	if (purged) {
+		isc_event_free(&event2_clone);
+	}
 
 	/*
 	 * Unblock the task, allowing event processing.