Fix UAF with timer setting and freeing

Thread A creates a timer and sets it Thread B starts to handle the timer and begins to nanosleep Thread A cancels the timer and immediately frees it Thread B wakes up and still has a pointer to the freed timer
Jsn2win · Jun 27, 2020 · 930e0cc · 930e0cc
1 parent 65aca71
commit 930e0cc
Show file tree

Hide file tree

Showing 2 changed files with 22 additions and 23 deletions.
diff --git a/util/timer.c b/util/timer.c
@@ -5,27 +5,28 @@
 #include "misc.h"
 
 struct timer *timer_new(clockid_t clockid, timer_callback_t callback, void *data) {
+//    assert(clockid == CLOCK_MONOTONIC || clockid == CLOCK_REALTIME);
     struct timer *timer = malloc(sizeof(struct timer));
     timer->clockid = clockid;
     timer->callback = callback;
     timer->data = data;
-    timer->running = false;
-    timer->dead = false;
+    timer->active = false;
+    timer->thread_running = false;
     lock_init(&timer->lock);
+    timer->dead = false;
     return timer;
 }
 
 void timer_free(struct timer *timer) {
     lock(&timer->lock);
-    if (timer->running) {
-        timer->running = false;
+    timer->active = false;
+    if (timer->thread_running) {
         timer->dead = true;
         pthread_kill(timer->thread, SIGUSR1);
         unlock(&timer->lock);
     } else {
         unlock(&timer->lock);
-        if (!timer->dead)
-            free(timer);
+        free(timer);
     }
 }
 
@@ -34,22 +35,22 @@ static void *timer_thread(void *param) {
     lock(&timer->lock);
     while (true) {
         struct timespec remaining = timespec_subtract(timer->end, timespec_now(timer->clockid));
-        while (timer->running && timespec_positive(remaining)) {
+        while (timer->active && timespec_positive(remaining)) {
             unlock(&timer->lock);
             nanosleep(&remaining, NULL);
             lock(&timer->lock);
             remaining = timespec_subtract(timer->end, timespec_now(timer->clockid));
         }
-        if (timer->running)
+        if (timer->active)
             timer->callback(timer->data);
-        if (timer->running && timespec_positive(timer->interval)) {
+        if (timer->active && timespec_positive(timer->interval)) {
             timer->start = timer->end;
             timer->end = timespec_add(timer->start, timer->interval);
         } else {
             break;
         }
     }
-    timer->running = false;
+    timer->thread_running = false;
     if (timer->dead)
         free(timer);
     else
@@ -68,17 +69,13 @@ int timer_set(struct timer *timer, struct timer_spec spec, struct timer_spec *ol
     timer->start = now;
     timer->end = timespec_add(timer->start, spec.value);
     timer->interval = spec.interval;
-    if (!timespec_is_zero(spec.value)) {
-        if (!timer->running) {
-            timer->running = true;
-            pthread_create(&timer->thread, NULL, timer_thread, timer);
-            pthread_detach(timer->thread);
-        }
-    } else {
-        if (timer->running) {
-            timer->running = false;
-            pthread_kill(timer->thread, SIGUSR1);
-        }
+    timer->active = !timespec_is_zero(spec.value);
+    if (timer->thread_running) {
+        pthread_kill(timer->thread, SIGUSR1);
+    } else if (timer->active) {
+        timer->thread_running = true;
+        pthread_create(&timer->thread, NULL, timer_thread, timer);
+        pthread_detach(timer->thread);
     }
     unlock(&timer->lock);
     return 0;

diff --git a/util/timer.h b/util/timer.h
@@ -56,12 +56,14 @@ struct timer {
     struct timespec end;
     struct timespec interval;
 
-    bool running;
+    bool active;
+    bool thread_running;
     pthread_t thread;
     timer_callback_t callback;
     void *data;
     lock_t lock;
-    bool dead;
+
+    bool dead; // set by timer_free, the thread will free the timer if this is set when it finishes
 };
 
 struct timer *timer_new(clockid_t clockid, timer_callback_t callback, void *data);