From 51753458c419527eb706cc417692bda461df85a2 Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w@1wt.eu>
Date: Thu, 23 Nov 2017 18:36:50 +0100
Subject: [PATCH] BUG/MAJOR: threads/task: dequeue expired tasks under the WQ
 lock

There is a small unprotected window for a task between the wait queue
and the run queue where a task could be woken up and destroyed at the
same time. What typically happens is that a timeout is reached at the
same time an I/O completes and wakes it up, and the I/O terminates the
task, causing a use after free in wake_expired_tasks() possibly causing
a crash and/or memory corruption :

       thread 1                             thread 2
  (wake_expired_tasks)                (stream_int_notify)

 HA_SPIN_UNLOCK(TASK_WQ_LOCK, &wq_lock);
                              task_wakeup(task, TASK_WOKEN_IO);
                              ...
                              process_stream()
                                stream_free()
                                   task_free()
                                      pool_free(task)
 task_wakeup(task, TASK_WOKEN_TIMER);

This case is reasonably easy to reproduce with a config using very short
server timeouts (100ms) and client timeouts (10ms), while injecting on
httpterm requesting medium sized objects (5kB) over SSL. All this is
easier done with more threads than allocated CPUs so that pauses can
happen anywhere and last long enough for process_stream() to kill the
task.

This patch inverts the lock and the wakeup(), but requires some changes
in process_runnable_tasks() to ensure we never try to grab the WQ lock
while having the RQ lock held. This means we have to release the RQ lock
before calling task_queue(), so we can't hold the RQ lock during the
loop and must take and drop it.

It seems that a different approach with the scope-aware trees could be
easier, but it would possibly not cover situations where a task is
allowed to run on multiple threads. The current solution covers it and
doesn't seem to have any measurable performance impact.
---
 src/task.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/task.c b/src/task.c
index f48807de8..25776fe8f 100644
--- a/src/task.c
+++ b/src/task.c
@@ -162,8 +162,8 @@ int wake_expired_tasks()
 				__task_queue(task);
 			goto lookup_next;
 		}
-		HA_SPIN_UNLOCK(TASK_WQ_LOCK, &wq_lock);
 		task_wakeup(task, TASK_WOKEN_TIMER);
+		HA_SPIN_UNLOCK(TASK_WQ_LOCK, &wq_lock);
 	}
 
 	HA_SPIN_UNLOCK(TASK_WQ_LOCK, &wq_lock);
@@ -306,21 +306,27 @@ void process_runnable_tasks()
 				local_tasks[final_tasks_count++] = t;
 		}
 
-		HA_SPIN_LOCK(TASK_RQ_LOCK, &rq_lock);
 		for (i = 0; i < final_tasks_count ; i++) {
 			t = local_tasks[i];
-			t->state &= ~TASK_RUNNING;
 			/* If there is a pending state
 			 * we have to wake up the task
 			 * immediatly, else we defer
 			 * it into wait queue
 			 */
-			if (t->pending_state)
+			HA_SPIN_LOCK(TASK_RQ_LOCK, &rq_lock);
+			t->state &= ~TASK_RUNNING;
+			if (t->pending_state) {
 				__task_wakeup(t);
-			else
+				HA_SPIN_UNLOCK(TASK_RQ_LOCK, &rq_lock);
+			}
+			else {
+				/* we must never hold the RQ lock before the WQ lock */
+				HA_SPIN_UNLOCK(TASK_RQ_LOCK, &rq_lock);
 				task_queue(t);
+			}
 		}
 
+		HA_SPIN_LOCK(TASK_RQ_LOCK, &rq_lock);
 		if (max_processed <= 0) {
 			active_tasks_mask |= tid_bit;
 			break;