Fix a race in taskmgr between worker and task pausing/unpausing.

To reproduce the race - create a task, send two events to it, first one
must take some time. Then, from the outside, pause(), unpause() and detach()
the task.
When the long-running event is processed by the task it is in
task_state_running state. When we called pause() the state changed to
task_state_paused, on unpause we checked that there are events in the task
queue, changed the state to task_state_ready and enqueued the task on the
workers readyq. We then detach the task.
The dispatch() is done with processing the event, it processes the second
event in the queue, and then shuts down the task and frees it (as it's not
referenced anymore). Dispatcher then takes the, already freed, task from
the queue where it was wrongly put, causing an use-after free and,
subsequently, either an assertion failure or a segmentation fault.
The probability of this happening is very slim, yet it might happen under a
very high load, more probably on a recursive resolver than on an
authoritative.
The fix introduces a new 'task_state_pausing' state - to which tasks
are moved if they're being paused while still running. They are moved
to task_state_paused state when dispatcher is done with them, and
if we unpause a task in paused state it's moved back to task_state_running
and not requeued.

diff --git a/lib/isc/task.c b/lib/isc/task.c
index 33114993d42baca2c86f616cf602e55fe0d31ade..f859849935d6dacccc3b442b7726d39b2b3f7f45 100644 (file)
--- a/lib/isc/task.c
+++ b/lib/isc/task.c
@@ -78,13 +78,17 @@
  ***/
 
 typedef enum {
-       task_state_idle, task_state_ready, task_state_paused,
-       task_state_running, task_state_done
+       task_state_idle,        /* not doing anything, events queue empty */
+       task_state_ready,       /* waiting in worker's queue */
+       task_state_paused,      /* not running, paused */
+       task_state_pausing,     /* running, waiting to be paused */
+       task_state_running,     /* actively processing events */
+       task_state_done         /* shutting down, no events or references */
 } task_state_t;
 
 #if defined(HAVE_LIBXML2) || defined(HAVE_JSON_C)
 static const char *statenames[] = {
-       "idle", "ready", "running", "done",
+       "idle", "ready", "paused", "pausing", "running", "done",
 };
 #endif
 
@@ -381,6 +385,7 @@ task_shutdown(isc__task_t *task) {
                }
                INSIST(task->state == task_state_ready ||
                       task->state == task_state_paused ||
+                      task->state == task_state_pausing ||
                       task->state == task_state_running);
 
                /*
@@ -502,7 +507,8 @@ task_send(isc__task_t *task, isc_event_t **eventp, int c) {
        }
        INSIST(task->state == task_state_ready ||
               task->state == task_state_running ||
-              task->state == task_state_paused);
+              task->state == task_state_paused ||
+              task->state == task_state_pausing);
        ENQUEUE(task->events, event, ev_link);
        task->nevents++;
        *eventp = NULL;
@@ -1200,10 +1206,12 @@ dispatch(isc__taskmgr_t *manager, unsigned int threadid) {
                                                finished = true;
                                                task->state = task_state_done;
                                        } else {
-                                               /* It might be paused */
                                                if (task->state ==
                                                    task_state_running) {
                                                        task->state = task_state_idle;
+                                               } else if (task->state ==
+                                                          task_state_pausing) {
+                                                       task->state = task_state_paused;
                                                }
                                        }
                                        done = true;
@@ -1226,6 +1234,9 @@ dispatch(isc__taskmgr_t *manager, unsigned int threadid) {
                                                 */
                                                task->state = task_state_ready;
                                                requeue = true;
+                                       } else if (task->state ==
+                                                  task_state_pausing) {
+                                               task->state = task_state_paused;
                                        }
                                        done = true;
                                }
@@ -1682,8 +1693,12 @@ isc_task_pause(isc_task_t *task0) {
        INSIST(task->state == task_state_idle ||
               task->state == task_state_ready ||
               task->state == task_state_running);
-       running = (task->state == task_state_running);
-       task->state = task_state_paused;
+       if (task->state == task_state_running) {
+               running = true;
+               task->state = task_state_pausing;
+       } else {
+               task->state = task_state_paused;
+       }
        UNLOCK(&task->lock);
 
        if (running) {
@@ -1706,13 +1721,18 @@ isc_task_unpause(isc_task_t *task0) {
        REQUIRE(ISCAPI_TASK_VALID(task0));
 
        LOCK(&task->lock);
-       INSIST(task->state == task_state_paused);
-       if (!EMPTY(task->events)) {
-               task->state = task_state_ready;
-               was_idle = true;
+       INSIST(task->state == task_state_paused ||
+              task->state == task_state_pausing);
+       /* If the task was pausing we can't reschedule it */
+       if (task->state == task_state_pausing) {
+               task->state = task_state_running;
        } else {
                task->state = task_state_idle;
        }
+       if (task->state == task_state_idle && !EMPTY(task->events)) {
+               task->state = task_state_ready;
+               was_idle = true;
+       }
        UNLOCK(&task->lock);
 
        if (was_idle) {