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

[pitrou]

https://github.com/python/cpython/commit/68245b7a1030287294c65c298975ab9026543fd2
commit: 68245b7a1030287294c65c298975ab9026543fd2
branch: master
author: Antoine Pitrou <antoine at python.org>
committer: pitrou <pitrou at free.fr>
date: 2021-03-05T10:32:50+01:00
summary:

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

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.

files:
A Misc/NEWS.d/next/Core and Builtins/2021-03-04-22-53-10.bpo-43406.Na_VpA.rst
M Lib/test/test_signal.py
M Modules/signalmodule.c

diff --git a/Lib/test/test_signal.py b/Lib/test/test_signal.py
index 6a43fe70372c5..c9de4a4e015a3 100644
--- a/Lib/test/test_signal.py
+++ b/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
@@ -1251,6 +1252,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):
diff --git a/Misc/NEWS.d/next/Core and Builtins/2021-03-04-22-53-10.bpo-43406.Na_VpA.rst b/Misc/NEWS.d/next/Core and Builtins/2021-03-04-22-53-10.bpo-43406.Na_VpA.rst
new file mode 100644
index 0000000000000..c18a55ef32306
--- /dev/null
+++ b/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.
diff --git a/Modules/signalmodule.c b/Modules/signalmodule.c
index 7ac797a3aa3f8..c6564be9c6aab 100644
--- a/Modules/signalmodule.c
+++ b/Modules/signalmodule.c
@@ -1706,10 +1706,34 @@ _PyErr_CheckSignalsTstate(PyThreadState *tstate)
         }
         _Py_atomic_store_relaxed(&Handlers[i].tripped, 0);
 
+        /* 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 *arglist = Py_BuildValue("(iO)", i, frame);
         PyObject *result;
         if (arglist) {
-            result = _PyObject_Call(tstate, Handlers[i].func, arglist, NULL);
+            result = _PyObject_Call(tstate, func, arglist, NULL);
             Py_DECREF(arglist);
         }
         else {