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

[dick.r.chiang@gmail.com]

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

Four attachments:

#1. Want to revert commit 9c62ffb

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Revert-Fix-lock-failures-in-xg_select.patch --]
[-- Type: text/x-diff, Size: 4545 bytes --]

From 01b8b847f5fc4b65eaac06c7c2d85f8c5410c497 Mon Sep 17 00:00:00 2001
From: dickmao <none>
Date: Sun, 6 Jun 2021 11:10:13 -0400
Subject: [PATCH] Revert "Fix lock failures in xg_select"

This reverts commit 9c62ffb08262c82b7e38e6eb5767f2087424aa47.
* src/thread.c (really_call_select): revert 9c62ffb
* src/xgselect.c (xg_select): revert 9c62ffb
---
 src/thread.c   |  8 --------
 src/xgselect.c | 42 ++++++++++++++----------------------------
 src/xgselect.h |  2 --
 3 files changed, 14 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..d7c63e3be1 100644
--- a/src/xgselect.c
+++ b/src/xgselect.c
@@ -29,27 +29,6 @@ Copyright (C) 2009-2021 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
@@ -75,19 +54,26 @@ 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);
+  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 +151,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 +162,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: 57 bytes --]


#2. Fails on tip of master, succeeds after patch in #1.

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

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


#3. Abridged original example, fails after patch in #1.

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

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

Fails not necessarily because xgselect.c is wrong, but rather because
channel-recv blocks on a mutex before channel-send can get its act together.
This was hard for all to discern because OP seemed to have gone out of his way
to obfuscate his "minimum" example.

#4. What #3 probably intended, succeeds after patch in #1.

[-- Attachment #8: report-2.el --]
[-- Type: application/emacs-lisp, Size: 5014 bytes --]