From 6966d841aff87530844805bdf7fe26fe5d5113d9 Mon Sep 17 00:00:00 2001 From: Christine Caulfield Date: Wed, 13 May 2020 14:27:16 +0100 Subject: [PATCH] log: Fix threading races It's possible that cs->filename or cs->format could be read in the 'fast' path while the 'slow' path is still constructing the object. So we need to lock arr_next_lock before copying them out for the caller. Also wthread_should_exit was unprotected. --- lib/log_dcs.c | 15 ++++++++++++--- lib/log_thread.c | 2 ++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/lib/log_dcs.c b/lib/log_dcs.c index 0249e6d08..e8e3b6f9b 100644 --- a/lib/log_dcs.c +++ b/lib/log_dcs.c @@ -105,21 +105,30 @@ qb_log_dcs_get(int32_t * newly_created, if (format == NULL) { safe_format = ""; } + /* - * try the fastest access first (no locking needed) + * try the fastest access first. */ rc = qb_array_index(lookup_arr, lineno, (void **)&csl_head); assert(rc == 0); + + /* + * Still need to lock here as we are reading from cs->* members + * and they might not be completely filled in yet by another thread + * that's running in _log_dcs_new_cs() below + */ + (void)qb_thread_lock(arr_next_lock); if (csl_head->cs && priority == csl_head->cs->priority && strcmp(safe_filename, csl_head->cs->filename) == 0 && strcmp(safe_format, csl_head->cs->format) == 0) { + (void)qb_thread_unlock(arr_next_lock); return csl_head->cs; } + /* - * so we will either have to create it or go through a list, so lock it. + * so we will either have to create it or go through a list */ - (void)qb_thread_lock(arr_next_lock); if (csl_head->cs == NULL) { csl_head->cs = _log_dcs_new_cs(safe_function, safe_filename, safe_format, priority, lineno, tags); diff --git a/lib/log_thread.c b/lib/log_thread.c index 27a24b6fe..40b8239c1 100644 --- a/lib/log_thread.c +++ b/lib/log_thread.c @@ -296,7 +296,9 @@ qb_log_thread_stop(void) free(rec); } } else { + (void)qb_thread_lock(logt_wthread_lock); wthread_should_exit = QB_TRUE; + (void)qb_thread_unlock(logt_wthread_lock); sem_post(&logt_print_finished); pthread_join(logt_thread_id, NULL); }