From: Ivan Maidanski Date: Thu, 23 Nov 2017 09:20:18 +0000 (+0300) Subject: Fix data race in GC_suspend/resume_thread X-Git-Tag: v8.0.0~495 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=ce09fd5;p=gc Fix data race in GC_suspend/resume_thread * include/private/pthread_support.h [GC_ENABLE_SUSPEND_THREAD && !GC_DARWIN_THREADS && !GC_OPENBSD_UTHREADS && !NACL] (GC_Thread_Rep.suspended_ext): New field. * include/private/pthread_support.h (SUSPENDED_EXT): Remove macro. * pthread_stop_world.c [GC_ENABLE_SUSPEND_THREAD && !GC_OPENBSD_UTHREADS && !NACL] (GC_suspend_handler_inner): Replace me->flags & SUSPENDED_EXT with AO_load(&me->suspended_ext). * pthread_stop_world.c [GC_ENABLE_SUSPEND_THREAD && !GC_OPENBSD_UTHREADS && !NACL] (suspend_self_inner): Replace me->flags & SUSPENDED_EXT with AO_load_acquire(&me->suspended_ext). * pthread_stop_world.c [GC_ENABLE_SUSPEND_THREAD && !GC_OPENBSD_UTHREADS && !NACL] (GC_suspend_thread): Replace t->flags & SUSPENDED_EXT with t->suspended_ext; replace t->flags|=SUSPENDED_EXT with AO_store_release(&t->suspended_ext, TRUE); add comment. * pthread_stop_world.c [GC_ENABLE_SUSPEND_THREAD && !GC_OPENBSD_UTHREADS && !NACL] (GC_resume_thread): Replace t->flags&=~SUSPENDED_EXT with AO_store(&t->suspended_ext, FALSE). * pthread_stop_world.c [GC_ENABLE_SUSPEND_THREAD && !GC_OPENBSD_UTHREADS && !NACL] (GC_is_thread_suspended, GC_suspend_all, GC_start_world): Replace t->flags & SUSPENDED_EXT with t->suspended_ext. * pthread_support.c (GC_register_my_thread): Remove assertion that uses SUSPENDED_EXT. --- diff --git a/include/private/pthread_support.h b/include/private/pthread_support.h index 4ab9c210..9699995b 100644 --- a/include/private/pthread_support.h +++ b/include/private/pthread_support.h @@ -64,6 +64,11 @@ typedef struct GC_Thread_Rep { /* and GC_suspend_handler_inner (which sets store_stop.stack_ptr). */ struct thread_stop_info stop_info; +# if defined(GC_ENABLE_SUSPEND_THREAD) && !defined(GC_DARWIN_THREADS) \ + && !defined(GC_OPENBSD_UTHREADS) && !defined(NACL) + volatile AO_t suspended_ext; /* Thread was suspended externally. */ +# endif + unsigned char flags; # define FINISHED 1 /* Thread has exited. */ # define DETACHED 2 /* Thread is treated as detached. */ @@ -74,7 +79,6 @@ typedef struct GC_Thread_Rep { /* it unregisters itself, since it */ /* may not return a GC pointer. */ # define MAIN_THREAD 4 /* True for the original thread only. */ -# define SUSPENDED_EXT 8 /* Thread was suspended externally. */ # define DISABLED_GC 0x10 /* Collections are disabled while the */ /* thread is exiting. */ diff --git a/pthread_stop_world.c b/pthread_stop_world.c index 7d53234d..0370cf31 100644 --- a/pthread_stop_world.c +++ b/pthread_stop_world.c @@ -291,7 +291,7 @@ STATIC void GC_suspend_handler_inner(ptr_t dummy GC_ATTR_UNUSED, me = GC_lookup_thread_async(self); # ifdef GC_ENABLE_SUSPEND_THREAD - if ((me -> flags & SUSPENDED_EXT) != 0) { + if (AO_load(&me->suspended_ext)) { # ifdef SPARC me -> stop_info.stack_ptr = GC_save_regs_in_stack(); # else @@ -440,7 +440,7 @@ STATIC void GC_restart_handler(int sig) static void *GC_CALLBACK suspend_self_inner(void *client_data) { GC_thread me = (GC_thread)client_data; - while ((me -> flags & SUSPENDED_EXT) != 0) { + while (AO_load_acquire(&me->suspended_ext)) { /* TODO: Use sigsuspend() instead. */ GC_brief_async_signal_safe_sleep(); } @@ -454,12 +454,14 @@ STATIC void GC_restart_handler(int sig) LOCK(); t = GC_lookup_thread((pthread_t)thread); - if (t == NULL || (t -> flags & SUSPENDED_EXT) != 0) { + if (t == NULL || t -> suspended_ext) { UNLOCK(); return; } - t -> flags |= SUSPENDED_EXT; + /* Set the flag making the change visible to the signal handler. */ + AO_store_release(&t->suspended_ext, (AO_t)TRUE); + if ((pthread_t)thread == pthread_self()) { UNLOCK(); /* It is safe as "t" cannot become invalid here (no race with */ @@ -514,21 +516,21 @@ STATIC void GC_restart_handler(int sig) LOCK(); t = GC_lookup_thread((pthread_t)thread); if (t != NULL) - t -> flags &= ~SUSPENDED_EXT; + AO_store(&t->suspended_ext, (AO_t)FALSE); UNLOCK(); } GC_API int GC_CALL GC_is_thread_suspended(GC_SUSPEND_THREAD_ID thread) { GC_thread t; - int flags = 0; + int is_suspended = 0; DCL_LOCK_STATE; LOCK(); t = GC_lookup_thread((pthread_t)thread); - if (t != NULL) - flags = t -> flags; + if (t != NULL && t -> suspended_ext) + is_suspended = (int)TRUE; UNLOCK(); - return (flags & SUSPENDED_EXT) != 0; + return is_suspended; } # endif /* GC_ENABLE_SUSPEND_THREAD */ @@ -663,9 +665,12 @@ STATIC int GC_suspend_all(void) for (i = 0; i < THREAD_TABLE_SZ; i++) { for (p = GC_threads[i]; p != 0; p = p -> next) { if (!THREAD_EQUAL(p -> id, self)) { - if ((p -> flags & (FINISHED | SUSPENDED_EXT)) != 0) continue; + if ((p -> flags & FINISHED) != 0) continue; if (p -> thread_blocked) /* Will wait */ continue; # ifndef GC_OPENBSD_UTHREADS +# ifdef GC_ENABLE_SUSPEND_THREAD + if (p -> suspended_ext) continue; +# endif if (p -> stop_info.last_stop_count == GC_stop_count) continue; n_live_threads++; # endif @@ -1044,9 +1049,12 @@ GC_INNER void GC_start_world(void) for (i = 0; i < THREAD_TABLE_SZ; i++) { for (p = GC_threads[i]; p != 0; p = p -> next) { if (!THREAD_EQUAL(p -> id, self)) { - if ((p -> flags & (FINISHED | SUSPENDED_EXT)) != 0) continue; + if ((p -> flags & FINISHED) != 0) continue; if (p -> thread_blocked) continue; # ifndef GC_OPENBSD_UTHREADS +# ifdef GC_ENABLE_SUSPEND_THREAD + if (p -> suspended_ext) continue; +# endif n_live_threads++; # endif # ifdef DEBUG_THREADS diff --git a/pthread_support.c b/pthread_support.c index c70ab7da..2d68293e 100644 --- a/pthread_support.c +++ b/pthread_support.c @@ -1688,7 +1688,6 @@ GC_API int GC_CALL GC_register_my_thread(const struct GC_stack_base *sb) } else if ((me -> flags & FINISHED) != 0) { /* This code is executed when a thread is registered from the */ /* client thread key destructor. */ - GC_ASSERT((me -> flags & SUSPENDED_EXT) == 0); GC_record_stack_base(me, sb); me -> flags &= ~FINISHED; /* but not DETACHED */ # ifdef GC_EXPLICIT_SIGNALS_UNBLOCK