Commit e1c4a691 authored by Witold Krecicki's avatar Witold Krecicki

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.
parent 684a44b4
......@@ -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) {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment