From 9b7ef4d25842dc6a79eaa76200511cce5a3610f5 Mon Sep 17 00:00:00 2001 From: Ivan Maidanski Date: Tue, 22 Aug 2017 11:23:27 +0300 Subject: [PATCH] Fix deadlock in GC_help_marker caused by use of mark_cv of parent process The marker threads of the parent process are blocked on mark_cv at fork. So pthread_cond_wait() malfunction (hang) is possible in the child process without mark_cv state cleanup (reinitialization). * pthread_support.c [PARALLEL_MARK] (mark_cv): Move static variable definition upper to be before GC_start_mark_threads_inner). * win32_threads.c [GC_PTHREADS_PARAMARK] (mark_cv): Likewise. * pthread_support.c [PARALLEL_MARK && CAN_HANDLE_FORK] (mark_cv): Do not initialize statically; add comment. * win32_threads.c [GC_PTHREADS_PARAMARK && CAN_HANDLE_FORK] (mark_cv): Likewise. * pthread_support.c [PARALLEL_MARK && CAN_HANDLE_FORK] (GC_start_mark_threads_inner): Initialize mark_cv to PTHREAD_COND_INITIALIZER (unless available_markers_m1 <= 0 or GC_parallel); add comment. * win32_threads.c [GC_PTHREADS_PARAMARK && CAN_HANDLE_FORK] (GC_start_mark_threads_inner): Likewise. * pthread_support.c [PARALLEL_MARK] (GC_wait_marker, GC_notify_all_marker): Add assertion that GC_parallel is true (so mark_cv is initialized). * win32_threads.c [GC_PTHREADS_PARAMARK] (GC_wait_marker, GC_notify_all_marker): Likewise. --- pthread_support.c | 19 +++++++++++++++++-- win32_threads.c | 17 +++++++++++++++-- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/pthread_support.c b/pthread_support.c index f996c755..14b184cb 100644 --- a/pthread_support.c +++ b/pthread_support.c @@ -385,8 +385,11 @@ STATIC pthread_t GC_mark_threads[MAX_MARKERS]; #ifdef CAN_HANDLE_FORK static int available_markers_m1 = 0; + static pthread_cond_t mark_cv; + /* initialized by GC_start_mark_threads_inner */ #else # define available_markers_m1 GC_markers_m1 + static pthread_cond_t mark_cv = PTHREAD_COND_INITIALIZER; #endif GC_INNER void GC_start_mark_threads_inner(void) @@ -402,6 +405,18 @@ GC_INNER void GC_start_mark_threads_inner(void) /* Skip if parallel markers disabled or already started. */ # ifdef CAN_HANDLE_FORK if (GC_parallel) return; + + /* Initialize mark_cv (for the first time), or cleanup its value */ + /* after forking in the child process. All the marker threads in */ + /* the parent process were blocked on this variable at fork, so */ + /* pthread_cond_wait() malfunction (hang) is possible in the */ + /* child process without such a cleanup. */ + /* TODO: This is not portable, it is better to shortly unblock */ + /* all marker threads in the parent process at fork. */ + { + pthread_cond_t mark_cv_local = PTHREAD_COND_INITIALIZER; + BCOPY(&mark_cv_local, &mark_cv, sizeof(mark_cv)); + } # endif GC_ASSERT(GC_fl_builder_count == 0); @@ -2153,11 +2168,10 @@ GC_INNER void GC_notify_all_builder(void) } } -static pthread_cond_t mark_cv = PTHREAD_COND_INITIALIZER; - GC_INNER void GC_wait_marker(void) { ASSERT_CANCEL_DISABLED(); + GC_ASSERT(GC_parallel); UNSET_MARK_LOCK_HOLDER; if (pthread_cond_wait(&mark_cv, &mark_mutex) != 0) { ABORT("pthread_cond_wait failed"); @@ -2168,6 +2182,7 @@ GC_INNER void GC_wait_marker(void) GC_INNER void GC_notify_all_marker(void) { + GC_ASSERT(GC_parallel); if (pthread_cond_broadcast(&mark_cv) != 0) { ABORT("pthread_cond_broadcast failed"); } diff --git a/win32_threads.c b/win32_threads.c index a17e018c..b34429f4 100644 --- a/win32_threads.c +++ b/win32_threads.c @@ -1780,6 +1780,13 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, /* Id not guaranteed to be unique. */ # endif +# ifdef CAN_HANDLE_FORK + static pthread_cond_t mark_cv; + /* initialized by GC_start_mark_threads_inner */ +# else + static pthread_cond_t mark_cv = PTHREAD_COND_INITIALIZER; +# endif + /* GC_start_mark_threads is the same as in pthread_support.c except */ /* for thread stack that is assumed to be large enough. */ @@ -1797,6 +1804,12 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, /* Skip if parallel markers disabled or already started. */ # ifdef CAN_HANDLE_FORK if (GC_parallel) return; + + /* Reset mark_cv state after forking (as in pthread_support.c). */ + { + pthread_cond_t mark_cv_local = PTHREAD_COND_INITIALIZER; + BCOPY(&mark_cv_local, &mark_cv, sizeof(mark_cv)); + } # endif GC_ASSERT(GC_fl_builder_count == 0); @@ -1925,10 +1938,9 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, } } - static pthread_cond_t mark_cv = PTHREAD_COND_INITIALIZER; - GC_INNER void GC_wait_marker(void) { + GC_ASSERT(GC_parallel); UNSET_MARK_LOCK_HOLDER; if (pthread_cond_wait(&mark_cv, &mark_mutex) != 0) { ABORT("pthread_cond_wait failed"); @@ -1939,6 +1951,7 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, GC_INNER void GC_notify_all_marker(void) { + GC_ASSERT(GC_parallel); if (pthread_cond_broadcast(&mark_cv) != 0) { ABORT("pthread_cond_broadcast failed"); } -- 2.40.0