[glib: 1/2] gmain: Access Unix signal handler state atomically

[Philip Withnall <pwithnall src gnome org>]

commit 253f5cda82bce8c1dba5e2d3516ff41b52d30989
Author: Philip Withnall <withnall endlessm com>
Date:   Wed Dec 18 16:46:13 2019 +0000

    gmain: Access Unix signal handler state atomically
    
    There are two variables which are used to pass state from the Unix
    signal handler interrupt function to the rest of `gmain.c`. They are
    currently defined as `sig_atomic_t`, which means that they are
    guaranteed to be interrupt safe. However, it does not guarantee they are
    thread-safe, and GLib attaches its signal handler interrupt function to
    a worker thread.
    
    Make them thread-safe using atomics. It’s not possible to use locks, as
    pthread mutex functions are not signal-handler-safe. In particular, this
    means we have to be careful not to end up using GLib’s fallback atomics
    implementation, as that secretly uses a mutex. Better to be unsafe than
    have a re-entrant call into `pthread_mutex_lock()` from a nested signal
    handler.
    
    This commit solves two problems:
     1. Writes to `any_unix_signal_pending` and `unix_signal_pending` could
        be delivered out of order to the worker thread which calls
        `dispatch_unix_signals()`, resulting in signals not being handled
        until the next iteration of that worker thread. This is a
        performance problem but not a correctness problem.
     2. Setting an element of `unix_signal_pending` from
        `g_unix_signal_handler()` and clearing it from
        `dispatch_unix_signals_unlocked()` (in the worker thread) could
        race, resulting in a signal emission being cleared without being
        handled. That’s a correctness problem.
    
    Signed-off-by: Philip Withnall <withnall endlessm com>
    
    Fixes: #1670

 glib/gmain.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)
---
diff --git a/glib/gmain.c b/glib/gmain.c
index ee23a6519..88140c865 100644
--- a/glib/gmain.c
+++ b/glib/gmain.c
@@ -448,6 +448,10 @@ static GMainContext *glib_worker_context;
 
 /* UNIX signals work by marking one of these variables then waking the
  * worker context to check on them and dispatch accordingly.
+ *
+ * Both variables must be accessed using atomic primitives, unless those atomic
+ * primitives are implemented using fallback mutexes (as those aren’t safe in
+ * an interrupt context).
  */
 #ifdef HAVE_SIG_ATOMIC_T
 static volatile sig_atomic_t unix_signal_pending[NSIG];
@@ -5199,7 +5203,7 @@ dispatch_unix_signals_unlocked (void)
   gint i;
 
   /* clear this first in case another one arrives while we're processing */
-  any_unix_signal_pending = FALSE;
+  g_atomic_int_set (&any_unix_signal_pending, 0);
 
   /* We atomically test/clear the bit from the global array in case
    * other signals arrive while we are dispatching.
@@ -5218,9 +5222,7 @@ dispatch_unix_signals_unlocked (void)
        *
        * Note specifically: we must check _our_ array.
        */
-      pending[i] = unix_signal_pending[i];
-      if (pending[i])
-        unix_signal_pending[i] = FALSE;
+      pending[i] = g_atomic_int_compare_and_exchange (&unix_signal_pending[i], 1, 0);
     }
 
   /* handle GChildWatchSource instances */
@@ -5526,8 +5528,14 @@ g_unix_signal_handler (int signum)
 {
   gint saved_errno = errno;
 
-  unix_signal_pending[signum] = TRUE;
-  any_unix_signal_pending = TRUE;
+#if defined(G_ATOMIC_LOCK_FREE) && defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4)
+  g_atomic_int_set (&unix_signal_pending[signum], 1);
+  g_atomic_int_set (&any_unix_signal_pending, 1);
+#else
+#warning "Can't use atomics in g_unix_signal_handler(): Unix signal handling will be racy"
+  unix_signal_pending[signum] = 1;
+  any_unix_signal_pending = 1;
+#endif
 
   g_wakeup_signal (glib_worker_context->wakeup);
 
@@ -5996,7 +6004,7 @@ glib_worker_main (gpointer data)
       g_main_context_iteration (glib_worker_context, TRUE);
 
 #ifdef G_OS_UNIX
-      if (any_unix_signal_pending)
+      if (g_atomic_int_get (&any_unix_signal_pending))
         dispatch_unix_signals ();
 #endif
     }