From 35513538439c7babc27bcc5e2fbc68cf55ba67a3 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 24 Oct 2017 11:57:59 +0200 Subject: [PATCH 1/6] bpo-30768: Recompute timeout on interrupted lock Fix the pthread+semaphore implementation of PyThread_acquire_lock_timed() when called with timeout > 0 and intr_flag=0: recompute the timeout if sem_timedwait() is interrupted by a signal (EINTR). See also the PEP 475. --- .../2017-10-24-12-00-16.bpo-30768.Om8Yj_.rst | 3 ++ Python/thread_pthread.h | 49 ++++++++++++++++--- 2 files changed, 46 insertions(+), 6 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2017-10-24-12-00-16.bpo-30768.Om8Yj_.rst diff --git a/Misc/NEWS.d/next/Library/2017-10-24-12-00-16.bpo-30768.Om8Yj_.rst b/Misc/NEWS.d/next/Library/2017-10-24-12-00-16.bpo-30768.Om8Yj_.rst new file mode 100644 index 00000000000000..77bff97a7ee3d8 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2017-10-24-12-00-16.bpo-30768.Om8Yj_.rst @@ -0,0 +1,3 @@ +Fix the pthread+semaphore implementation of PyThread_acquire_lock_timed() when +called with timeout > 0 and intr_flag=0: recompute the timeout if +sem_timedwait() is interrupted by a signal (EINTR). See also the :pep:`475`. diff --git a/Python/thread_pthread.h b/Python/thread_pthread.h index c5b7f3256faf96..0de02d63648477 100644 --- a/Python/thread_pthread.h +++ b/Python/thread_pthread.h @@ -318,23 +318,59 @@ PyThread_acquire_lock_timed(PyThread_type_lock lock, PY_TIMEOUT_T microseconds, sem_t *thelock = (sem_t *)lock; int status, error = 0; struct timespec ts; + _PyTime_t deadline = 0; (void) error; /* silence unused-but-set-variable warning */ dprintf(("PyThread_acquire_lock_timed(%p, %lld, %d) called\n", lock, microseconds, intr_flag)); - if (microseconds > 0) + if (microseconds > 0) { MICROSECONDS_TO_TIMESPEC(microseconds, ts); - do { - if (microseconds > 0) + + if (!intr_flag) { + _PyTime_t timeout = _PyTime_FromNanoseconds(microseconds * 1000); + deadline = _PyTime_GetMonotonicClock() + timeout; + } + } + + while (1) { + if (microseconds > 0) { status = fix_status(sem_timedwait(thelock, &ts)); - else if (microseconds == 0) + } + else if (microseconds == 0) { status = fix_status(sem_trywait(thelock)); - else + } + else { status = fix_status(sem_wait(thelock)); + } + /* Retry if interrupted by a signal, unless the caller wants to be notified. */ - } while (!intr_flag && status == EINTR); + if (intr_flag || status != EINTR) { + break; + } + + if (microseconds > 0) { + /* wait interrupted by a signal (EINTR): recompute the timeout */ + _PyTime_t dt = deadline - _PyTime_GetMonotonicClock(); + if (dt < 0) { + status = ETIMEDOUT; + break; + } + else if (dt > 0) { + _PyTime_t realtime_deadline = _PyTime_GetSystemClock() + dt; + if (_PyTime_AsTimespec(realtime_deadline, &ts) < 0) { + success = PY_LOCK_FAILURE; + goto exit; + } + /* no need to update microseconds value, the code only care + if (microseconds > 0 or (microseconds == 0). */ + } + else { + microseconds = 0; + } + } + } /* Don't check the status if we're stopping because of an interrupt. */ if (!(intr_flag && status == EINTR)) { @@ -359,6 +395,7 @@ PyThread_acquire_lock_timed(PyThread_type_lock lock, PY_TIMEOUT_T microseconds, success = PY_LOCK_FAILURE; } +exit: dprintf(("PyThread_acquire_lock_timed(%p, %lld, %d) -> %d\n", lock, microseconds, intr_flag, success)); return success; From 764d0cdf5e7c4c06a56246e4d0ca2ad5be3ce7cf Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 24 Oct 2017 17:40:29 +0200 Subject: [PATCH 2/6] Adjust PY_TIMEOUT_MAX to prevent overflow --- Include/pythread.h | 8 +++++++- Modules/_threadmodule.c | 2 +- Python/thread_pthread.h | 3 +++ 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/Include/pythread.h b/Include/pythread.h index d6674685f289ff..2f25b0dba8be83 100644 --- a/Include/pythread.h +++ b/Include/pythread.h @@ -42,7 +42,13 @@ PyAPI_FUNC(int) PyThread_acquire_lock(PyThread_type_lock, int); and floating-point numbers allowed. */ #define PY_TIMEOUT_T long long -#define PY_TIMEOUT_MAX PY_LLONG_MAX +#if defined(_POSIX_THREADS) + /* PyThread_acquire_lock_timed() uses _PyTime_FromNanoseconds(us * 1000), + convert microseconds to nanoseconds. */ +# define PY_TIMEOUT_MAX (PY_LLONG_MAX / 1000) +#else +# define PY_TIMEOUT_MAX PY_LLONG_MAX +#endif /* In the NT API, the timeout is a DWORD and is expressed in milliseconds */ #if defined (NT_THREADS) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 72df78f9ccc577..8a44475998af8e 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1363,7 +1363,7 @@ PyInit__thread(void) if (m == NULL) return NULL; - timeout_max = PY_TIMEOUT_MAX / 1000000; + timeout_max = (double)PY_TIMEOUT_MAX * 1e-6; time_max = floor(_PyTime_AsSecondsDouble(_PyTime_MAX)); timeout_max = Py_MIN(timeout_max, time_max); diff --git a/Python/thread_pthread.h b/Python/thread_pthread.h index 0de02d63648477..836bacdf781237 100644 --- a/Python/thread_pthread.h +++ b/Python/thread_pthread.h @@ -328,6 +328,9 @@ PyThread_acquire_lock_timed(PyThread_type_lock lock, PY_TIMEOUT_T microseconds, MICROSECONDS_TO_TIMESPEC(microseconds, ts); if (!intr_flag) { + /* the caller must ensures that microseconds <= PY_TIMEOUT_MAX + and so microseconds * 1000 cannot overflow. PY_TIMEOUT_MAX + is defined to prevent this specific overflow. */ _PyTime_t timeout = _PyTime_FromNanoseconds(microseconds * 1000); deadline = _PyTime_GetMonotonicClock() + timeout; } From 17aba7c333ec03f87e4f3b4e721aef2e4647dcd7 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 24 Oct 2017 18:08:43 +0200 Subject: [PATCH 3/6] Fix _thread.TIMEOUT_MAX rounding on Windows --- Modules/_threadmodule.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 8a44475998af8e..99611eebcb7445 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1364,8 +1364,10 @@ PyInit__thread(void) return NULL; timeout_max = (double)PY_TIMEOUT_MAX * 1e-6; - time_max = floor(_PyTime_AsSecondsDouble(_PyTime_MAX)); + time_max = _PyTime_AsSecondsDouble(_PyTime_MAX); timeout_max = Py_MIN(timeout_max, time_max); + /* Round towards minus infinity */ + timeout_max = floor(timeout_max); v = PyFloat_FromDouble(timeout_max); if (!v) From 7c428b7014ecd9058f06d30ccd4deb93a6c28c0e Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 24 Oct 2017 18:49:00 +0200 Subject: [PATCH 4/6] Fatal error if timeout > PY_TIMEOUT_MAX The pthread implementation of PyThread_acquire_lock() now fails with a fatal error if the timeout is larger than PY_TIMEOUT_MAX, as done in the Windows implementation. The check prevents any risk of overflow in PyThread_acquire_lock(). Add also PY_DWORD_MAX constant. --- Include/pyport.h | 3 +++ Include/pythread.h | 15 ++++++++------- Modules/_winapi.c | 8 +++----- Modules/posixmodule.c | 4 +--- Python/thread_nt.h | 9 +++++---- Python/thread_pthread.h | 14 +++++++++----- 6 files changed, 29 insertions(+), 24 deletions(-) diff --git a/Include/pyport.h b/Include/pyport.h index 2742e477fd4e8e..0e82543ac78355 100644 --- a/Include/pyport.h +++ b/Include/pyport.h @@ -787,6 +787,9 @@ extern _invalid_parameter_handler _Py_silent_invalid_parameter_handler; #include #endif +/* Maximum value of the Windows DWORD type */ +#define PY_DWORD_MAX 4294967295U + /* This macro used to tell whether Python was built with multithreading * enabled. Now multithreading is always enabled, but keep the macro * for compatibility. diff --git a/Include/pythread.h b/Include/pythread.h index 2f25b0dba8be83..eb61033b2d9089 100644 --- a/Include/pythread.h +++ b/Include/pythread.h @@ -42,21 +42,22 @@ PyAPI_FUNC(int) PyThread_acquire_lock(PyThread_type_lock, int); and floating-point numbers allowed. */ #define PY_TIMEOUT_T long long + #if defined(_POSIX_THREADS) /* PyThread_acquire_lock_timed() uses _PyTime_FromNanoseconds(us * 1000), convert microseconds to nanoseconds. */ # define PY_TIMEOUT_MAX (PY_LLONG_MAX / 1000) +#elif defined (NT_THREADS) + /* In the NT API, the timeout is a DWORD and is expressed in milliseconds */ +# if 0xFFFFFFFFLL * 1000 < PY_LLONG_MAX +# define PY_TIMEOUT_MAX (0xFFFFFFFFLL * 1000) +# else +# define PY_TIMEOUT_MAX PY_LLONG_MAX +# endif #else # define PY_TIMEOUT_MAX PY_LLONG_MAX #endif -/* In the NT API, the timeout is a DWORD and is expressed in milliseconds */ -#if defined (NT_THREADS) -#if 0xFFFFFFFFLL * 1000 < PY_TIMEOUT_MAX -#undef PY_TIMEOUT_MAX -#define PY_TIMEOUT_MAX (0xFFFFFFFFLL * 1000) -#endif -#endif /* If microseconds == 0, the call is non-blocking: it returns immediately even when the lock can't be acquired. diff --git a/Modules/_winapi.c b/Modules/_winapi.c index 00a26d515e0cd3..118ef6c93f9f11 100644 --- a/Modules/_winapi.c +++ b/Modules/_winapi.c @@ -61,8 +61,6 @@ #define T_HANDLE T_POINTER -#define DWORD_MAX 4294967295U - /* Grab CancelIoEx dynamically from kernel32 */ static int has_CancelIoEx = -1; static BOOL (CALLBACK *Py_CancelIoEx)(HANDLE, LPOVERLAPPED); @@ -184,7 +182,7 @@ class DWORD_return_converter(CReturnConverter): def render(self, function, data): self.declare(data) - self.err_occurred_if("_return_value == DWORD_MAX", data) + self.err_occurred_if("_return_value == PY_DWORD_MAX", data) data.return_conversion.append( 'return_value = Py_BuildValue("k", _return_value);\n') [python start generated code]*/ @@ -1009,7 +1007,7 @@ _winapi_GetExitCodeProcess_impl(PyObject *module, HANDLE process) if (! result) { PyErr_SetFromWindowsErr(GetLastError()); - exit_code = DWORD_MAX; + exit_code = PY_DWORD_MAX; } return exit_code; @@ -1466,7 +1464,7 @@ _winapi_WriteFile_impl(PyObject *module, HANDLE handle, PyObject *buffer, } Py_BEGIN_ALLOW_THREADS - len = (DWORD)Py_MIN(buf->len, DWORD_MAX); + len = (DWORD)Py_MIN(buf->len, PY_DWORD_MAX); ret = WriteFile(handle, buf->buf, len, &written, overlapped ? &overlapped->overlapped : NULL); Py_END_ALLOW_THREADS diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index c7d8b00e6f9a43..661fa1312ad060 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -390,8 +390,6 @@ static int win32_can_symlink = 0; #endif #endif -#define DWORD_MAX 4294967295U - #ifdef MS_WINDOWS #define INITFUNC PyInit_nt #define MODNAME "nt" @@ -3817,7 +3815,7 @@ os__getvolumepathname_impl(PyObject *module, PyObject *path) /* Volume path should be shorter than entire path */ buflen = Py_MAX(buflen, MAX_PATH); - if (buflen > DWORD_MAX) { + if (buflen > PY_DWORD_MAX) { PyErr_SetString(PyExc_OverflowError, "path too long"); return NULL; } diff --git a/Python/thread_nt.h b/Python/thread_nt.h index bae8bcc35669fd..46df3466928391 100644 --- a/Python/thread_nt.h +++ b/Python/thread_nt.h @@ -283,12 +283,13 @@ PyThread_acquire_lock_timed(PyThread_type_lock aLock, milliseconds = microseconds / 1000; if (microseconds % 1000 > 0) ++milliseconds; - if ((DWORD) milliseconds != milliseconds) - Py_FatalError("Timeout too large for a DWORD, " - "please check PY_TIMEOUT_MAX"); + if (milliseconds > PY_DWORD_MAX) { + Py_FatalError("Timeout larger than PY_TIMEOUT_MAX"); + } } - else + else { milliseconds = INFINITE; + } dprintf(("%lu: PyThread_acquire_lock_timed(%p, %lld) called\n", PyThread_get_thread_ident(), aLock, microseconds)); diff --git a/Python/thread_pthread.h b/Python/thread_pthread.h index 836bacdf781237..4bb8ef87fb983e 100644 --- a/Python/thread_pthread.h +++ b/Python/thread_pthread.h @@ -324,13 +324,16 @@ PyThread_acquire_lock_timed(PyThread_type_lock lock, PY_TIMEOUT_T microseconds, dprintf(("PyThread_acquire_lock_timed(%p, %lld, %d) called\n", lock, microseconds, intr_flag)); + if (microseconds > PY_TIMEOUT_MAX) { + Py_FatalError("Timeout larger than PY_TIMEOUT_MAX"); + } + if (microseconds > 0) { MICROSECONDS_TO_TIMESPEC(microseconds, ts); if (!intr_flag) { - /* the caller must ensures that microseconds <= PY_TIMEOUT_MAX - and so microseconds * 1000 cannot overflow. PY_TIMEOUT_MAX - is defined to prevent this specific overflow. */ + /* cannot overflow thanks to (microseconds > PY_TIMEOUT_MAX) + check done above */ _PyTime_t timeout = _PyTime_FromNanoseconds(microseconds * 1000); deadline = _PyTime_GetMonotonicClock() + timeout; } @@ -363,8 +366,9 @@ PyThread_acquire_lock_timed(PyThread_type_lock lock, PY_TIMEOUT_T microseconds, else if (dt > 0) { _PyTime_t realtime_deadline = _PyTime_GetSystemClock() + dt; if (_PyTime_AsTimespec(realtime_deadline, &ts) < 0) { - success = PY_LOCK_FAILURE; - goto exit; + /* Cannot occur thanks to (microseconds > PY_TIMEOUT_MAX) + check done above */ + Py_UNREACHABLE(); } /* no need to update microseconds value, the code only care if (microseconds > 0 or (microseconds == 0). */ From 68e3f02d663ff0846bb8f30c587eef132cc8aa5e Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 25 Oct 2017 01:21:55 +0200 Subject: [PATCH 5/6] Remove now unused "exit" label --- Python/thread_pthread.h | 1 - 1 file changed, 1 deletion(-) diff --git a/Python/thread_pthread.h b/Python/thread_pthread.h index 4bb8ef87fb983e..13cffa3bf3e3f8 100644 --- a/Python/thread_pthread.h +++ b/Python/thread_pthread.h @@ -402,7 +402,6 @@ PyThread_acquire_lock_timed(PyThread_type_lock lock, PY_TIMEOUT_T microseconds, success = PY_LOCK_FAILURE; } -exit: dprintf(("PyThread_acquire_lock_timed(%p, %lld, %d) -> %d\n", lock, microseconds, intr_flag, success)); return success; From 1fb576c77694ae4ae545b2414823c72493cbfa97 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 25 Oct 2017 01:22:10 +0200 Subject: [PATCH 6/6] Update generated code: run "make clinic" --- Modules/_winapi.c | 2 +- Modules/clinic/_winapi.c.h | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Modules/_winapi.c b/Modules/_winapi.c index 118ef6c93f9f11..7e8d4e38464077 100644 --- a/Modules/_winapi.c +++ b/Modules/_winapi.c @@ -186,7 +186,7 @@ class DWORD_return_converter(CReturnConverter): data.return_conversion.append( 'return_value = Py_BuildValue("k", _return_value);\n') [python start generated code]*/ -/*[python end generated code: output=da39a3ee5e6b4b0d input=94819e72d2c6d558]*/ +/*[python end generated code: output=da39a3ee5e6b4b0d input=4527052fe06e5823]*/ #include "clinic/_winapi.c.h" diff --git a/Modules/clinic/_winapi.c.h b/Modules/clinic/_winapi.c.h index 9e1fbe1fdb09af..01bba3607140b6 100644 --- a/Modules/clinic/_winapi.c.h +++ b/Modules/clinic/_winapi.c.h @@ -460,7 +460,7 @@ _winapi_GetExitCodeProcess(PyObject *module, PyObject *arg) goto exit; } _return_value = _winapi_GetExitCodeProcess_impl(module, process); - if ((_return_value == DWORD_MAX) && PyErr_Occurred()) { + if ((_return_value == PY_DWORD_MAX) && PyErr_Occurred()) { goto exit; } return_value = Py_BuildValue("k", _return_value); @@ -487,7 +487,7 @@ _winapi_GetLastError(PyObject *module, PyObject *Py_UNUSED(ignored)) DWORD _return_value; _return_value = _winapi_GetLastError_impl(module); - if ((_return_value == DWORD_MAX) && PyErr_Occurred()) { + if ((_return_value == PY_DWORD_MAX) && PyErr_Occurred()) { goto exit; } return_value = Py_BuildValue("k", _return_value); @@ -889,4 +889,4 @@ _winapi_WriteFile(PyObject *module, PyObject **args, Py_ssize_t nargs, PyObject exit: return return_value; } -/*[clinic end generated code: output=afa6bd61eb0f18d2 input=a9049054013a1b77]*/ +/*[clinic end generated code: output=fba2ad7bf1a87e4a input=a9049054013a1b77]*/