From 0a68ec4b3f60f82b712a499ffa3b31394acce866 Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Mon, 26 Jun 2017 19:16:49 +0200 Subject: [PATCH 1/5] Improve signal delivery Avoid using Py_AddPendingCall from signal handler, to avoid calling signal-unsafe functions. --- Include/ceval.h | 1 + Modules/signalmodule.c | 14 +++++++------- Python/ceval.c | 29 ++++++++++++++++++++++++----- 3 files changed, 32 insertions(+), 12 deletions(-) diff --git a/Include/ceval.h b/Include/ceval.h index e5cb4112274794..b2d57cbd6f7425 100644 --- a/Include/ceval.h +++ b/Include/ceval.h @@ -54,6 +54,7 @@ PyAPI_FUNC(int) PyEval_MergeCompilerFlags(PyCompilerFlags *cf); #endif PyAPI_FUNC(int) Py_AddPendingCall(int (*func)(void *), void *arg); +PyAPI_FUNC(void) _PyEval_SignalReceived(void); PyAPI_FUNC(int) Py_MakePendingCalls(void); /* Protection against deeply nested recursive calls diff --git a/Modules/signalmodule.c b/Modules/signalmodule.c index 75abc98bc4b593..34af6555764c49 100644 --- a/Modules/signalmodule.c +++ b/Modules/signalmodule.c @@ -244,12 +244,10 @@ trip_signal(int sig_num) Handlers[sig_num].tripped = 1; - if (!is_tripped) { - /* Set is_tripped after setting .tripped, as it gets - cleared in PyErr_CheckSignals() before .tripped. */ - is_tripped = 1; - Py_AddPendingCall(checksignals_witharg, NULL); - } + /* Set is_tripped after setting .tripped, as it gets + cleared in PyErr_CheckSignals() before .tripped. */ + is_tripped = 1; + _PyEval_SignalReceived(); /* And then write to the wakeup fd *after* setting all the globals and doing the Py_AddPendingCall. We used to write to the wakeup fd and then @@ -1556,8 +1554,10 @@ PyErr_CheckSignals(void) arglist); Py_DECREF(arglist); } - if (!result) + if (!result) { + is_tripped = 1; return -1; + } Py_DECREF(result); } diff --git a/Python/ceval.c b/Python/ceval.c index 3243a4f8a01d9c..6fbc8ebe8391c8 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -387,6 +387,15 @@ Py_AddPendingCall(int (*func)(void *), void *arg) return result; } +void +_PyEval_SignalReceived(void) +{ + /* bpo-30703: Function called when the C signal handler of Python gets a + signal. We cannot queue a callback using Py_AddPendingCall() since this + function is not reentrant (use a lock and a list). */ + SIGNAL_PENDING_CALLS(); +} + int Py_MakePendingCalls(void) { @@ -408,6 +417,16 @@ Py_MakePendingCalls(void) if (busy) return 0; busy = 1; + + /* Python signal handler doesn't really queue a callback: it only signals + that an UNIX signal was received, see _PyEval_SignalReceived(). */ + UNSIGNAL_PENDING_CALLS(); + if (PyErr_CheckSignals() < 0) { + SIGNAL_PENDING_CALLS(); + r = -1; + goto end; + } + /* perform a bounded number of calls, in case of recursion */ for (i=0; i Date: Mon, 26 Jun 2017 20:00:48 +0200 Subject: [PATCH 2/5] Remove unused function --- Modules/signalmodule.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/Modules/signalmodule.c b/Modules/signalmodule.c index 34af6555764c49..7a2f985d5e5734 100644 --- a/Modules/signalmodule.c +++ b/Modules/signalmodule.c @@ -188,12 +188,6 @@ The default handler for SIGINT installed by Python.\n\ It raises KeyboardInterrupt."); -static int -checksignals_witharg(void * unused) -{ - return PyErr_CheckSignals(); -} - static int report_wakeup_write_error(void *data) { From b577db5880e314707807ec442a158e82a4cdb09d Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Mon, 26 Jun 2017 20:22:41 +0200 Subject: [PATCH 3/5] Improve comments --- Modules/signalmodule.c | 10 +++++++--- Python/ceval.c | 4 ++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/Modules/signalmodule.c b/Modules/signalmodule.c index 7a2f985d5e5734..cbf0d543051e95 100644 --- a/Modules/signalmodule.c +++ b/Modules/signalmodule.c @@ -244,9 +244,9 @@ trip_signal(int sig_num) _PyEval_SignalReceived(); /* And then write to the wakeup fd *after* setting all the globals and - doing the Py_AddPendingCall. We used to write to the wakeup fd and then - set the flag, but this allowed the following sequence of events - (especially on windows, where trip_signal runs in a new thread): + doing the _PyEval_SignalReceived. We used to write to the wakeup fd + and then set the flag, but this allowed the following sequence of events + (especially on windows, where trip_signal may run in a new thread): - main thread blocks on select([wakeup_fd], ...) - signal arrives @@ -281,6 +281,8 @@ trip_signal(int sig_num) wakeup.send_err_set = 1; wakeup.send_errno = errno; wakeup.send_win_error = GetLastError(); + /* Py_AddPendingCall() isn't signal-safe, but we + still use it for this exceptional case. */ Py_AddPendingCall(report_wakeup_send_error, NULL); } } @@ -294,6 +296,8 @@ trip_signal(int sig_num) rc = _Py_write_noraise(fd, &byte, 1); if (rc < 0) { + /* Py_AddPendingCall() isn't signal-safe, but we + still use it for this exceptional case. */ Py_AddPendingCall(report_wakeup_write_error, (void *)(intptr_t)errno); } diff --git a/Python/ceval.c b/Python/ceval.c index 6fbc8ebe8391c8..3958bb3aa40d62 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -392,7 +392,7 @@ _PyEval_SignalReceived(void) { /* bpo-30703: Function called when the C signal handler of Python gets a signal. We cannot queue a callback using Py_AddPendingCall() since this - function is not reentrant (use a lock and a list). */ + function is not reentrant (uses a lock and a list). */ SIGNAL_PENDING_CALLS(); } @@ -419,7 +419,7 @@ Py_MakePendingCalls(void) busy = 1; /* Python signal handler doesn't really queue a callback: it only signals - that an UNIX signal was received, see _PyEval_SignalReceived(). */ + that a signal was received, see _PyEval_SignalReceived(). */ UNSIGNAL_PENDING_CALLS(); if (PyErr_CheckSignals() < 0) { SIGNAL_PENDING_CALLS(); From 1b6b61db5bdb86a0548279a8565912f85569da2d Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Mon, 26 Jun 2017 20:30:27 +0200 Subject: [PATCH 4/5] Use _Py_atomic API for concurrency-sensitive signal state --- Modules/signalmodule.c | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/Modules/signalmodule.c b/Modules/signalmodule.c index cbf0d543051e95..19ffff7b653d87 100644 --- a/Modules/signalmodule.c +++ b/Modules/signalmodule.c @@ -93,7 +93,7 @@ static pid_t main_pid; #endif static volatile struct { - sig_atomic_t tripped; + _Py_atomic_int tripped; PyObject *func; } Handlers[NSIG]; @@ -113,7 +113,7 @@ static volatile sig_atomic_t wakeup_fd = -1; #endif /* Speed up sigcheck() when none tripped */ -static volatile sig_atomic_t is_tripped = 0; +static _Py_atomic_int is_tripped; static PyObject *DefaultHandler; static PyObject *IgnoreHandler; @@ -236,11 +236,13 @@ trip_signal(int sig_num) int fd; Py_ssize_t rc; - Handlers[sig_num].tripped = 1; + _Py_atomic_store_relaxed(&Handlers[sig_num].tripped, 1); /* Set is_tripped after setting .tripped, as it gets cleared in PyErr_CheckSignals() before .tripped. */ - is_tripped = 1; + _Py_atomic_store(&is_tripped, 1); + + /* Notify ceval.c */ _PyEval_SignalReceived(); /* And then write to the wakeup fd *after* setting all the globals and @@ -461,7 +463,7 @@ signal_signal_impl(PyObject *module, int signalnum, PyObject *handler) return NULL; } old_handler = Handlers[signalnum].func; - Handlers[signalnum].tripped = 0; + _Py_atomic_store_relaxed(&Handlers[signalnum].tripped, 0); Py_INCREF(handler); Handlers[signalnum].func = handler; if (old_handler != NULL) @@ -1265,11 +1267,11 @@ PyInit__signal(void) goto finally; Py_INCREF(IntHandler); - Handlers[0].tripped = 0; + _Py_atomic_store_relaxed(&Handlers[0].tripped, 0); for (i = 1; i < NSIG; i++) { void (*t)(int); t = PyOS_getsig(i); - Handlers[i].tripped = 0; + _Py_atomic_store_relaxed(&Handlers[i].tripped, 0); if (t == SIG_DFL) Handlers[i].func = DefaultHandler; else if (t == SIG_IGN) @@ -1493,7 +1495,7 @@ finisignal(void) for (i = 1; i < NSIG; i++) { func = Handlers[i].func; - Handlers[i].tripped = 0; + _Py_atomic_store_relaxed(&Handlers[i].tripped, 0); Handlers[i].func = NULL; if (i != SIGINT && func != NULL && func != Py_None && func != DefaultHandler && func != IgnoreHandler) @@ -1514,7 +1516,7 @@ PyErr_CheckSignals(void) int i; PyObject *f; - if (!is_tripped) + if (!_Py_atomic_load(&is_tripped)) return 0; #ifdef WITH_THREAD @@ -1536,16 +1538,16 @@ PyErr_CheckSignals(void) * we receive a signal i after we zero is_tripped and before we * check Handlers[i].tripped. */ - is_tripped = 0; + _Py_atomic_store(&is_tripped, 0); if (!(f = (PyObject *)PyEval_GetFrame())) f = Py_None; for (i = 1; i < NSIG; i++) { - if (Handlers[i].tripped) { + if (_Py_atomic_load_relaxed(&Handlers[i].tripped)) { PyObject *result = NULL; PyObject *arglist = Py_BuildValue("(iO)", i, f); - Handlers[i].tripped = 0; + _Py_atomic_store_relaxed(&Handlers[i].tripped, 0); if (arglist) { result = PyEval_CallObject(Handlers[i].func, @@ -1553,7 +1555,7 @@ PyErr_CheckSignals(void) Py_DECREF(arglist); } if (!result) { - is_tripped = 1; + _Py_atomic_store(&is_tripped, 1); return -1; } @@ -1592,12 +1594,12 @@ PyOS_FiniInterrupts(void) int PyOS_InterruptOccurred(void) { - if (Handlers[SIGINT].tripped) { + if (_Py_atomic_load_relaxed(&Handlers[SIGINT].tripped)) { #ifdef WITH_THREAD if (PyThread_get_thread_ident() != main_thread) return 0; #endif - Handlers[SIGINT].tripped = 0; + _Py_atomic_store_relaxed(&Handlers[SIGINT].tripped, 0); return 1; } return 0; @@ -1607,11 +1609,11 @@ static void _clear_pending_signals(void) { int i; - if (!is_tripped) + if (!_Py_atomic_load(&is_tripped)) return; - is_tripped = 0; + _Py_atomic_store(&is_tripped, 0); for (i = 1; i < NSIG; ++i) { - Handlers[i].tripped = 0; + _Py_atomic_store_relaxed(&Handlers[i].tripped, 0); } } From 0144f9ceb308ab4bfefda2d7150b8550aba28d1f Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Mon, 17 Jul 2017 12:13:14 +0200 Subject: [PATCH 5/5] Add blurb --- .../Core and Builtins/2017-07-17-12-12-59.bpo-30808.bA3zOv.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2017-07-17-12-12-59.bpo-30808.bA3zOv.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2017-07-17-12-12-59.bpo-30808.bA3zOv.rst b/Misc/NEWS.d/next/Core and Builtins/2017-07-17-12-12-59.bpo-30808.bA3zOv.rst new file mode 100644 index 00000000000000..8adbbe7dc3a07e --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2017-07-17-12-12-59.bpo-30808.bA3zOv.rst @@ -0,0 +1 @@ +Use _Py_atomic API for concurrency-sensitive signal state.