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

[Pip Cet <pipcet@gmail.com>]

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

On Fri, Jul 12, 2019 at 9:02 AM Pip Cet <pipcet@gmail.com> wrote:
>
> On Thu, Jul 11, 2019 at 8:52 PM Andreas Politz
> <politza@hochschule-trier.de> wrote:
> > I think there is a race-condition in the implementation of threads.  I
> > tried to find a minimal test-case, without success.  Thus, I've attached
> > a lengthy source-file.  Loading that file should trigger this bug and
> > may freeze your session.
>
> It does here, so I can provide further debugging information if
> needed. On first glance, it appears that xgselect returns abnormally
> with g_main_context acquired in one thread, and then other threads
> fail to acquire it and loop endlessly.

Okay, I'm still not sure this is really the problem Andreas was
seeing, but this code fails to work with xg_select:

(let ((thread (make-thread (lambda ()
                 (sleep-for 3)))))
  (thread-yield)
  (thread-signal thread 'error nil))

Proposed patch attached.

[-- Attachment #2: 0001-Protect-against-abnormal-exit-in-xg_select.patch --]
[-- Type: text/x-patch, Size: 2239 bytes --]

From 012341a8a674b130c335cfdd8d24888406e98ece Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Fri, 12 Jul 2019 12:39:29 +0000
Subject: [PATCH] Protect against abnormal exit in `xg_select'

* src/xgselect.c (release_g_main_context): New function.
(xg_select): Unwind-protect release of the GLib main context.
---
 src/xgselect.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/src/xgselect.c b/src/xgselect.c
index 9982a1f0e9..e35310be34 100644
--- a/src/xgselect.c
+++ b/src/xgselect.c
@@ -29,6 +29,15 @@ Copyright (C) 2009-2019 Free Software Foundation, Inc.
 #include "blockinput.h"
 #include "systime.h"
 
+/* Helper function for `unwind_protect'.  */
+
+static void release_g_main_context (void *ptr)
+{
+  GMainContext *context = (GMainContext *) ptr;
+
+  g_main_context_release (context);
+}
+
 /* `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
@@ -58,8 +67,12 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
   int i, nfds, tmo_in_millisec, must_free = 0;
   bool need_to_dispatch;
 
+  ptrdiff_t count = SPECPDL_INDEX ();
   context = g_main_context_default ();
   context_acquired = g_main_context_acquire (context);
+
+  if (context_acquired)
+    record_unwind_protect_ptr (release_g_main_context, (void *)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
@@ -164,9 +177,6 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
       errno = pselect_errno;
     }
 
-  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))
     {
@@ -174,6 +184,8 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
       errno = EINTR;
     }
 
+  unbind_to (count, Qnil);
+
   return retval;
 }
 #endif /* HAVE_GLIB */
-- 
2.20.1