[Python-checkins] [3.8] bpo-43406: Fix possible race condition where ``PyErr_CheckSignals`` tries to execute a non-Python signal handler (GH-24756) (GH-24762)
miss-islington
webhook-mailer at python.org
Sat Mar 6 10:08:05 EST 2021
https://github.com/python/cpython/commit/4715be8a4384159e47fb09e5f3bd077734842655
commit: 4715be8a4384159e47fb09e5f3bd077734842655
branch: 3.8
author: Antoine Pitrou <antoine at python.org>
committer: miss-islington <31488909+miss-islington at users.noreply.github.com>
date: 2021-03-06T07:08:01-08:00
summary:
[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 68245b7a1030287294c65c298975ab9026543fd2)
Co-authored-by: Antoine Pitrou <antoine at python.org>
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 d41e94b07f430..865e0102637c2 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
@@ -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):
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 c63ede97b5ab9..4b855d6f3078d 100644
--- a/Modules/signalmodule.c
+++ b/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;
More information about the Python-checkins
mailing list