diff --git a/Lib/test/test_signal.py b/Lib/test/test_signal.py index 6a43fe70372c55..c9de4a4e015a39 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 00000000000000..c18a55ef323069 --- /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 7ac797a3aa3f87..c6564be9c6aabf 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 {