Merge branch 'gmain-tsan' into 'master'

gmain: Fix data races in GUnixSignalWatchSource and GChildWatchSource

See merge request GNOME/glib!446

glib/gmain.c

@@ -327,7 +327,7 @@ struct _GChildWatchSource
 #ifdef G_OS_WIN32
   GPollFD     poll;
 #else /* G_OS_WIN32 */
-  gboolean    child_exited;
+  gboolean    child_exited; /* (atomic) */
 #endif /* G_OS_WIN32 */
 };
 
@@ -335,7 +335,7 @@ struct _GUnixSignalWatchSource
 {
   GSource     source;
   int         signum;
-  gboolean    pending;
+  gboolean    pending; /* (atomic) */
 };
 
 struct _GPollRec
@@ -462,10 +462,10 @@ static volatile sig_atomic_t any_unix_signal_pending;
 static volatile int unix_signal_pending[NSIG];
 static volatile int any_unix_signal_pending;
 #endif
-static volatile guint unix_signal_refcount[NSIG];
 
 /* Guards all the data below */
 G_LOCK_DEFINE_STATIC (unix_signal_lock);
+static guint unix_signal_refcount[NSIG];
 static GSList *unix_signal_watches;
 static GSList *unix_child_watches;
 
@@ -5107,7 +5107,7 @@ dispatch_unix_signals_unlocked (void)
         {
           GChildWatchSource *source = node->data;
 
-          if (!source->child_exited)
+          if (!g_atomic_int_get (&source->child_exited))
             {
               pid_t pid;
               do
@@ -5117,14 +5117,14 @@ dispatch_unix_signals_unlocked (void)
                   pid = waitpid (source->pid, &source->child_status, WNOHANG);
                   if (pid > 0)
                     {
-                      source->child_exited = TRUE;
+                      g_atomic_int_set (&source->child_exited, TRUE);
                       wake_source ((GSource *) source);
                     }
                   else if (pid == -1 && errno == ECHILD)
                     {
                       g_warning ("GChildWatchSource: Exit status of a child process was requested but ECHILD was received by waitpid(). See the documentation of g_child_watch_source_new() for possible causes.");
-                      source->child_exited = TRUE;
                       source->child_status = 0;
+                      g_atomic_int_set (&source->child_exited, TRUE);
                       wake_source ((GSource *) source);
                     }
                 }
@@ -5138,14 +5138,10 @@ dispatch_unix_signals_unlocked (void)
     {
       GUnixSignalWatchSource *source = node->data;
 
-      if (!source->pending)
+      if (pending[source->signum] &&
+          g_atomic_int_compare_and_exchange (&source->pending, FALSE, TRUE))
         {
-          if (pending[source->signum])
+          wake_source ((GSource *) source);
-            {
-              source->pending = TRUE;
-
-              wake_source ((GSource *) source);
-            }
         }
     }
 
@@ -5167,7 +5163,7 @@ g_child_watch_prepare (GSource *source,
 
   child_watch_source = (GChildWatchSource *) source;
 
-  return child_watch_source->child_exited;
+  return g_atomic_int_get (&child_watch_source->child_exited);
 }
 
 static gboolean
@@ -5177,7 +5173,7 @@ g_child_watch_check (GSource *source)
 
   child_watch_source = (GChildWatchSource *) source;
 
-  return child_watch_source->child_exited;
+  return g_atomic_int_get (&child_watch_source->child_exited);
 }
 
 static gboolean
@@ -5188,7 +5184,7 @@ g_unix_signal_watch_prepare (GSource *source,
 
   unix_signal_source = (GUnixSignalWatchSource *) source;
 
-  return unix_signal_source->pending;
+  return g_atomic_int_get (&unix_signal_source->pending);
 }
 
 static gboolean
@@ -5198,7 +5194,7 @@ g_unix_signal_watch_check (GSource  *source)
 
   unix_signal_source = (GUnixSignalWatchSource *) source;
 
-  return unix_signal_source->pending;
+  return g_atomic_int_get (&unix_signal_source->pending);
 }
 
 static gboolean
@@ -5218,9 +5214,9 @@ g_unix_signal_watch_dispatch (GSource    *source,
       return FALSE;
     }
 
-  again = (callback) (user_data);
+  g_atomic_int_set (&unix_signal_source->pending, FALSE);
 
-  unix_signal_source->pending = FALSE;
+  again = (callback) (user_data);
 
   return again;
 }

glib/tests/unix.c

@@ -245,6 +245,56 @@ test_sighup_nested (void)
   g_main_loop_unref (mainloop);
 }
 
+static gboolean
+on_sigwinch_received (gpointer data)
+{
+  GMainLoop *loop = (GMainLoop *) data;
+
+  sig_counter ++;
+
+  if (sig_counter == 1)
+    kill (getpid (), SIGWINCH);
+  else if (sig_counter == 2)
+    g_main_loop_quit (loop);
+  else if (sig_counter > 2)
+    g_assert_not_reached ();
+
+  /* Increase the time window in which an issue could happen. */
+  g_usleep (G_USEC_PER_SEC);
+
+  return G_SOURCE_CONTINUE;
+}
+
+static void
+test_callback_after_signal (void)
+{
+  /* Checks that user signal callback is invoked *after* receiving a signal.
+   * In other words a new signal is never merged with the one being currently
+   * dispatched or whose dispatch had already finished. */
+
+  GMainLoop *mainloop;
+  GMainContext *context;
+  GSource *source;
+
+  sig_counter = 0;
+
+  context = g_main_context_new ();
+  mainloop = g_main_loop_new (context, FALSE);
+
+  source = g_unix_signal_source_new (SIGWINCH);
+  g_source_set_callback (source, on_sigwinch_received, mainloop, NULL);
+  g_source_attach (source, context);
+  g_source_unref (source);
+
+  g_assert_cmpint (sig_counter, ==, 0);
+  kill (getpid (), SIGWINCH);
+  g_main_loop_run (mainloop);
+  g_assert_cmpint (sig_counter, ==, 2);
+
+  g_main_loop_unref (mainloop);
+  g_main_context_unref (context);
+}
+
 int
 main (int   argc,
       char *argv[])
@@ -259,6 +309,7 @@ main (int   argc,
   g_test_add_func ("/glib-unix/sighup_again", test_sighup);
   g_test_add_func ("/glib-unix/sighup_add_remove", test_sighup_add_remove);
   g_test_add_func ("/glib-unix/sighup_nested", test_sighup_nested);
+  g_test_add_func ("/glib-unix/callback_after_signal", test_callback_after_signal);
 
   return g_test_run();
 }