From 07542cf69360200762ec4131d3bf98fc63632954 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pokorn=C3=BD?= Date: Thu, 20 Oct 2016 15:30:44 +0200 Subject: [PATCH 1/2] Med: log_thread: logt_wthread_lock is vital for logging thread This fixes issue with would-fail-if-applied-to-thread-right-away qb_log_thread_priority_set invocation when logging thread doesn't exist yet, which will arrange for calling itself at the time of thread's birth that is the moment it will actually fail. In this + lock-could-not-have-been-initialized corner cases, the already running thread would proceed as allowed by error condition handling in the main thread, trying to dereference uninitialized (or outdated) pointer to the lock at hand, resulting in segfault. Also include the test that would have been caught that (we use the fact that it doesn't matter whether setting of the scheduler parameters fails due to bad input or just because of lack of privileges as it's the failure at the right moment that is of our interest). See also: https://github.com/ClusterLabs/libqb/issues/229 --- lib/log_thread.c | 17 +++++++++++++---- tests/check_log.c | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 4 deletions(-) diff --git a/lib/log_thread.c b/lib/log_thread.c index ee6d338..613298a 100644 --- a/lib/log_thread.c +++ b/lib/log_thread.c @@ -146,6 +146,7 @@ int32_t qb_log_thread_start(void) { int res; + qb_thread_lock_t *wthread_lock; if (wthread_active) { return 0; @@ -154,10 +155,16 @@ qb_log_thread_start(void) wthread_active = QB_TRUE; sem_init(&logt_thread_start, 0, 0); sem_init(&logt_print_finished, 0, 0); + errno = 0; + logt_wthread_lock = qb_thread_lock_create(QB_THREAD_LOCK_SHORT); + if (logt_wthread_lock == NULL) { + return errno ? -errno : -1; + } res = pthread_create(&logt_thread_id, NULL, qb_logt_worker_thread, NULL); if (res != 0) { wthread_active = QB_FALSE; + (void)qb_thread_lock_destroy(logt_wthread_lock); return -res; } sem_wait(&logt_thread_start); @@ -174,10 +181,6 @@ qb_log_thread_start(void) } logt_sched_param_queued = QB_FALSE; } - logt_wthread_lock = qb_thread_lock_create(QB_THREAD_LOCK_SHORT); - if (logt_wthread_lock == NULL) { - goto cleanup_pthread; - } return 0; @@ -185,6 +188,12 @@ cleanup_pthread: wthread_should_exit = QB_TRUE; sem_post(&logt_print_finished); pthread_join(logt_thread_id, NULL); + + wthread_active = QB_FALSE; + wthread_lock = logt_wthread_lock; + logt_wthread_lock = NULL; + (void)qb_thread_lock_destroy(wthread_lock); + sem_destroy(&logt_print_finished); sem_destroy(&logt_thread_start); diff --git a/tests/check_log.c b/tests/check_log.c index 7d6a88e..8a04695 100644 --- a/tests/check_log.c +++ b/tests/check_log.c @@ -726,6 +726,42 @@ START_TEST(test_threaded_logging) } END_TEST +#ifdef HAVE_PTHREAD_SETSCHEDPARAM +START_TEST(test_threaded_logging_bad_sched_params) +{ + int32_t t; + int32_t rc; + + qb_log_init("test", LOG_USER, LOG_EMERG); + qb_log_ctl(QB_LOG_SYSLOG, QB_LOG_CONF_ENABLED, QB_FALSE); + + t = qb_log_custom_open(_test_logger, NULL, NULL, NULL); + rc = qb_log_filter_ctl(t, QB_LOG_FILTER_ADD, + QB_LOG_FILTER_FILE, "*", LOG_INFO); + ck_assert_int_eq(rc, 0); + qb_log_format_set(t, "%b"); + rc = qb_log_ctl(t, QB_LOG_CONF_ENABLED, QB_TRUE); + ck_assert_int_eq(rc, 0); + rc = qb_log_ctl(t, QB_LOG_CONF_THREADED, QB_TRUE); + ck_assert_int_eq(rc, 0); + +#if defined(SCHED_RR) +#define QB_SCHED SCHED_RR +#elif defined(SCHED_FIFO) +#define QB_SCHED SCHED_FIFO +#else +#define QB_SCHED (-1) +#endif + rc = qb_log_thread_priority_set(QB_SCHED, -1); + ck_assert_int_eq(rc, 0); + + rc = qb_log_thread_start(); + ck_assert_int_ne(rc, 0); + qb_log_fini(); +} +END_TEST +#endif + START_TEST(test_extended_information) { int32_t t; @@ -817,6 +853,9 @@ log_suite(void) add_tcase(s, tc, test_log_long_msg); add_tcase(s, tc, test_log_filter_fn); add_tcase(s, tc, test_threaded_logging); +#ifdef HAVE_PTHREAD_SETSCHEDPARAM + add_tcase(s, tc, test_threaded_logging_bad_sched_params); +#endif add_tcase(s, tc, test_extended_information); #ifdef HAVE_SYSLOG_TESTS From ca710b2505befa6d1b680261263b44ab0610b86e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pokorn=C3=BD?= Date: Thu, 20 Oct 2016 17:11:10 +0200 Subject: [PATCH 2/2] Refactor: log_thread: fix and diminish inferior comments --- lib/log_thread.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/lib/log_thread.c b/lib/log_thread.c index 613298a..79a22e8 100644 --- a/lib/log_thread.c +++ b/lib/log_thread.c @@ -62,9 +62,7 @@ qb_logt_worker_thread(void *data) int dropped = 0; int res; - /* - * Signal wthread_create that the initialization process may continue - */ + /* Signal qb_log_thread_start that the initialization may continue */ sem_post(&logt_thread_start); for (;;) { retry_sem_wait: @@ -72,9 +70,7 @@ retry_sem_wait: if (res == -1 && errno == EINTR) { goto retry_sem_wait; } else if (res == -1) { - /* - * This case shouldn't happen - */ + /* This case shouldn't happen */ pthread_exit(NULL); }