glib-unix: fix handling of multiple signal source for the same signal

We can't reset the pending flag for a signal until we've traversed
the whole list, as the documentation clearly says that in case multiple
sources they all get invoked.
This is still racy if you get a signal after checking the flag
but before resetting it, but it was the same before. The correct
fix would be to use sigwait() or signalfd(), but that would mean
blocking all signals in all threads, which is not compatible
with existing applications.

https://bugzilla.gnome.org/show_bug.cgi?id=704322

diff --git a/glib/gmain.c b/glib/gmain.c
index 7f1d2b9..695336d 100644 (file)
--- a/glib/gmain.c
+++ b/glib/gmain.c
@@ -4828,8 +4828,6 @@ dispatch_unix_signals (void)
   /* handle GChildWatchSource instances */
   if (unix_signal_pending[SIGCHLD])
     {
-      unix_signal_pending[SIGCHLD] = FALSE;
-
       /* The only way we can do this is to scan all of the children.
        *
        * The docs promise that we will not reap children that we are not
@@ -4875,7 +4873,6 @@ dispatch_unix_signals (void)
         {
           if (unix_signal_pending[source->signum])
             {
-              unix_signal_pending[source->signum] = FALSE;
               source->pending = TRUE;
 
               wake_source ((GSource *) source);
@@ -4883,6 +4880,8 @@ dispatch_unix_signals (void)
         }
     }
 
+  memset ((void*)unix_signal_pending, 0, sizeof (unix_signal_pending));
+
   G_UNLOCK(unix_signal_lock);
 }
 
@@ -4994,7 +4993,6 @@ _g_main_create_unix_signal_watch (int signum)
   unix_signal_watches = g_slist_prepend (unix_signal_watches, unix_signal_source);
   if (unix_signal_pending[signum])
     unix_signal_source->pending = TRUE;
-  unix_signal_pending[signum] = FALSE;
   G_UNLOCK (unix_signal_lock);
 
   return source;

diff --git a/glib/tests/unix.c b/glib/tests/unix.c
index 329e19a..c523475 100644 (file)
--- a/glib/tests/unix.c
+++ b/glib/tests/unix.c
@@ -65,6 +65,7 @@ test_error (void)
 }
 
 static gboolean sig_received = FALSE;
+static int sig_counter = 0;
 
 static gboolean
 on_sig_received (gpointer user_data)
@@ -72,6 +73,7 @@ on_sig_received (gpointer user_data)
   GMainLoop *loop = user_data;
   g_main_loop_quit (loop);
   sig_received = TRUE;
+  sig_counter ++;
   return G_SOURCE_REMOVE;
 }
 
@@ -92,6 +94,17 @@ exit_mainloop (gpointer data)
   return G_SOURCE_REMOVE;
 }
 
+static gboolean
+on_sig_received_2 (gpointer data)
+{
+  GMainLoop *loop = data;
+
+  sig_counter ++;
+  if (sig_counter == 2)
+    g_main_loop_quit (loop);
+  return G_SOURCE_REMOVE;
+}
+
 static void
 test_signal (int signum)
 {
@@ -101,6 +114,7 @@ test_signal (int signum)
   mainloop = g_main_loop_new (NULL, FALSE);
 
   sig_received = FALSE;
+  sig_counter = 0;
   g_unix_signal_add (signum, on_sig_received, mainloop);
   kill (getpid (), signum);
   g_assert (!sig_received);
@@ -114,8 +128,19 @@ test_signal (int signum)
   g_timeout_add (500, exit_mainloop, mainloop);
   g_main_loop_run (mainloop);
   g_assert (!sig_received);
-  g_main_loop_unref (mainloop);
 
+  /* Ensure that two sources for the same signal get it */
+  sig_counter = 0;
+  g_unix_signal_add (signum, on_sig_received_2, mainloop);
+  g_unix_signal_add (signum, on_sig_received_2, mainloop);
+  id = g_timeout_add (500, sig_not_received, mainloop);
+
+  kill (getpid (), signum);
+  g_main_loop_run (mainloop);
+  g_assert_cmpint (sig_counter, ==, 2);
+  g_source_remove (id);
+
+  g_main_loop_unref (mainloop);
 }
 
 static void
@@ -147,6 +172,48 @@ test_sighup_add_remove (void)
 
 }
 
+static gboolean
+nested_idle (gpointer data)
+{
+  GMainLoop *parent, *nested;
+  GMainContext *context;
+  GSource *source;
+
+  context = g_main_context_new ();
+  nested = g_main_loop_new (context, FALSE);
+
+  source = g_unix_signal_source_new (SIGHUP);
+  g_source_set_callback (source, on_sig_received, nested, NULL);
+  g_source_attach (source, context);
+
+  kill (getpid (), SIGHUP);
+  g_main_loop_run (nested);
+  g_assert_cmpint (sig_counter, ==, 1);
+
+  g_main_loop_unref (nested);
+  g_main_context_unref (context);
+
+  return G_SOURCE_REMOVE;
+}
+
+static void
+test_sighup_nested (void)
+{
+  GMainLoop *mainloop;
+
+  mainloop = g_main_loop_new (NULL, FALSE);
+
+  sig_counter = 0;
+  sig_received = FALSE;
+  g_unix_signal_add (SIGHUP, on_sig_received, mainloop);
+  g_idle_add (nested_idle, mainloop);
+
+  g_main_loop_run (mainloop);
+  g_assert_cmpint (sig_counter, ==, 2);
+
+  g_main_loop_unref (mainloop);
+}
+
 int
 main (int   argc,
       char *argv[])
@@ -159,6 +226,7 @@ main (int   argc,
   g_test_add_func ("/glib-unix/sigterm", test_sigterm);
   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);
 
   return g_test_run();
 }