From b3a7c5715b199c9ce935a901d2eb02f238af0769 Mon Sep 17 00:00:00 2001 From: Tom Gillespie 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