Fix potential race if start_mark_threads called from threads in child

There should be no race in setting GC_markers_m1 value even in case of
a client calls GC_start_mark_threads() from multiple threads in a child
process.

* include/gc/gc.h (GC_start_mark_threads): Add comment that the
allocation lock is acquired internally.
* mark.c [PARALLEL_MARK] (GC_wait_for_markers_init): Change assertion
from I_DONT_HOLD_LOCK to I_HOLD_LOCK.
* pthread_support.c [PARALLEL_MARK] (GC_start_mark_threads_inner):
Likewise.
* win32_threads.c [PARALLEL_MARK] (GC_start_mark_threads_inner):
Likewise.
* mark.c [PARALLEL_MARK] (GC_wait_for_markers_init): Remove LOCK/UNLOCK
around GC_add_to_our_memory() call.
* misc.c [PARALLEL_MARK && GC_ASSERTIONS && GC_ALWAYS_MULTITHREADED]
(GC_init): Remove UNLOCK/LOCK around GC_start_mark_threads_inner call.
* misc.c [THREADS && PARALLEL_MARK && CAN_HANDLE_FORK
&& !THREAD_SANITIZER] (GC_start_mark_threads): Wrap
GC_start_mark_threads_inner() call into LOCK/UNLOCK.

5 files changed, 8 insertions, 13 deletions

diff --git a/include/gc/gc.h b/include/gc/gc.h
index cea646e5..29e967a1 100644
--- a/include/gc/gc.h
+++ b/include/gc/gc.h
@@ -1540,6 +1540,7 @@ GC_API void * GC_CALL GC_call_with_stack_base(GC_stack_base_func /* fn */,
 
   /* Restart marker threads after POSIX fork in child.  Meaningless in  */
   /* other situations.  Should not be called if fork followed by exec.  */
+  /* Acquires the GC lock to avoid a data race.                         */
   GC_API void GC_CALL GC_start_mark_threads(void);
 
   /* Explicitly enable GC_register_my_thread() invocation.              */
diff --git a/mark.c b/mark.c
index b005a1d2..53ca3e4a 100644
--- a/mark.c
+++ b/mark.c
@@ -924,7 +924,7 @@ GC_INNER void GC_wait_for_markers_init(void)
 {
   signed_word count;
 
-  GC_ASSERT(I_DONT_HOLD_LOCK());
+  GC_ASSERT(I_HOLD_LOCK());
   if (GC_markers_m1 == 0)
     return;
 
@@ -937,15 +937,12 @@ GC_INNER void GC_wait_for_markers_init(void)
   {
     size_t bytes_to_get =
                 ROUNDUP_PAGESIZE_IF_MMAP(LOCAL_MARK_STACK_SIZE * sizeof(mse));
-    DCL_LOCK_STATE;
 
     GC_ASSERT(GC_page_size != 0);
     GC_main_local_mark_stack = (mse *)GET_MEM(bytes_to_get);
     if (NULL == GC_main_local_mark_stack)
       ABORT("Insufficient memory for main local_mark_stack");
-    LOCK();
     GC_add_to_our_memory((ptr_t)GC_main_local_mark_stack, bytes_to_get);
-    UNLOCK();
   }
 
   /* Reuse marker lock and builders count to synchronize        */
diff --git a/misc.c b/misc.c
index d6cab723..646dbb31 100644
--- a/misc.c
+++ b/misc.c
@@ -1348,13 +1348,7 @@ GC_API void GC_CALL GC_init(void)
 #       endif
 #       ifdef PARALLEL_MARK
           /* Actually start helper threads.     */
-#         if defined(GC_ASSERTIONS) && defined(GC_ALWAYS_MULTITHREADED)
-            UNLOCK();
-#         endif
           GC_start_mark_threads_inner();
-#         if defined(GC_ASSERTIONS) && defined(GC_ALWAYS_MULTITHREADED)
-            LOCK();
-#         endif
 #       endif
 #   endif
     COND_DUMP;
@@ -1458,9 +1452,12 @@ GC_API void GC_CALL GC_enable_incremental(void)
        && !defined(THREAD_SANITIZER)
       /* TSan does not support threads creation in the child process.   */
       IF_CANCEL(int cancel_state;)
+      DCL_LOCK_STATE;
 
       DISABLE_CANCEL(cancel_state);
+      LOCK();
       GC_start_mark_threads_inner();
+      UNLOCK();
       RESTORE_CANCEL(cancel_state);
 #   else
       /* No action since parallel markers are disabled (or no POSIX fork). */
diff --git a/pthread_support.c b/pthread_support.c
index bebbf92e..34cf21f2 100644
--- a/pthread_support.c
+++ b/pthread_support.c
@@ -457,7 +457,7 @@ GC_INNER void GC_start_mark_threads_inner(void)
       sigset_t set, oldset;
 #   endif
 
-    GC_ASSERT(I_DONT_HOLD_LOCK());
+    GC_ASSERT(I_HOLD_LOCK());
     if (available_markers_m1 <= 0) return;
                 /* Skip if parallel markers disabled or already started. */
 #   ifdef CAN_HANDLE_FORK
diff --git a/win32_threads.c b/win32_threads.c
index 80b8c637..dad450ab 100644
--- a/win32_threads.c
+++ b/win32_threads.c
@@ -2131,7 +2131,7 @@ GC_INNER void GC_get_next_stack(char *start, char *limit,
         sigset_t set, oldset;
 #     endif
 
-      GC_ASSERT(I_DONT_HOLD_LOCK());
+      GC_ASSERT(I_HOLD_LOCK());
       if (available_markers_m1 <= 0) return;
                 /* Skip if parallel markers disabled or already started. */
 #     ifdef CAN_HANDLE_FORK
@@ -2304,7 +2304,7 @@ GC_INNER void GC_get_next_stack(char *start, char *limit,
     {
       int i;
 
-      GC_ASSERT(I_DONT_HOLD_LOCK());
+      GC_ASSERT(I_HOLD_LOCK());
       if (available_markers_m1 <= 0) return;
 
       GC_ASSERT(GC_fl_builder_count == 0);