From 32d86f8b7a327cfc6904d51d5c3ea8f5c6115e44 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Fri, 5 May 2017 17:30:21 +0000 Subject: [PATCH] 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 --- lib/thread.c | 71 +++++++++++++++++++++++++++++++--------------------- lib/thread.h | 52 +++++++++++++++++++------------------- 2 files changed, 69 insertions(+), 54 deletions(-) diff --git a/lib/thread.c b/lib/thread.c index 37a71824d7..e4dbebe1c4 100644 --- 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 fc345768f9..3eaae8883b 100644 --- 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 -- 2.39.5