[3.8] bpo-43406: Fix possible race condition where PyErr_CheckSignals tries to execute a non-Python signal handler (GH-24756) (GH-24762)

We can receive signals (at the C level, in `trip_signal()` in signalmodule.c) while `signal.signal` is being called to modify the corresponding handler.  Later when `PyErr_CheckSignals()` is called to handle the given signal, the handler may be a non-callable object and would raise a cryptic asynchronous exception..
(cherry picked from commit 68245b7)

Co-authored-by: Antoine Pitrou <[email protected]>

Author: pitrou

Lib/test/test_signal.py

@@ -6,6 +6,7 @@
 import statistics
 import subprocess
 import sys
+import threading
 import time
 import unittest
 from test import support
@@ -1243,6 +1244,55 @@ def handler(signum, frame):
         # Python handler
         self.assertEqual(len(sigs), N, "Some signals were lost")
 
+    @unittest.skipUnless(hasattr(signal, "SIGUSR1"),
+                         "test needs SIGUSR1")
+    def test_stress_modifying_handlers(self):
+        # bpo-43406: race condition between trip_signal() and signal.signal
+        signum = signal.SIGUSR1
+        num_sent_signals = 0
+        num_received_signals = 0
+        do_stop = False
+
+        def custom_handler(signum, frame):
+            nonlocal num_received_signals
+            num_received_signals += 1
+
+        def set_interrupts():
+            nonlocal num_sent_signals
+            while not do_stop:
+                signal.raise_signal(signum)
+                num_sent_signals += 1
+
+        def cycle_handlers():
+            while num_sent_signals < 100:
+                for i in range(20000):
+                    # Cycle between a Python-defined and a non-Python handler
+                    for handler in [custom_handler, signal.SIG_IGN]:
+                        signal.signal(signum, handler)
+
+        old_handler = signal.signal(signum, custom_handler)
+        self.addCleanup(signal.signal, signum, old_handler)
+        t = threading.Thread(target=set_interrupts)
+        t.start()
+        try:
+            with support.catch_unraisable_exception() as cm:
+                cycle_handlers()
+                if cm.unraisable is not None:
+                    # An unraisable exception may be printed out when
+                    # a signal is ignored due to the aforementioned
+                    # race condition, check it.
+                    self.assertIsInstance(cm.unraisable.exc_value, OSError)
+                    self.assertIn(
+                        f"Signal {signum} ignored due to race condition",
+                        str(cm.unraisable.exc_value))
+            # Sanity check that some signals were received, but not all
+            self.assertGreater(num_received_signals, 0)
+            self.assertLess(num_received_signals, num_sent_signals)
+        finally:
+            do_stop = True
+            t.join()
+
+
 class RaiseSignalTest(unittest.TestCase):
 
     def test_sigint(self):

Misc/NEWS.d/next/Core and Builtins/2021-03-04-22-53-10.bpo-43406.Na_VpA.rst

@@ -0,0 +1,2 @@
+Fix a possible race condition where ``PyErr_CheckSignals`` tries to execute a
+non-Python signal handler.

Modules/signalmodule.c

@@ -1675,23 +1675,47 @@ _PyErr_CheckSignals(void)
         f = Py_None;
 
     for (i = 1; i < NSIG; i++) {
-        if (_Py_atomic_load_relaxed(&Handlers[i].tripped)) {
-            PyObject *result = NULL;
-            PyObject *arglist = Py_BuildValue("(iO)", i, f);
-            _Py_atomic_store_relaxed(&Handlers[i].tripped, 0);
-
-            if (arglist) {
-                result = PyEval_CallObject(Handlers[i].func,
-                                           arglist);
-                Py_DECREF(arglist);
-            }
-            if (!result) {
-                _Py_atomic_store(&is_tripped, 1);
-                return -1;
-            }
+        if (!_Py_atomic_load_relaxed(&Handlers[i].tripped)) {
+            continue;
+        }
+        _Py_atomic_store_relaxed(&Handlers[i].tripped, 0);
 
-            Py_DECREF(result);
+        /* Signal handlers can be modified while a signal is received,
+         * and therefore the fact that trip_signal() or PyErr_SetInterrupt()
+         * was called doesn't guarantee that there is still a Python
+         * signal handler for it by the time PyErr_CheckSignals() is called
+         * (see bpo-43406).
+         */
+        PyObject *func = Handlers[i].func;
+        if (func == NULL || func == Py_None || func == IgnoreHandler ||
+            func == DefaultHandler) {
+            /* No Python signal handler due to aforementioned race condition.
+             * We can't call raise() as it would break the assumption
+             * that PyErr_SetInterrupt() only *simulates* an incoming
+             * signal (i.e. it will never kill the process).
+             * We also don't want to interrupt user code with a cryptic
+             * asynchronous exception, so instead just write out an
+             * unraisable error.
+             */
+            PyErr_Format(PyExc_OSError,
+                         "Signal %i ignored due to race condition",
+                         i);
+            PyErr_WriteUnraisable(Py_None);
+            continue;
+        }
+
+        PyObject *result = NULL;
+        PyObject *arglist = Py_BuildValue("(iO)", i, f);
+        if (arglist) {
+            result = PyEval_CallObject(func, arglist);
+            Py_DECREF(arglist);
         }
+        if (!result) {
+            /* On error, re-schedule a call to PyErr_CheckSignals() */
+            _Py_atomic_store(&is_tripped, 1);
+            return -1;
+        }
+        Py_DECREF(result);
     }
 
     return 0;