Merge branch 'aram/isc_task_purgeevent-race-fix' into 'bind-9.18'

Fix a data race in isc_task_purgeevent()

See merge request isc-projects/bind9!8937

CHANGES

@@ -1,3 +1,5 @@
+6390.	[bug]		Fix a data race in isc_task_purgeevent(). [GL !8937]
+
 6389.	[bug]		dnssec-verify and dnssec-signzone could fail if there
 			was an obscured DNSKEY RRset at a delegatation.
 			[GL #4517]

lib/isc/include/isc/timer.h

@@ -224,6 +224,21 @@ isc_timer_touch(isc_timer_t *timer);
  *\li	Unexpected error
  */
 
+void
+isc_timer_purge(isc_timer_t *timer);
+/*%<
+ * Purge timer.
+ *
+ * Requires:
+ *
+ *\li	'timer' points to a valid timer.
+ *
+ * Ensures:
+ *
+ *\li	Any events already posted by the timer are purged.
+ *
+ */
+
 void
 isc_timer_destroy(isc_timer_t **timerp);
 /*%<

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

lib/isc/timer.c

@@ -231,8 +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);
-		(void)isc_task_purgeevent(timer->task, (isc_event_t *)event);
+#if defined(UNIT_TESTING)
+		usleep(100);
+#endif
+		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);
 	}
 }
@@ -469,6 +484,15 @@ isc_timer_touch(isc_timer_t *timer) {
 	return (result);
 }
 
+void
+isc_timer_purge(isc_timer_t *timer) {
+	REQUIRE(VALID_TIMER(timer));
+
+	LOCK(&timer->lock);
+	timer_purge(timer);
+	UNLOCK(&timer->lock);
+}
+
 void
 isc_timer_destroy(isc_timer_t **timerp) {
 	isc_timer_t *timer = NULL;

tests/isc/task_test.c

@@ -38,6 +38,7 @@
 #include <isc/util.h>
 
 #include "netmgr/uv-compat.h"
+#include "timer.c"
 
 #include <tests/isc.h>
 
@@ -400,8 +401,18 @@ ISC_RUN_TEST_IMPL(privilege_drop) {
 }
 
 /*
- * Basic task functions:
+ * Basic task variables and functions:
  */
+
+static char one[] = "1";
+static char two[] = "2";
+static char three[] = "3";
+static char four[] = "4";
+static char tick[] = "tick";
+static char tock[] = "tock";
+static char quick[] = "quick";
+static isc_timer_t *ti3 = NULL;
+
 static void
 basic_cb(isc_task_t *task, isc_event_t *event) {
 	int i, j;
@@ -434,7 +445,27 @@ basic_shutdown(isc_task_t *task, isc_event_t *event) {
 }
 
 static void
-basic_tick(isc_task_t *task, isc_event_t *event) {
+basic_tick1(isc_task_t *task, isc_event_t *event) {
+	UNUSED(task);
+
+	if (verbose) {
+		print_message("# %s\n", (char *)event->ev_arg);
+	}
+
+	/* Test for a race condition with isc_event_free() in basic_quick(). */
+	if (!atomic_load(&done)) {
+		LOCK(&lock);
+		if (ti3 != NULL) {
+			isc_timer_purge(ti3);
+		}
+		UNLOCK(&lock);
+	}
+
+	isc_event_free(&event);
+}
+
+static void
+basic_tick2(isc_task_t *task, isc_event_t *event) {
 	UNUSED(task);
 
 	if (verbose) {
@@ -444,12 +475,12 @@ basic_tick(isc_task_t *task, isc_event_t *event) {
 	isc_event_free(&event);
 }
 
-static char one[] = "1";
-static char two[] = "2";
-static char three[] = "3";
-static char four[] = "4";
-static char tick[] = "tick";
-static char tock[] = "tock";
+static void
+basic_quick(isc_task_t *task, isc_event_t *event) {
+	UNUSED(task);
+
+	isc_event_free(&event);
+}
 
 ISC_RUN_TEST_IMPL(basic) {
 	isc_result_t result;
@@ -466,15 +497,22 @@ ISC_RUN_TEST_IMPL(basic) {
 			      one, two, three, four, two, three, four, NULL };
 	int i;
 
+	atomic_init(&done, false);
+
 	UNUSED(state);
 
-	result = isc_task_create(taskmgr, 0, &task1);
+	/*
+	 * Note: running task1 and task4 on different threads, because they
+	 * test a race condition.
+	 */
+
+	result = isc_task_create_bound(taskmgr, 0, &task1, 0);
 	assert_int_equal(result, ISC_R_SUCCESS);
 	result = isc_task_create(taskmgr, 0, &task2);
 	assert_int_equal(result, ISC_R_SUCCESS);
 	result = isc_task_create(taskmgr, 0, &task3);
 	assert_int_equal(result, ISC_R_SUCCESS);
-	result = isc_task_create(taskmgr, 0, &task4);
+	result = isc_task_create_bound(taskmgr, 0, &task4, 1);
 	assert_int_equal(result, ISC_R_SUCCESS);
 
 	result = isc_task_onshutdown(task1, basic_shutdown, one);
@@ -489,14 +527,21 @@ ISC_RUN_TEST_IMPL(basic) {
 	isc_time_settoepoch(&absolute);
 	isc_interval_set(&interval, 1, 0);
 	result = isc_timer_create(timermgr, isc_timertype_ticker, &absolute,
-				  &interval, task1, basic_tick, tick, &ti1);
+				  &interval, task1, basic_tick1, tick, &ti1);
 	assert_int_equal(result, ISC_R_SUCCESS);
 
 	ti2 = NULL;
 	isc_time_settoepoch(&absolute);
 	isc_interval_set(&interval, 1, 0);
 	result = isc_timer_create(timermgr, isc_timertype_ticker, &absolute,
-				  &interval, task2, basic_tick, tock, &ti2);
+				  &interval, task2, basic_tick2, tock, &ti2);
+	assert_int_equal(result, ISC_R_SUCCESS);
+
+	ti3 = NULL;
+	isc_time_settoepoch(&absolute);
+	isc_interval_set(&interval, 0, 1000);
+	result = isc_timer_create(timermgr, isc_timertype_ticker, &absolute,
+				  &interval, task4, basic_quick, quick, &ti3);
 	assert_int_equal(result, ISC_R_SUCCESS);
 
 	sleep(2);
@@ -524,9 +569,14 @@ ISC_RUN_TEST_IMPL(basic) {
 	isc_task_detach(&task3);
 	isc_task_detach(&task4);
 
+	atomic_store(&done, true);
+
 	sleep(10);
 	isc_timer_destroy(&ti1);
 	isc_timer_destroy(&ti2);
+	LOCK(&lock);
+	isc_timer_destroy(&ti3);
+	UNLOCK(&lock);
 }
 
 /*
@@ -1348,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.