From: Victor Stinner Date: Tue, 24 Oct 2017 23:53:32 +0000 (-0700) Subject: bpo-30768: Recompute timeout on interrupted lock (GH-4103) X-Git-Tag: v3.7.0a3~312 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=850a18e03e8f8309bc8c39adc6e7d51a4568cd9a;p=python bpo-30768: Recompute timeout on interrupted lock (GH-4103) 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. 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. --- diff --git a/Include/pyport.h b/Include/pyport.h index 2742e477fd..0e82543ac7 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 d6674685f2..eb61033b2d 100644 --- a/Include/pythread.h +++ b/Include/pythread.h @@ -42,16 +42,23 @@ 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 -/* 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 +#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 + /* If microseconds == 0, the call is non-blocking: it returns immediately even when the lock can't be acquired. If microseconds > 0, the call waits up to the specified duration. 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 0000000000..77bff97a7e --- /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/Modules/_threadmodule.c b/Modules/_threadmodule.c index 72df78f9cc..99611eebcb 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1363,9 +1363,11 @@ PyInit__thread(void) if (m == NULL) return NULL; - timeout_max = PY_TIMEOUT_MAX / 1000000; - time_max = floor(_PyTime_AsSecondsDouble(_PyTime_MAX)); + timeout_max = (double)PY_TIMEOUT_MAX * 1e-6; + 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) diff --git a/Modules/_winapi.c b/Modules/_winapi.c index 00a26d515e..7e8d4e3846 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,11 +182,11 @@ 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]*/ -/*[python end generated code: output=da39a3ee5e6b4b0d input=94819e72d2c6d558]*/ +/*[python end generated code: output=da39a3ee5e6b4b0d input=4527052fe06e5823]*/ #include "clinic/_winapi.c.h" @@ -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/clinic/_winapi.c.h b/Modules/clinic/_winapi.c.h index 9e1fbe1fdb..01bba36071 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]*/ diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index c7d8b00e6f..661fa1312a 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 bae8bcc356..46df346692 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 c5b7f3256f..13cffa3bf3 100644 --- a/Python/thread_pthread.h +++ b/Python/thread_pthread.h @@ -318,23 +318,66 @@ 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 > PY_TIMEOUT_MAX) { + Py_FatalError("Timeout larger than PY_TIMEOUT_MAX"); + } + + if (microseconds > 0) { MICROSECONDS_TO_TIMESPEC(microseconds, ts); - do { - if (microseconds > 0) + + if (!intr_flag) { + /* cannot overflow thanks to (microseconds > PY_TIMEOUT_MAX) + check done above */ + _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) { + /* 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). */ + } + else { + microseconds = 0; + } + } + } /* Don't check the status if we're stopping because of an interrupt. */ if (!(intr_flag && status == EINTR)) {