lib: safely check & set thread pointers

When scheduling a thread, the scheduling function returns a pointer to
the struct thread that was placed on one of the scheduling queues in the
associated thread master. This pointer is used to check whether or not
the thread is scheduled, and is passed to thread_cancel() should the
daemon need to cancel that particular task.

The thread_fetch() function is called to retrieve the next thread to
execute. However, when it returns, the aforementioned pointer is not
updated. As a result, in order for the above use cases to work, every
thread handler function must set the associated pointer to NULL. This is
bug prone, and moreover, not thread safe.

This patch changes the thread scheduling functions to return void. If
the caller needs a reference to the scheduled thread, it must pass in a
pointer to store the pointer to the thread struct in. Subsequent calls
to thread_cancel(), thread_cancel_event() or thread_fetch() will result
in that pointer being nulled before return. These operations occur
within the thread_master critical sections.

Overall this should avoid bugs introduced by thread handler funcs
forgetting to null the associated pointer, double-scheduling caused by
overwriting pointers to currently scheduled threads without performing a
nullity check, and the introduction of true kernel threads causing race
conditions within the userspace threading world.

Also removes the return value for thread_execute since it always returns
null...

Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>

diff --git a/lib/thread.c b/lib/thread.c
index 37a71824d7499b04adc1348730b146d90ce8f9b4..e4dbebe1c401286407f34fa70208d943b71b8654 100644 (file)
--- a/lib/thread.c
+++ b/lib/thread.c
@@ -454,6 +454,7 @@ thread_add_unuse (struct thread_master *m, struct thread *thread)
   assert (m != NULL && thread != NULL);
   assert (thread->next == NULL);
   assert (thread->prev == NULL);
+  thread->ref = NULL;
 
   thread->type = THREAD_UNUSED;
   thread->hist->total_active--;
@@ -775,7 +776,7 @@ fd_clear_read_write (struct thread *thread)
 }
 
 /* Add new read thread. */
-struct thread *
+void
 funcname_thread_add_read_write (int dir, struct thread_master *m,
         int (*func) (struct thread *), void *arg, int fd, struct thread **t_ptr,
         debugargdef)
@@ -787,7 +788,7 @@ funcname_thread_add_read_write (int dir, struct thread_master *m,
     if (t_ptr && *t_ptr) // thread is already scheduled; don't reschedule
       {
         pthread_mutex_unlock (&m->mtx);
-        return NULL;
+        return;
       }
 
 #if defined (HAVE_POLL_CALL)
@@ -831,14 +832,15 @@ funcname_thread_add_read_write (int dir, struct thread_master *m,
       }
 
     if (t_ptr)
-      *t_ptr = thread;
+      {
+        *t_ptr = thread;
+        thread->ref = t_ptr;
+      }
   }
   pthread_mutex_unlock (&m->mtx);
-
-  return thread;
 }
 
-static struct thread *
+static void
 funcname_thread_add_timer_timeval (struct thread_master *m,
          int (*func) (struct thread *), int type, void *arg,
          struct timeval *time_relative, struct thread **t_ptr, debugargdef)
@@ -856,7 +858,7 @@ funcname_thread_add_timer_timeval (struct thread_master *m,
     if (t_ptr && *t_ptr) // thread is already scheduled; don't reschedule
       {
         pthread_mutex_unlock (&m->mtx);
-        return NULL;
+        return;
       }
 
     queue = ((type == THREAD_TIMER) ? m->timer : m->background);
@@ -871,16 +873,17 @@ funcname_thread_add_timer_timeval (struct thread_master *m,
     pthread_mutex_unlock (&thread->mtx);
 
     if (t_ptr)
-      *t_ptr = thread;
+      {
+        *t_ptr = thread;
+        thread->ref = t_ptr;
+      }
   }
   pthread_mutex_unlock (&m->mtx);
-
-  return thread;
 }
 
 
 /* Add timer event thread. */
-struct thread *
+void
 funcname_thread_add_timer (struct thread_master *m,
         int (*func) (struct thread *), void *arg, long timer,
         struct thread **t_ptr, debugargdef)
@@ -897,7 +900,7 @@ funcname_thread_add_timer (struct thread_master *m,
 }
 
 /* Add timer event thread with "millisecond" resolution */
-struct thread *
+void
 funcname_thread_add_timer_msec (struct thread_master *m,
         int (*func) (struct thread *), void *arg, long timer,
         struct thread **t_ptr, debugargdef)
@@ -909,22 +912,22 @@ funcname_thread_add_timer_msec (struct thread_master *m,
   trel.tv_sec = timer / 1000;
   trel.tv_usec = 1000*(timer % 1000);
 
-  return funcname_thread_add_timer_timeval (m, func, THREAD_TIMER, arg, &trel,
-                                            t_ptr, debugargpass);
+  funcname_thread_add_timer_timeval (m, func, THREAD_TIMER, arg, &trel,
+                                     t_ptr, debugargpass);
 }
 
 /* Add timer event thread with "millisecond" resolution */
-struct thread *
+void
 funcname_thread_add_timer_tv (struct thread_master *m,
         int (*func) (struct thread *), void *arg, struct timeval *tv,
         struct thread **t_ptr, debugargdef)
 {
-  return funcname_thread_add_timer_timeval (m, func, THREAD_TIMER, arg, tv,
-                                            t_ptr, debugargpass);
+  funcname_thread_add_timer_timeval (m, func, THREAD_TIMER, arg, tv, t_ptr,
+                                     debugargpass);
 }
 
 /* Add a background thread, with an optional millisec delay */
-struct thread *
+void
 funcname_thread_add_background (struct thread_master *m,
         int (*func) (struct thread *), void *arg, long delay,
         struct thread **t_ptr, debugargdef)
@@ -944,12 +947,12 @@ funcname_thread_add_background (struct thread_master *m,
       trel.tv_usec = 0;
     }
 
-  return funcname_thread_add_timer_timeval (m, func, THREAD_BACKGROUND, arg,
-                                            &trel, t_ptr, debugargpass);
+  funcname_thread_add_timer_timeval (m, func, THREAD_BACKGROUND, arg, &trel,
+                                     t_ptr, debugargpass);
 }
 
 /* Add simple event thread. */
-struct thread *
+void
 funcname_thread_add_event (struct thread_master *m,
         int (*func) (struct thread *), void *arg, int val,
         struct thread **t_ptr, debugargdef)
@@ -963,7 +966,7 @@ funcname_thread_add_event (struct thread_master *m,
     if (t_ptr && *t_ptr) // thread is already scheduled; don't reschedule
       {
         pthread_mutex_unlock (&m->mtx);
-        return NULL;
+        return;
       }
 
     thread = thread_get (m, THREAD_EVENT, func, arg, debugargpass);
@@ -975,11 +978,12 @@ funcname_thread_add_event (struct thread_master *m,
     pthread_mutex_unlock (&thread->mtx);
 
     if (t_ptr)
-      *t_ptr = thread;
+      {
+        *t_ptr = thread;
+        thread->ref = t_ptr;
+      }
   }
   pthread_mutex_unlock (&m->mtx);
-
-  return thread;
 }
 
 static void
@@ -1077,6 +1081,9 @@ thread_cancel (struct thread *thread)
       assert(!"Thread should be either in queue or list or array!");
     }
 
+  if (thread->ref)
+    *thread->ref = NULL;
+
   thread_add_unuse (thread->master, thread);
 
 done:
@@ -1106,6 +1113,8 @@ thread_cancel_event (struct thread_master *m, void *arg)
             {
               ret++;
               thread_list_delete (&m->event, t);
+              if (t->ref)
+                *t->ref = NULL;
               thread_add_unuse (m, t);
             }
         }
@@ -1125,6 +1134,8 @@ thread_cancel_event (struct thread_master *m, void *arg)
             {
               ret++;
               thread_list_delete (&m->ready, t);
+              if (t->ref)
+                *t->ref = NULL;
               thread_add_unuse (m, t);
             }
         }
@@ -1306,6 +1317,8 @@ thread_fetch (struct thread_master *m, struct thread *fetch)
       if ((thread = thread_trim_head (&m->ready)) != NULL)
         {
           fetch = thread_run (m, thread, fetch);
+          if (fetch->ref)
+            *fetch->ref = NULL;
           pthread_mutex_unlock (&m->mtx);
           return fetch;
         }
@@ -1375,6 +1388,8 @@ thread_fetch (struct thread_master *m, struct thread *fetch)
       if ((thread = thread_trim_head (&m->ready)) != NULL)
         {
           fetch = thread_run (m, thread, fetch);
+          if (fetch->ref)
+            *fetch->ref = NULL;
           pthread_mutex_unlock (&m->mtx);
           return fetch;
         }
@@ -1386,6 +1401,8 @@ thread_fetch (struct thread_master *m, struct thread *fetch)
       if ((thread = thread_trim_head (&m->ready)) != NULL)
         {
           fetch = thread_run (m, thread, fetch);
+          if (fetch->ref)
+            *fetch->ref = NULL;
           pthread_mutex_unlock (&m->mtx);
           return fetch;
         }
@@ -1493,7 +1510,7 @@ thread_call (struct thread *thread)
 }
 
 /* Execute thread */
-struct thread *
+void
 funcname_thread_execute (struct thread_master *m,
                 int (*func)(struct thread *), 
                 void *arg,
@@ -1525,6 +1542,4 @@ funcname_thread_execute (struct thread_master *m,
   dummy.schedfrom_line = fromln;
 
   thread_call (&dummy);
-
-  return NULL;
 }

diff --git a/lib/thread.h b/lib/thread.h
index fc345768f90a150d0c62d6b6e9ebf56991a859e7..3eaae8883b9d0ea57a202e99aa31e7cc721a77c1 100644 (file)
--- a/lib/thread.h
+++ b/lib/thread.h
@@ -96,26 +96,27 @@ typedef unsigned char thread_type;
 /* Thread itself. */
 struct thread
 {
-  thread_type type;            /* thread type */
-  thread_type add_type;                /* thread type */
-  struct thread *next;         /* next pointer of the thread */   
-  struct thread *prev;         /* previous pointer of the thread */
-  struct thread_master *master;        /* pointer to the struct thread_master. */
-  int (*func) (struct thread *); /* event function */
-  void *arg;                   /* event argument */
+  thread_type type;                 /* thread type */
+  thread_type add_type;             /* thread type */
+  struct thread *next;              /* next pointer of the thread */
+  struct thread *prev;              /* previous pointer of the thread */
+  struct thread **ref;              /* external reference (if given) */
+  struct thread_master *master;     /* pointer to the struct thread_master */
+  int (*func) (struct thread *);    /* event function */
+  void *arg;                        /* event argument */
   union {
-    int val;                   /* second argument of the event. */
-    int fd;                    /* file descriptor in case of read/write. */
-    struct timeval sands;      /* rest of time sands value. */
+    int val;                        /* second argument of the event. */
+    int fd;                         /* file descriptor in case of r/w */
+    struct timeval sands;           /* rest of time sands value. */
   } u;
-  int index;                   /* used for timers to store position in queue */
+  int index;                        /* queue position for timers */
   struct timeval real;
-  struct cpu_thread_history *hist; /* cache pointer to cpu_history */
-  unsigned long yield; /* yield time in us */
-  const char *funcname;
-  const char *schedfrom;
-  int schedfrom_line;
-  pthread_mutex_t mtx;
+  struct cpu_thread_history *hist;  /* cache pointer to cpu_history */
+  unsigned long yield;              /* yield time in microseconds */
+  const char *funcname;             /* name of thread function */
+  const char *schedfrom;            /* source file thread was scheduled from */
+  int schedfrom_line;               /* line number of source file */
+  pthread_mutex_t mtx;              /* mutex for thread.c functions */
 };
 
 struct cpu_thread_history 
@@ -184,26 +185,25 @@ extern struct thread_master *thread_master_create (void);
 extern void thread_master_free (struct thread_master *);
 extern void thread_master_free_unused(struct thread_master *);
 
-extern struct thread *funcname_thread_add_read_write (int dir, struct thread_master *,
+extern void funcname_thread_add_read_write (int dir, struct thread_master *,
     int (*)(struct thread *), void *, int, struct thread **, debugargdef);
 
-extern struct thread *funcname_thread_add_timer (struct thread_master *,
+extern void funcname_thread_add_timer (struct thread_master *,
     int (*)(struct thread *), void *, long, struct thread **, debugargdef);
 
-extern struct thread *funcname_thread_add_timer_msec (struct thread_master *,
+extern void funcname_thread_add_timer_msec (struct thread_master *,
     int (*)(struct thread *), void *, long, struct thread **, debugargdef);
 
-extern struct thread *funcname_thread_add_timer_tv (struct thread_master *,
+extern void funcname_thread_add_timer_tv (struct thread_master *,
     int (*)(struct thread *), void *, struct timeval *, struct thread **, debugargdef);
 
-extern struct thread *funcname_thread_add_event (struct thread_master *,
+extern void funcname_thread_add_event (struct thread_master *,
     int (*)(struct thread *), void *, int, struct thread **, debugargdef);
 
-extern struct thread *funcname_thread_add_background (struct thread_master *,
-    int (*func)(struct thread *), void *arg, long milliseconds_to_delay,
-    struct thread **, debugargdef);
+extern void funcname_thread_add_background (struct thread_master *,
+    int (*)(struct thread *), void *, long, struct thread **, debugargdef);
 
-extern struct thread *funcname_thread_execute (struct thread_master *,
+extern void funcname_thread_execute (struct thread_master *,
     int (*)(struct thread *), void *, int, debugargdef);
 #undef debugargdef