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

[dick.r.chiang@gmail.com]

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

EZ> You will have to provide a more detailed analysis, sorry.

There is no guarantee the static variable `threads_holding_glib_lock`
introduced in xgselect.c in commit 9c62ffb is synchronized across threads.  As
such, relying on it becoming zero in time for release_select_lock() is fraught.
(If you add print statements, this particular heisenbug disappears, at least
on Linux kernel 4.15.0-99-generic).

Four attachments:

1. Desired patch to master (reverts 9c62ffb, adds main-thread-p check).

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Only-acquire-glib-context-if-main-thread.patch --]
[-- Type: text/x-diff, Size: 4523 bytes --]

From 5b30b55f60844ada43b119640e052f8ed5ee6e9c Mon Sep 17 00:00:00 2001
From: dickmao <none>
Date: Wed, 9 Jun 2021 17:11:48 -0400
Subject: [PATCH] Only acquire glib context if main thread

* src/thread.c (really_call_select): revert 9c62ffb
* src/xgselect.c (xg_select): revert 9c62ffb, only acquire glib if main
---
 src/thread.c   |  8 --------
 src/xgselect.c | 44 ++++++++++++++++----------------------------
 src/xgselect.h |  2 --
 3 files changed, 16 insertions(+), 38 deletions(-)

diff --git a/src/thread.c b/src/thread.c
index f74f611148..609cd7c5fc 100644
--- a/src/thread.c
+++ b/src/thread.c
@@ -28,12 +28,6 @@ Copyright (C) 2012-2021 Free Software Foundation, Inc.
 #include "pdumper.h"
 #include "keyboard.h"
 
-#if defined HAVE_GLIB && ! defined (HAVE_NS)
-#include <xgselect.h>
-#else
-#define release_select_lock() do { } while (0)
-#endif
-
 union aligned_thread_state
 {
   struct thread_state s;
@@ -592,8 +586,6 @@ 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 0d91d55bad..b40f75cb50 100644
--- a/src/xgselect.c
+++ b/src/xgselect.c
@@ -28,27 +28,7 @@ Copyright (C) 2009-2021 Free Software Foundation, Inc.
 #include "lisp.h"
 #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. */
-	}
-    }
-}
+#include "thread.h"
 
 /* `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
@@ -75,19 +55,27 @@ 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 ();
-  acquire_select_lock (context);
+  if (main_thread_p (current_thread))
+    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.  */
 
   if (rfds) all_rfds = *rfds;
   else FD_ZERO (&all_rfds);
   if (wfds) all_wfds = *wfds;
   else FD_ZERO (&all_wfds);
 
-  n_gfds = g_main_context_query (context, G_PRIORITY_LOW, &tmo_in_millisec,
-				 gfds, gfds_size);
+  n_gfds = (context_acquired
+	    ? g_main_context_query (context, G_PRIORITY_LOW, &tmo_in_millisec,
+				    gfds, gfds_size)
+	    : -1);
 
   if (gfds_size < n_gfds)
     {
@@ -165,10 +153,8 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
 #else
   need_to_dispatch = true;
 #endif
-  if (need_to_dispatch)
+  if (need_to_dispatch && context_acquired)
     {
-      acquire_select_lock (context);
-
       int pselect_errno = errno;
       /* Prevent g_main_dispatch recursion, that would occur without
          block_input wrapper, because event handlers call
@@ -178,9 +164,11 @@ 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 2142a236b2..e00dce1283 100644
--- a/src/xgselect.h
+++ b/src/xgselect.h
@@ -29,6 +29,4 @@ #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.26.2


[-- Attachment #3: Type: text/plain, Size: 195 bytes --]


2. OP's original "my code doesn't work, here you figure it out", obfuscated MRE
Run as:
for i in 1 2 3 ; do src/emacs -Q -l ./report-orig.el & done
Fails if:
One of the emacsen stops responding

[-- Attachment #4: report-orig.el --]
[-- Type: application/emacs-lisp, Size: 6345 bytes --]

[-- Attachment #5: Type: text/plain, Size: 206 bytes --]


3. "Affine" redaction of OP's MRE *infrequently* breaks on tip of master (succeeds
after patch)
Run as:
for i in 1 2 3 ; do src/emacs -Q -l ./report.el & done
Fails if:
One of the emacsen stops responding

[-- Attachment #6: report.el --]
[-- Type: application/emacs-lisp, Size: 704 bytes --]

[-- Attachment #7: Type: text/plain, Size: 68 bytes --]


4. My MRE *frequently* breaks tip of master (succeeds after patch)

[-- Attachment #8: 42.el --]
[-- Type: application/emacs-lisp, Size: 1551 bytes --]

[-- Attachment #9: Type: text/plain, Size: 105 bytes --]

Run as:
for i in 1 2 3 ; do src/emacs -Q -l ./42.el & done
Fails if:
One of the emacsen stops responding