bug#36609: 27.0.50; Possible race-condition in threading implementation

[Pip Cet <pipcet@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 315 bytes --]

Patch attached.

It's possible this'll livelock certain glib libraries that want to
modify the glib main context while another thread is running. I don't
see a good way to fix that, but a bad way would be to apply some
heuristic (such as a 10 ms delay between calling select() and assuming
it actually was called).

[-- Attachment #2: 0001-Fix-lock-failures-in-xg_select-bug-36609.patch --]
[-- Type: text/x-patch, Size: 4691 bytes --]

From d99222b1cf5b63e8361e9a7cdbe13faafb40c1d5 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Sat, 13 Jul 2019 18:09:38 +0000
Subject: [PATCH] Fix lock failures in xg_select (bug#36609)

* src/xgselect.c (release_select_lock, acquire_select_lock):
Introduce.
(xg_select): Use `acquire_select_lock', `release_select_lock'.
* src/thread.c (release_select_lock): Introduce for non-GLib builds.
(really_call_select): Call `release_select_lock'.  Simplify by
ensuring acquisition of the lock always succeeds.
---
 src/thread.c   |  8 ++++++++
 src/xgselect.c | 42 ++++++++++++++++++++++++++++--------------
 src/xgselect.h |  2 ++
 3 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/src/thread.c b/src/thread.c
index e2deadd7a8..40a25fb5cd 100644
--- a/src/thread.c
+++ b/src/thread.c
@@ -28,6 +28,12 @@ Copyright (C) 2012-2019 Free Software Foundation, Inc.
 #include "pdumper.h"
 #include "keyboard.h"
 
+#ifdef HAVE_GLIB
+#include <xgselect.h>
+#else
+#define release_select_lock() do { } while (0)
+#endif
+
 union aligned_thread_state
 {
   struct thread_state s;
@@ -586,6 +592,8 @@ really_call_select (void *arg)
   sa->result = (sa->func) (sa->max_fds, sa->rfds, sa->wfds, sa->efds,
 			   sa->timeout, sa->sigmask);
 
+  release_select_lock ();
+
   block_interrupt_signal (&oldset);
   /* If we were interrupted by C-g while inside sa->func above, the
      signal handler could have called maybe_reacquire_global_lock, in
diff --git a/src/xgselect.c b/src/xgselect.c
index 9982a1f0e9..82f0a61882 100644
--- a/src/xgselect.c
+++ b/src/xgselect.c
@@ -29,6 +29,27 @@ Copyright (C) 2009-2019 Free Software Foundation, Inc.
 #include "blockinput.h"
 #include "systime.h"
 
+static ptrdiff_t threads_holding_glib_lock;
+static GMainContext *glib_main_context;
+
+void release_select_lock (void)
+{
+  if (--threads_holding_glib_lock == 0)
+    g_main_context_release (glib_main_context);
+}
+
+static void acquire_select_lock (GMainContext *context)
+{
+  if (threads_holding_glib_lock++ == 0)
+    {
+      glib_main_context = context;
+      while (!g_main_context_acquire (context))
+	{
+	  /* Spin. */
+	}
+    }
+}
+
 /* `xg_select' is a `pselect' replacement.  Why do we need a separate function?
    1. Timeouts.  Glib and Gtk rely on timer events.  If we did pselect
       with a greater timeout then the one scheduled by Glib, we would
@@ -54,26 +75,19 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
   GPollFD *gfds = gfds_buf;
   int gfds_size = ARRAYELTS (gfds_buf);
   int n_gfds, retval = 0, our_fds = 0, max_fds = fds_lim - 1;
-  bool context_acquired = false;
   int i, nfds, tmo_in_millisec, must_free = 0;
   bool need_to_dispatch;
 
   context = g_main_context_default ();
-  context_acquired = g_main_context_acquire (context);
-  /* FIXME: If we couldn't acquire the context, we just silently proceed
-     because this function handles more than just glib file descriptors.
-     Note that, as implemented, this failure is completely silent: there is
-     no feedback to the caller.  */
+  acquire_select_lock (context);
 
   if (rfds) all_rfds = *rfds;
   else FD_ZERO (&all_rfds);
   if (wfds) all_wfds = *wfds;
   else FD_ZERO (&all_wfds);
 
-  n_gfds = (context_acquired
-	    ? g_main_context_query (context, G_PRIORITY_LOW, &tmo_in_millisec,
-				    gfds, gfds_size)
-	    : -1);
+  n_gfds = g_main_context_query (context, G_PRIORITY_LOW, &tmo_in_millisec,
+				 gfds, gfds_size);
 
   if (gfds_size < n_gfds)
     {
@@ -151,8 +165,10 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
 #else
   need_to_dispatch = true;
 #endif
-  if (need_to_dispatch && context_acquired)
+  if (need_to_dispatch)
     {
+      acquire_select_lock (context);
+
       int pselect_errno = errno;
       /* Prevent g_main_dispatch recursion, that would occur without
          block_input wrapper, because event handlers call
@@ -162,11 +178,9 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
         g_main_context_dispatch (context);
       unblock_input ();
       errno = pselect_errno;
+      release_select_lock ();
     }
 
-  if (context_acquired)
-    g_main_context_release (context);
-
   /* To not have to recalculate timeout, return like this.  */
   if ((our_fds > 0 || (nfds == 0 && tmop == &tmo)) && (retval == 0))
     {
diff --git a/src/xgselect.h b/src/xgselect.h
index 92c79c5f64..0d5c96cd21 100644
--- a/src/xgselect.h
+++ b/src/xgselect.h
@@ -29,4 +29,6 @@ #define XGSELECT_H
 		      fd_set *rfds, fd_set *wfds, fd_set *efds,
 		      struct timespec *timeout, sigset_t *sigmask);
 
+extern void release_select_lock (void);
+
 #endif /* XGSELECT_H */
-- 
2.20.1