From d6db3f005ecfff2a54b0fbd228933ae5eb12be50 Mon Sep 17 00:00:00 2001 From: Ivan Maidanski Date: Thu, 27 Sep 2018 01:38:02 +0300 Subject: [PATCH] Prevent a deadlock in suspend_thread and after process forking (fix of commit 0c0e4cd) Issue #235 (bdwgc). * pthread_stop_world.c [GC_ENABLE_SUSPEND_THREAD] (GC_suspend_thread): Remove unneeded comment for AO_store_release() call; invoke GC_acquire_dirty_lock() and GC_release_dirty_lock() only if GC_manual_vdb; add comment for GC_acquire_dirty_lock() call. * pthread_stop_world.c [NACL] (GC_suspend_all): If GC_manual_vdb then call GC_acquire_dirty_lock() and GC_release_dirty_lock() around the code which ensures parking of threads. * pthread_support.c [CAN_HANDLE_FORK] (fork_prepare_proc): Call GC_acquire_dirty_lock(). * pthread_support.c [CAN_HANDLE_FORK] (fork_parent_proc, fork_child_proc): Call GC_release_dirty_lock(). --- pthread_stop_world.c | 19 ++++++++++++++----- pthread_support.c | 5 ++++- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/pthread_stop_world.c b/pthread_stop_world.c index 92605974..a94d4663 100644 --- a/pthread_stop_world.c +++ b/pthread_stop_world.c @@ -545,9 +545,6 @@ STATIC void GC_restart_handler(int sig) } /* Set the flag making the change visible to the signal handler. */ - /* This also removes the protection for t object, preventing */ - /* write faults in GC_store_stack_ptr (thus double-locking should */ - /* not occur in async_set_pht_entry_from_index). */ AO_store_release(&t->suspended_ext, TRUE); if (THREAD_EQUAL((pthread_t)thread, pthread_self())) { @@ -577,8 +574,15 @@ STATIC void GC_restart_handler(int sig) GC_wait_for_reclaim(); # endif + if (GC_manual_vdb) { + /* See the relevant comment in GC_stop_world. */ + GC_acquire_dirty_lock(); + } + /* Else do not acquire the lock as the write fault handler might */ + /* be trying to acquire this lock too, and the suspend handler */ + /* execution is deferred until the write fault handler completes. */ + /* TODO: Support GC_retry_signals (not needed for TSan) */ - GC_acquire_dirty_lock(); switch (RAISE_SIGNAL(t, GC_sig_suspend)) { /* ESRCH cannot happen as terminated threads are handled above. */ case 0: @@ -594,7 +598,8 @@ STATIC void GC_restart_handler(int sig) if (errno != EINTR) ABORT("sem_wait for handler failed (suspend_self)"); } - GC_release_dirty_lock(); + if (GC_manual_vdb) + GC_release_dirty_lock(); RESTORE_CANCEL(cancel_state); UNLOCK(); } @@ -821,6 +826,8 @@ STATIC int GC_suspend_all(void) GC_nacl_thread_parker = pthread_self(); GC_nacl_park_threads_now = 1; + if (GC_manual_vdb) + GC_acquire_dirty_lock(); while (1) { int num_threads_parked = 0; struct timespec ts; @@ -856,6 +863,8 @@ STATIC int GC_suspend_all(void) num_sleeps = 0; } } + if (GC_manual_vdb) + GC_release_dirty_lock(); # endif /* NACL */ return n_live_threads; } diff --git a/pthread_support.c b/pthread_support.c index fb8db26b..22b369d6 100644 --- a/pthread_support.c +++ b/pthread_support.c @@ -1087,6 +1087,7 @@ static void fork_prepare_proc(void) if (GC_parallel) GC_acquire_mark_lock(); # endif + GC_acquire_dirty_lock(); } /* Called in parent after a fork() (even if the latter failed). */ @@ -1095,6 +1096,7 @@ static void fork_prepare_proc(void) #endif static void fork_parent_proc(void) { + GC_release_dirty_lock(); # if defined(PARALLEL_MARK) if (GC_parallel) GC_release_mark_lock(); @@ -1109,11 +1111,12 @@ static void fork_parent_proc(void) #endif static void fork_child_proc(void) { - /* Clean up the thread table, so that just our thread is left. */ + GC_release_dirty_lock(); # if defined(PARALLEL_MARK) if (GC_parallel) GC_release_mark_lock(); # endif + /* Clean up the thread table, so that just our thread is left. */ GC_remove_all_threads_but_me(); # ifdef PARALLEL_MARK /* Turn off parallel marking in the child, since we are probably */ -- 2.50.1