bug#56487: xgselect race condition leading to abort when USE_GTK not defined

bug#56487: xgselect race condition leading to abort when USE_GTK not defined

[Tom Gillespie]

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

There is a race condition in xgselect.c leading to an abort being
raised in glib code. I have attached a patch that fixes the issue.

It took an absurd amount of time to track this down and debug. The
patch and repro file contain more information.

This is not the easiest bug to test for because the race condition is
non-deterministic, I have attached the elisp file that I use to
explore and reproduce the issue.

Ensure that you have glib installed and that you are building --with-x
so that the code in xgselect is enabled. Run configure as ./configure
--with-x-toolkit=lucid (or anything except gtk), and then run make.

Assuming process-thread-bugs.el is in the top level of the emacs repo
define the following function

function loop-abort () { while true; do src/emacs -Q -batch -l \
./process-thread-bugs.el will abort 1 || return $?; done }

You can then run loop-abort; echo $? and when the patch is not applied
Emacs will eventually abort. You can also add a call to fputs in
thread.c at the location of the comment and you will almost
immediately see the abort behavior, even when building with gtk.

[-- Attachment #2: 0001-xgselect.c-avoid-race-condition-leading-to-abort.patch --]
[-- Type: application/x-patch, Size: 5090 bytes --]

[-- Attachment #3: process-thread-bugs.el --]
[-- Type: application/x-lisp, Size: 9459 bytes --]

bug#56487: xgselect race condition leading to abort when USE_GTK not defined

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Tom Gillespie <tgbugs@gmail.com> writes:

> From e120751b9de79be521f490f8c2f99597868b3208 Mon Sep 17 00:00:00 2001
> From: Tom Gillespie <tgbugs@gmail.com>
> Date: Tue, 5 Jul 2022 13:43:25 -0700
> Subject: [PATCH] xgselect.c: avoid race condition leading to abort
>
> xgselect.c (xg_select): Remove ifdefs that were previously USE_GTK
> specific to be unconditional. This prevents a race condition caused by
> a call to release_select_lock from triggering when configuring Emacs
> with any toolkit other than gtk.
>
> xgselect.c (release_select_lock): Add a branch to check whether
> threads_holding_glib_lock has gone negative, and if so, restore to zero.
> In the case where there are multiple threads, threads_holding_glib_lock
> could become and stay negative, prevending g_main_context_release from
> ever being called, even when it should have been.
>
> As far as I can tell the way that the thread.c and xgselect.c code was
> written was with the intention of avoiding additional locks. This means
> that this code is exquisitely senstivie to slight changes in timing. A
> comment in thread.c has been added at one location where this happens.
>
> It is entirely possible that the removal of the ifdefs branching on
> USE_GTK resolves this issue by slightly changing the timings when
> using other/no toolkits so that the abort does not trigger. In all
> cases aborts can be induced by adding something like fputs in thread.c
> at the location of the new comment.
>
> For the record, the abort behavior is not present in Emacs 27, and was
> introduced by 9c62ffb08262c82b7e38e6eb5767f2087424aa47 (the bisect was
> quite a pain, so hopefully no one ever needs to do it again).

Thanks.  The commit message does not really comply with our style,
however.  The ChangeLog entries should look like this instead:


* src/xgselect.c (xg_select): Remove ifdefs that were previously
USE_GTK specific to be unconditional. This prevents a race
condition caused by a call to release_select_lock from
triggering when configuring Emacs with any toolkit other than
GTK.
(release_select_lock): Add a branch to check whether
threads_holding_glib_lock has gone negative, and if so, restore
to zero.  In the case where there are multiple threads,
threads_holding_glib_lock could become and stay negative,
prevending g_main_context_release from ever being called, even
when it should have been.


You can use C-c C-w inside a *vc-log* buffer to generate an
appropriately formatted commit message.

bug#56487: xgselect race condition leading to abort when USE_GTK not defined

[Tom Gillespie]

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

Here is (I think) the patch with correctly formatted change log entries.

[-- Attachment #2: 0001-xgselect.c-avoid-race-condition-leading-to-abort.patch --]
[-- Type: text/x-patch, Size: 5085 bytes --]

From b3a7c5715b199c9ce935a901d2eb02f238af0769 Mon Sep 17 00:00:00 2001
From: Tom Gillespie <tgbugs@gmail.com>
Date: Tue, 5 Jul 2022 13:43:25 -0700
Subject: [PATCH] xgselect.c: avoid race condition leading to abort

* src/xgselect.c (xg_select): Remove ifdefs that were previously USE_GTK
specific to be unconditional. This prevents a race condition caused by
a call to release_select_lock from triggering when configuring Emacs
with any toolkit other than gtk.
(release_select_lock): Add a branch to check whether
threads_holding_glib_lock has gone negative, and if so, restore to
zero.  In the case where there are multiple threads,
threads_holding_glib_lock could become and stay negative, prevending
g_main_context_release from ever being called, even when it should
have been.

As far as I can tell the way that the thread.c and xgselect.c code was
written was with the intention of avoiding additional locks. This means
that this code is exquisitely senstivie to slight changes in timing. A
comment in thread.c has been added at one location where this happens.

It is entirely possible that the removal of the ifdefs branching on
USE_GTK resolves this issue by slightly changing the timings when
using other/no toolkits so that the abort does not trigger. In all
cases aborts can be induced by adding something like fputs in thread.c
at the location of the new comment.

For the record, the abort behavior is not present in Emacs 27, and was
introduced by 9c62ffb08262c82b7e38e6eb5767f2087424aa47 (the bisect was
quite a pain, so hopefully no one ever needs to do it again).
---
 src/thread.c   |  5 +++++
 src/xgselect.c | 29 ++++++++++++-----------------
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/src/thread.c b/src/thread.c
index 626d14aad0..a0e85bb75c 100644
--- a/src/thread.c
+++ b/src/thread.c
@@ -612,6 +612,11 @@ really_call_select (void *arg)
   sa->result = (sa->func) (sa->max_fds, sa->rfds, sa->wfds, sa->efds,
 			   sa->timeout, sa->sigmask);
 
+  /* The exact timing of this call is critical. If you are debugging
+     and add a call to e.g. fputs before release_select_lock, then
+     the internal call to g_main_context_release can sometimes result
+     in a negative overflow of gmain context->owner_count, triggering
+     an abort. */
   release_select_lock ();
 
   block_interrupt_signal (&oldset);
diff --git a/src/xgselect.c b/src/xgselect.c
index 6e09a15fa8..9a79121617 100644
--- a/src/xgselect.c
+++ b/src/xgselect.c
@@ -39,12 +39,20 @@ Copyright (C) 2009-2022 Free Software Foundation, Inc.
 void
 release_select_lock (void)
 {
+  ptrdiff_t lthgl;
 #if GNUC_PREREQ (4, 7, 0)
-  if (__atomic_sub_fetch (&threads_holding_glib_lock, 1, __ATOMIC_ACQ_REL) == 0)
-    g_main_context_release (glib_main_context);
+  lthgl = __atomic_sub_fetch (&threads_holding_glib_lock, 1, __ATOMIC_ACQ_REL);
+  if (lthgl == 0)
+      g_main_context_release (glib_main_context);
+  else if (lthgl < 0)
+    /* reset to zero in the event that multiple threads cause this to go to -1 */
+    __atomic_fetch_add (&threads_holding_glib_lock, 1, __ATOMIC_ACQ_REL);
 #else
-  if (--threads_holding_glib_lock == 0)
+  lthgl = --threads_holding_glib_lock;
+  if (lthgl == 0)
     g_main_context_release (glib_main_context);
+  else if (lthgl < 0)
+    threads_holding_glib_lock++;
 #endif
 }
 
@@ -116,9 +124,7 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
   int n_gfds, retval = 0, our_fds = 0, max_fds = fds_lim - 1;
   int i, nfds, tmo_in_millisec, must_free = 0;
   bool need_to_dispatch;
-#ifdef USE_GTK
   bool already_has_events;
-#endif
 
   if (xg_select_suppress_count)
     return pselect (fds_lim, rfds, wfds, efds, timeout, sigmask);
@@ -126,8 +132,8 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
   context = g_main_context_default ();
   acquire_select_lock (context);
 
-#ifdef USE_GTK
   already_has_events = g_main_context_pending (context);
+#ifdef USE_GTK
 #ifndef HAVE_PGTK
   already_has_events = already_has_events && x_gtk_use_native_input;
 #endif
@@ -179,12 +185,6 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
 	tmop = &tmo;
     }
 
-#ifndef USE_GTK
-  fds_lim = max_fds + 1;
-  nfds = thread_select (pselect, fds_lim,
-			&all_rfds, have_wfds ? &all_wfds : NULL, efds,
-			tmop, sigmask);
-#else
   /* On PGTK, when you type a key, the key press event are received,
      and one more key press event seems to be received internally.
 
@@ -217,7 +217,6 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
 	FD_ZERO (efds);
       our_fds++;
     }
-#endif
 
   if (nfds < 0)
     retval = nfds;
@@ -248,11 +247,7 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
 
   /* If Gtk+ is in use eventually gtk_main_iteration will be called,
      unless retval is zero.  */
-#ifdef USE_GTK
   need_to_dispatch = retval == 0;
-#else
-  need_to_dispatch = true;
-#endif
 
   /* xwidgets make heavy use of GLib subprocesses, which add their own
      SIGCHLD handler at arbitrary locations.  That doesn't play well
-- 
2.35.1

bug#56487: xgselect race condition leading to abort when USE_GTK not defined

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Tom Gillespie <tgbugs@gmail.com> writes:

> -#ifdef USE_GTK
>    bool already_has_events;
> -#endif
>  
>    if (xg_select_suppress_count)
>      return pselect (fds_lim, rfds, wfds, efds, timeout, sigmask);
> @@ -126,8 +132,8 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
>    context = g_main_context_default ();
>    acquire_select_lock (context);
>  
> -#ifdef USE_GTK
>    already_has_events = g_main_context_pending (context);
> +#ifdef USE_GTK
>  #ifndef HAVE_PGTK
>    already_has_events = already_has_events && x_gtk_use_native_input;
>  #endif
> @@ -179,12 +185,6 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
>  	tmop = &tmo;
>      }
>  
> -#ifndef USE_GTK
> -  fds_lim = max_fds + 1;
> -  nfds = thread_select (pselect, fds_lim,
> -			&all_rfds, have_wfds ? &all_wfds : NULL, efds,
> -			tmop, sigmask);
> -#else
>    /* On PGTK, when you type a key, the key press event are received,
>       and one more key press event seems to be received internally.
>  
> @@ -217,7 +217,6 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
>  	FD_ZERO (efds);
>        our_fds++;
>      }
> -#endif
>  
>    if (nfds < 0)
>      retval = nfds;
> @@ -248,11 +247,7 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
>  
>    /* If Gtk+ is in use eventually gtk_main_iteration will be called,
>       unless retval is zero.  */
> -#ifdef USE_GTK
>    need_to_dispatch = retval == 0;
> -#else
> -  need_to_dispatch = true;
> -#endif
>  
>    /* xwidgets make heavy use of GLib subprocesses, which add their own
>       SIGCHLD handler at arbitrary locations.  That doesn't play well

Thanks.  Why did the code previously under !USE_GTK have to be removed?

bug#56487: xgselect race condition leading to abort when USE_GTK not defined

[Tom Gillespie]

> Thanks.  Why did the code previously under !USE_GTK have to be removed?

When the !USE_GTK code is used an abort in glib will happen
stochastically due to an out-of-sync call to release_select_lock
in thread.c. This happens on my system somewhere between
approximately 1 in 10 and 1 in 10000 times that the test file
is run.

As far as I can tell from testing there is no difference in behavior
between the USE_GTK and !USE_GTK code. Also, as far as
I can tell from reading, the behavior should be almost identical.
The only addition is to check for already_has_events before
calling thread_select, which may be enough to shift the timing
to prevent a race.

I have not been able to figure out what the actual underlying
cause is (I tried). All I can say for sure is that there is
something that calls into g_main_context_release and
context->owner_count has a negative overflow to 4294967295.

I do not think that it is because something somehow sneaks
in between the calls to the atomics in acquire_select_lock
and relese_select_lock. If you would like I can send along
a couple of patches that include changes I made to try to
see what is going on.

The real underlying issue would seem to be that there is a
missing lock somewhere and that the use of atomics is not
sufficient, but I could be wrong about that.

bug#56487: xgselect race condition leading to abort when USE_GTK not defined

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Tom Gillespie <tgbugs@gmail.com> writes:

>> Thanks.  Why did the code previously under !USE_GTK have to be removed?
>
> When the !USE_GTK code is used an abort in glib will happen
> stochastically due to an out-of-sync call to release_select_lock
> in thread.c. This happens on my system somewhere between
> approximately 1 in 10 and 1 in 10000 times that the test file
> is run.
>
> As far as I can tell from testing there is no difference in behavior
> between the USE_GTK and !USE_GTK code. Also, as far as
> I can tell from reading, the behavior should be almost identical.
> The only addition is to check for already_has_events before
> calling thread_select, which may be enough to shift the timing
> to prevent a race.
>
> I have not been able to figure out what the actual underlying
> cause is (I tried). All I can say for sure is that there is
> something that calls into g_main_context_release and
> context->owner_count has a negative overflow to 4294967295.
>
> I do not think that it is because something somehow sneaks
> in between the calls to the atomics in acquire_select_lock
> and relese_select_lock. If you would like I can send along
> a couple of patches that include changes I made to try to
> see what is going on.
>
> The real underlying issue would seem to be that there is a
> missing lock somewhere and that the use of atomics is not
> sufficient, but I could be wrong about that.

So I suggest that someone find that problem instead.  Any attempt to
"fix" a race condition by moving code around so that the timings are
slightly different is simply deluding oneself.

Thanks.

bug#56487: xgselect race condition leading to abort when USE_GTK not defined

[Tom Gillespie]

> So I suggest that someone find that problem instead.  Any attempt to
> "fix" a race condition by moving code around so that the timings are
> slightly different is simply deluding oneself.

I agree. However that means that the current GTK implementation is
itself fundamentally broken/inadequate. Bringing the other toolkits up
to its level of brokenness seems reasonable in this case?

bug#56487: xgselect race condition leading to abort when USE_GTK not defined

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Tom Gillespie <tgbugs@gmail.com> writes:

> I agree. However that means that the current GTK implementation is
> itself fundamentally broken/inadequate. Bringing the other toolkits up
> to its level of brokenness seems reasonable in this case?

No, because we don't want the different GLib event dispatch behavior
outside of GTK with native input enabled.

bug#56487: xgselect race condition leading to abort when USE_GTK not defined

[Tom Gillespie]

> No, because we don't want the different GLib event dispatch behavior
> outside of GTK with native input enabled.

Ok, got it. I'll see if I can figure out an alternate solution with that
constraint in mind.

Should I split out the release_select_lock change into a separate
patch so that we can get that merged independent of a fix for the
abort behavior?

bug#56487: xgselect race condition leading to abort when USE_GTK not defined

[Eli Zaretskii]

> Cc: 56487@debbugs.gnu.org
> From: Tom Gillespie <tgbugs@gmail.com>
> Date: Mon, 11 Jul 2022 19:20:02 -0700
> 
> > No, because we don't want the different GLib event dispatch behavior
> > outside of GTK with native input enabled.
> 
> Ok, got it. I'll see if I can figure out an alternate solution with that
> constraint in mind.

TIA.

> Should I split out the release_select_lock change into a separate
> patch so that we can get that merged independent of a fix for the
> abort behavior?

Yes, I think that's a good idea.

bug#56487: xgselect race condition leading to abort when USE_GTK not defined

[Tom Gillespie]

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

For the record, while I was hunting a better repro for
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=56488
I had the following abort happen when built with gtk3,
so it seems that there are other conditions that can
trigger the abort in the gtk3 code path as well. The
sequence of events is attached.

[-- Attachment #2: for-the-record.txt --]
[-- Type: text/plain, Size: 3573 bytes --]

tom@host ~ $ ~/git/NOFORK/emacs/src/emacs -Q -batch -l ~/ni/sparc/prrequaestor.el -- --user-emacs-directory /tmp/lol2 --build-dir /tmp/test-build-dir --user-init-file ~/.emacs.d/prm-init.el --fun nif-scr --debug --specs ~/ni/sparc/sync-specs.el 
Loading /home/tom/.emacs.d/prm-init.el (source)...
../../tmp/lol2/elpa/bind-key-20210210.1609/bind-key.el: Warning: Use keywords rather than deprecated positional arguments to `define-minor-mode'
Loading /home/tom/ni/sparc/sync-specs.el (source)...
we get here
Pulling SciCrunch/NIF-Ontology...done
Storing SciCrunch/NIF-Ontology...
Storing SciCrunch/NIF-Ontology...done
existing-pull-requests: ((automated-sct-2022-07-10T2324Z))
stash and pull ...
running command ...
    (lambda nil
      (message "we get here too")
      (ow-run-command
       (python-impl)
       "-m" "nifstd_tools.scr_sync" "--git-local" prm-build-dir "--host" "localhost" "--port" "33060"))
    
we get here too
**
GLib:ERROR:../glib-2.72.3/glib/gmain.c:3489:g_main_context_acquire: assertion failed: (context->owner_count == 0)
Bail out! GLib:ERROR:../glib-2.72.3/glib/gmain.c:3489:g_main_context_acquire: assertion failed: (context->owner_count == 0)
Fatal error 6: Aborted
Backtrace:
/home/tom/git/NOFORK/emacs/src/emacs(+0x19a675)[0x563fa91a3675]
/home/tom/git/NOFORK/emacs/src/emacs(+0x19a749)[0x563fa91a3749]
/lib64/libc.so.6(+0x3d8e0)[0x7f5f87e658e0]
/lib64/libc.so.6(+0x8927c)[0x7f5f87eb127c]
/lib64/libc.so.6(raise+0x12)[0x7f5f87e65842]
/lib64/libc.so.6(abort+0xcf)[0x7f5f87e5046b]
/usr/lib64/libglib-2.0.so.0(+0x1dddc)[0x7f5f895e6ddc]
/usr/lib64/libglib-2.0.so.0(g_assertion_message_expr+0x4b)[0x7f5f8964610b]
/usr/lib64/libglib-2.0.so.0(+0x52e86)[0x7f5f8961be86]
/home/tom/git/NOFORK/emacs/src/emacs(+0x2a7d80)[0x563fa92b0d80]
/home/tom/git/NOFORK/emacs/src/emacs(+0x25d6b4)[0x563fa92666b4]
/home/tom/git/NOFORK/emacs/src/emacs(+0x25f684)[0x563fa9268684]
/home/tom/git/NOFORK/emacs/src/emacs(+0x20ba2b)[0x563fa9214a2b]
/home/tom/git/NOFORK/emacs/src/emacs(+0x20c4de)[0x563fa92154de]
/home/tom/git/NOFORK/emacs/src/emacs(+0x20b962)[0x563fa9214962]
/home/tom/git/NOFORK/emacs/src/emacs(+0x20d76d)[0x563fa921676d]
/home/tom/git/NOFORK/emacs/src/emacs(+0x20b962)[0x563fa9214962]
/home/tom/git/NOFORK/emacs/src/emacs(+0x20c9d5)[0x563fa92159d5]
/home/tom/git/NOFORK/emacs/src/emacs(+0x20ce07)[0x563fa9215e07]
/home/tom/git/NOFORK/emacs/src/emacs(+0x20b866)[0x563fa9214866]
/home/tom/git/NOFORK/emacs/src/emacs(+0x20c9d5)[0x563fa92159d5]
/home/tom/git/NOFORK/emacs/src/emacs(+0x207b33)[0x563fa9210b33]
/home/tom/git/NOFORK/emacs/src/emacs(+0x285116)[0x563fa928e116]
/home/tom/git/NOFORK/emacs/src/emacs(+0x2063a7)[0x563fa920f3a7]
/home/tom/git/NOFORK/emacs/src/emacs(+0x28582f)[0x563fa928e82f]
/lib64/libc.so.6(+0x875a7)[0x7f5f87eaf5a7]
/lib64/libc.so.6(+0x10942c)[0x7f5f87f3142c]
Aborted
tom@host ~ $ ~/git/NOFORK/emacs/src/emacs -Q -batch -l ~/ni/sparc/prrequaestor.el -- --user-emacs-directory /tmp/lol2 --build-dir /tmp/test-build-dir --user-init-file ~/.emacs.d/prm-init.el --fun nif-scr --debug --specs ~/ni/sparc/sync-specs.el ^C
tom@host ~ $ ~/git/NOFORK/emacs/src/emacs -Q -batch -eval '(message "%S" system-configuration-features)'
"ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG JSON LCMS2 LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS WEBP X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB"
tom@host ~ $ pushd git/NOFORK/emacs
~/git/NOFORK/emacs ~
tom@host ~/git/NOFORK/emacs $ git rev-parse HEAD
2e0c76e397811837fd119c1298a5606b4ef65be5
tom@host ~/git/NOFORK/emacs $ 

bug#56487: xgselect race condition leading to abort when USE_GTK not defined

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 56487@debbugs.gnu.org
>> From: Tom Gillespie <tgbugs@gmail.com>
>> Date: Mon, 11 Jul 2022 19:20:02 -0700
>>
>> > No, because we don't want the different GLib event dispatch behavior
>> > outside of GTK with native input enabled.
>>
>> Ok, got it. I'll see if I can figure out an alternate solution with that
>> constraint in mind.
>
> TIA.
>
>> Should I split out the release_select_lock change into a separate
>> patch so that we can get that merged independent of a fix for the
>> abort behavior?
>
> Yes, I think that's a good idea.

(That was one year ago.)

Tom, did you get any further here?  For example, the second part (a
separate patch for release_select_lock) sounds useful independently of
the rest.

Thanks in advance.