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.

diff --git a/lib/isc/task.c b/lib/isc/task.c
index 439d43079007d80acb538923e95a5efd64138893..48c3e790f4fc085cf038164f37cbb338b69890df 100644 (file)
--- 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 53a820c2c38502def0408a8b05dcb454e08a0147..5d3d3b627a1c9e89f4a5d0c38cd8540dfe1adcb6 100644 (file)
--- 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 d1b0c8913362bfe139b00314e5fef9185731caab..d718adcff25b8e8383017322ee3a459125ecda45 100644 (file)
--- 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.