libubox: runqueue: fix use-after-free bug

Fixes a use-after-free bug in runqueue_task_kill():

 Invalid read of size 8
    at runqueue_task_kill (runqueue.c:200)
    by uloop_process_timeouts (uloop.c:505)
    by uloop_run_timeout (uloop.c:542)
    by uloop_run (uloop.h:111)
    by main (tests/test-runqueue.c:126)
  Address 0x5a4b058 is 24 bytes inside a block of size 208 free'd
    at free
    by runqueue_task_complete (runqueue.c:234)
    by runqueue_task_kill (runqueue.c:199)
    by uloop_process_timeouts (uloop.c:505)
    by uloop_run_timeout (uloop.c:542)
    by uloop_run (uloop.h:111)
    by main (tests/test-runqueue.c:126)
  Block was alloc'd at
    at calloc
    by add_sleeper (tests/test-runqueue.c:101)
    by main (tests/test-runqueue.c:123)

Since commit 11e8afea (runqueue should call the complete handler from
more places) the call to the complete() callback has been moved to
runqueue_task_complete().  However in runqueue_task_kill()
runqueue_task_complete() is called before the kill() callback.  This
will result in a use after free if the complete() callback frees the
task struct.

Furthermore runqueue_start_next() is already called at the end of
runqueue_task_complete(), so there is no need to call it again in
runqueue_task_kill().

The issue was that the _complete() callback frees the memory used by the
task struct, which is then read after the _complete() callback returns.

Ref: FS#3016
Signed-off-by: Alban Bedel <albeu@free.fr>
[initial test case, kill cb comment fix]
Signed-off-by: Chris Nisbet <nischris@gmail.com>
[testcase improvements and commit subject/description tweaks]
Signed-off-by: Petr Štetiar <ynezz@true.cz>

4 files changed, 87 insertions, 31 deletions

diff --git a/runqueue.c b/runqueue.c
index a1d0133..d5719d1 100644
--- a/runqueue.c
+++ b/runqueue.c
@@ -196,11 +196,10 @@ void runqueue_task_kill(struct runqueue_task *t)
 	if (!t->queued)
 		return;
 
-	runqueue_task_complete(t);
 	if (running && t->type->kill)
 		t->type->kill(q, t);
 
-	runqueue_start_next(q);
+	runqueue_task_complete(t);
 }
 
 void runqueue_stop(struct runqueue *q)
diff --git a/runqueue.h b/runqueue.h
index 08879d4..9728c9c 100644
--- a/runqueue.h
+++ b/runqueue.h
@@ -63,7 +63,7 @@ struct runqueue_task_type {
 
 	/*
 	 * called to kill a task. must not make any calls to runqueue_task_complete,
-	 * it has already been removed from the list.
+	 * which will be called after this returns.
 	 */
 	void (*kill)(struct runqueue *q, struct runqueue_task *t);
 };
diff --git a/tests/cram/test_runqueue.t b/tests/cram/test_runqueue.t
index 227f414..cd9d96b 100644
--- a/tests/cram/test_runqueue.t
+++ b/tests/cram/test_runqueue.t
@@ -2,25 +2,35 @@ check that runqueue is producing expected results:
 
   $ [ -n "$TEST_BIN_DIR" ] && export PATH="$TEST_BIN_DIR:$PATH"
   $ valgrind --quiet --leak-check=full test-runqueue
-  [1/1] start 'sleep 1'
-  [1/1] cancel 'sleep 1'
-  [0/1] finish 'sleep 1'
-  [1/1] start 'sleep 1'
-  [1/1] cancel 'sleep 1'
-  [0/1] finish 'sleep 1'
-  [1/1] start 'sleep 1'
-  [1/1] cancel 'sleep 1'
-  [0/1] finish 'sleep 1'
+  [1/1] start 'sleep 1' (killer)
+  [1/1] killing process (killer)
+  [0/1] finish 'sleep 1' (killer) 
+  [0/1] finish 'sleep 1' (killer) 
+  [0/1] finish 'sleep 1' (killer) 
+  [1/1] start 'sleep 1' (sleeper)
+  [1/1] cancel 'sleep 1' (sleeper)
+  [0/1] finish 'sleep 1' (sleeper) 
+  [1/1] start 'sleep 1' (sleeper)
+  [1/1] cancel 'sleep 1' (sleeper)
+  [0/1] finish 'sleep 1' (sleeper) 
+  [1/1] start 'sleep 1' (sleeper)
+  [1/1] cancel 'sleep 1' (sleeper)
+  [0/1] finish 'sleep 1' (sleeper) 
   All done!
 
   $ test-runqueue-san
-  [1/1] start 'sleep 1'
-  [1/1] cancel 'sleep 1'
-  [0/1] finish 'sleep 1'
-  [1/1] start 'sleep 1'
-  [1/1] cancel 'sleep 1'
-  [0/1] finish 'sleep 1'
-  [1/1] start 'sleep 1'
-  [1/1] cancel 'sleep 1'
-  [0/1] finish 'sleep 1'
+  [1/1] start 'sleep 1' (killer)
+  [1/1] killing process (killer)
+  [0/1] finish 'sleep 1' (killer) 
+  [0/1] finish 'sleep 1' (killer) 
+  [0/1] finish 'sleep 1' (killer) 
+  [1/1] start 'sleep 1' (sleeper)
+  [1/1] cancel 'sleep 1' (sleeper)
+  [0/1] finish 'sleep 1' (sleeper) 
+  [1/1] start 'sleep 1' (sleeper)
+  [1/1] cancel 'sleep 1' (sleeper)
+  [0/1] finish 'sleep 1' (sleeper) 
+  [1/1] start 'sleep 1' (sleeper)
+  [1/1] cancel 'sleep 1' (sleeper)
+  [0/1] finish 'sleep 1' (sleeper) 
   All done!
diff --git a/tests/test-runqueue.c b/tests/test-runqueue.c
index 13ab864..fb99892 100644
--- a/tests/test-runqueue.c
+++ b/tests/test-runqueue.c
@@ -16,6 +16,7 @@
  * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
+#include <stdbool.h>
 #include <stdlib.h>
 #include <stdio.h>
 #include <unistd.h>
@@ -26,8 +27,10 @@
 static struct runqueue q;
 
 struct sleeper {
-	struct runqueue_process proc;
 	int val;
+	bool kill;
+	struct uloop_timeout t;
+	struct runqueue_process proc;
 };
 
 static void q_empty(struct runqueue *q)
@@ -36,13 +39,19 @@ static void q_empty(struct runqueue *q)
 	uloop_end();
 }
 
+static const char* sleeper_type(struct sleeper *s)
+{
+	return s->kill ? "killer" : "sleeper";
+}
+
 static void q_sleep_run(struct runqueue *q, struct runqueue_task *t)
 {
 	struct sleeper *s = container_of(t, struct sleeper, proc.task);
 	char str[32];
 	pid_t pid;
 
-	fprintf(stderr, "[%d/%d] start 'sleep %d'\n", q->running_tasks, q->max_running_tasks, s->val);
+	fprintf(stderr, "[%d/%d] start 'sleep %d' (%s)\n", q->running_tasks,
+			q->max_running_tasks, s->val, sleeper_type(s));
 
 	pid = fork();
 	if (pid < 0)
@@ -62,7 +71,8 @@ static void q_sleep_cancel(struct runqueue *q, struct runqueue_task *t, int type
 {
 	struct sleeper *s = container_of(t, struct sleeper, proc.task);
 
-	fprintf(stderr, "[%d/%d] cancel 'sleep %d'\n", q->running_tasks, q->max_running_tasks, s->val);
+	fprintf(stderr, "[%d/%d] cancel 'sleep %d' (%s)\n", q->running_tasks,
+			q->max_running_tasks, s->val, sleeper_type(s));
 	runqueue_process_cancel_cb(q, t, type);
 }
 
@@ -70,10 +80,40 @@ static void q_sleep_complete(struct runqueue *q, struct runqueue_task *p)
 {
 	struct sleeper *s = container_of(p, struct sleeper, proc.task);
 
-	fprintf(stderr, "[%d/%d] finish 'sleep %d'\n", q->running_tasks, q->max_running_tasks, s->val);
+	fprintf(stderr, "[%d/%d] finish 'sleep %d' (%s) \n", q->running_tasks,
+			q->max_running_tasks, s->val, sleeper_type(s));
 	free(s);
 }
 
+static void my_runqueue_process_kill_cb(struct runqueue *q, struct runqueue_task *p)
+{
+	struct sleeper *s = container_of(p, struct sleeper, proc.task);
+
+	fprintf(stderr, "[%d/%d] killing process (%s)\n", q->running_tasks,
+			q->max_running_tasks, sleeper_type(s));
+	runqueue_process_kill_cb(q, p);
+}
+
+static void timer_cb(struct uloop_timeout *t)
+{
+	struct sleeper *s = container_of(t, struct sleeper, t);
+	if (s->kill)
+		runqueue_task_kill(&s->proc.task);
+}
+
+static struct sleeper* create_sleeper(int val, const struct runqueue_task_type *type, bool kill)
+{
+	struct sleeper *s = calloc(1, sizeof(*s));
+	s->kill = kill;
+	s->t.cb = timer_cb;
+	s->proc.task.type = type;
+	s->proc.task.run_timeout = 500;
+	s->proc.task.complete = q_sleep_complete;
+	s->val = val;
+
+	return s;
+}
+
 static void add_sleeper(int val)
 {
 	static const struct runqueue_task_type sleeper_type = {
@@ -81,13 +121,19 @@ static void add_sleeper(int val)
 		.cancel = q_sleep_cancel,
 		.kill = runqueue_process_kill_cb,
 	};
-	struct sleeper *s;
 
-	s = calloc(1, sizeof(*s));
-	s->proc.task.type = &sleeper_type;
-	s->proc.task.run_timeout = 500;
-	s->proc.task.complete = q_sleep_complete;
-	s->val = val;
+	static const struct runqueue_task_type killer_type = {
+		.run = q_sleep_run,
+		.cancel = q_sleep_cancel,
+		.kill = my_runqueue_process_kill_cb,
+	};
+
+	struct sleeper *k = create_sleeper(val, &killer_type, true);
+	uloop_timeout_set(&k->t, 100);
+	uloop_timeout_add(&k->t);
+	runqueue_task_add(&q, &k->proc.task, false);
+
+	struct sleeper *s = create_sleeper(val, &sleeper_type, false);
 	runqueue_task_add(&q, &s->proc.task, false);
 }
 
@@ -105,6 +151,7 @@ int main(int argc, char **argv)
 	add_sleeper(1);
 	add_sleeper(1);
 	add_sleeper(1);
+
 	uloop_run();
 	uloop_done();