From 8f791d9081bfb1c9abf104f01a9c49ec1039f908 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Thu, 30 Sep 2021 01:21:39 +0200 Subject: [PATCH 1/5] bpo-41710: PyThread_acquire_lock_timed() clamps the timout PyThread_acquire_lock_timed() now clamps the timeout into the [_PyTime_MIN; _PyTime_MAX] range (_PyTime_t type) if it is too large, rather than calling Py_FatalError() which aborts the process. PyThread_acquire_lock_timed() no longer uses MICROSECONDS_TO_TIMESPEC() to compute sem_timedwait() argument, but _PyTime_GetSystemClock() and _PyTime_AsTimespec_truncate(). --- .../2021-09-30-03-14-35.bpo-41710.DDWJKx.rst | 2 + Python/thread_nt.h | 8 ++- Python/thread_pthread.h | 61 ++++++++++--------- 3 files changed, 41 insertions(+), 30 deletions(-) create mode 100644 Misc/NEWS.d/next/C API/2021-09-30-03-14-35.bpo-41710.DDWJKx.rst diff --git a/Misc/NEWS.d/next/C API/2021-09-30-03-14-35.bpo-41710.DDWJKx.rst b/Misc/NEWS.d/next/C API/2021-09-30-03-14-35.bpo-41710.DDWJKx.rst new file mode 100644 index 00000000000000..902c7cc8f2b99f --- /dev/null +++ b/Misc/NEWS.d/next/C API/2021-09-30-03-14-35.bpo-41710.DDWJKx.rst @@ -0,0 +1,2 @@ +The PyThread_acquire_lock_timed() function now clamps the timeout if it is +too large, rather than aborting the process. Patch by Victor Stinner. diff --git a/Python/thread_nt.h b/Python/thread_nt.h index f8c098ca5238a8..b2f111e4ba115e 100644 --- a/Python/thread_nt.h +++ b/Python/thread_nt.h @@ -312,7 +312,13 @@ PyThread_acquire_lock_timed(PyThread_type_lock aLock, if (microseconds % 1000 > 0) ++milliseconds; if (milliseconds > PY_DWORD_MAX) { - Py_FatalError("Timeout larger than PY_TIMEOUT_MAX"); + // bpo-41710: PyThread_acquire_lock_timed() cannot report timeout + // overflow to the caller, so clamp the timeout to + // [0, PY_DWORD_MAX] milliseconds. + // + // _thread.Lock.acquire() and _thread.RLock.acquire() raise an + // OverflowError if microseconds is greater than PY_TIMEOUT_MAX. + milliseconds = PY_DWORD_MAX; } } else { diff --git a/Python/thread_pthread.h b/Python/thread_pthread.h index 7f04151ca91fdc..8348ab875e0357 100644 --- a/Python/thread_pthread.h +++ b/Python/thread_pthread.h @@ -433,33 +433,45 @@ PyThread_acquire_lock_timed(PyThread_type_lock lock, PY_TIMEOUT_T microseconds, PyLockStatus success; 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 > PY_TIMEOUT_MAX) { - Py_FatalError("Timeout larger than PY_TIMEOUT_MAX"); + _PyTime_t timeout; + if (microseconds >= 0) { + _PyTime_t ns; + if (microseconds < _PyTime_MAX / 1000) { + ns = microseconds * 1000; + } + else { + // bpo-41710: PyThread_acquire_lock_timed() cannot report timeout + // overflow to the caller, so clamp the timeout to + // [_PyTime_MIN, _PyTime_MAX]. + // + // _thread.Lock.acquire() and _thread.RLock.acquire() raise an + // OverflowError if microseconds is greater than PY_TIMEOUT_MAX. + ns = _PyTime_MAX; + } + timeout = _PyTime_FromNanoseconds(ns); + } + else { + timeout = _PyTime_FromNanoseconds(-1); } - if (microseconds > 0) { - MICROSECONDS_TO_TIMESPEC(microseconds, ts); - - 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; - } + _PyTime_t deadline = 0; + if (timeout > 0 && !intr_flag) { + deadline = _PyTime_GetMonotonicClock() + timeout; } while (1) { - if (microseconds > 0) { + if (timeout > 0) { + _PyTime_t t = _PyTime_GetSystemClock() + timeout; + struct timespec ts; + _PyTime_AsTimespec_clamp(t, &ts); status = fix_status(sem_timedwait(thelock, &ts)); } - else if (microseconds == 0) { + else if (timeout == 0) { status = fix_status(sem_trywait(thelock)); } else { @@ -472,32 +484,23 @@ PyThread_acquire_lock_timed(PyThread_type_lock lock, PY_TIMEOUT_T microseconds, break; } - if (microseconds > 0) { + if (timeout > 0) { /* wait interrupted by a signal (EINTR): recompute the timeout */ - _PyTime_t dt = deadline - _PyTime_GetMonotonicClock(); - if (dt < 0) { + _PyTime_t timeout = deadline - _PyTime_GetMonotonicClock(); + if (timeout < 0) { status = ETIMEDOUT; break; } - else if (dt > 0) { - _PyTime_t realtime_deadline = _PyTime_GetSystemClock() + dt; - _PyTime_AsTimespec_clamp(realtime_deadline, &ts); - /* 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)) { - if (microseconds > 0) { + if (timeout > 0) { if (status != ETIMEDOUT) CHECK_STATUS("sem_timedwait"); } - else if (microseconds == 0) { + else if (timeout == 0) { if (status != EAGAIN) CHECK_STATUS("sem_trywait"); } From d8e1989e23c3250e358e44a93fa5e79deeab0f00 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Thu, 30 Sep 2021 03:52:24 +0200 Subject: [PATCH 2/5] Fix test --- Python/thread_pthread.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/thread_pthread.h b/Python/thread_pthread.h index 8348ab875e0357..ac9ebc73fba99d 100644 --- a/Python/thread_pthread.h +++ b/Python/thread_pthread.h @@ -441,7 +441,7 @@ PyThread_acquire_lock_timed(PyThread_type_lock lock, PY_TIMEOUT_T microseconds, _PyTime_t timeout; if (microseconds >= 0) { _PyTime_t ns; - if (microseconds < _PyTime_MAX / 1000) { + if (microseconds <= _PyTime_MAX / 1000) { ns = microseconds * 1000; } else { From 1cd14744a704477dd97750b577338e877e3f1901 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Thu, 30 Sep 2021 08:55:39 +0200 Subject: [PATCH 3/5] Fix PY_TIMEOUT_MAX on Windows Fix _thread.TIMEOUT_MAX value on Windows: the maximum timeout is 0x7FFFFFFF milliseconds (around 24.9 days), not 0xFFFFFFFF milliseconds (around 49.7 days). Set PY_TIMEOUT_MAX to 0x7FFFFFFF milliseconds, rather than 0xFFFFFFFF milliseconds. Fix PY_TIMEOUT_MAX overflow test: replace (us >= PY_TIMEOUT_MAX) with (us > PY_TIMEOUT_MAX). --- Include/cpython/pytime.h | 2 ++ Include/pythread.h | 8 +++++--- .../2021-09-30-08-57-50.bpo-41710.JMsPAW.rst | 3 +++ Modules/_queuemodule.c | 2 +- Modules/_threadmodule.c | 2 +- Modules/faulthandler.c | 2 +- Python/thread_nt.h | 18 +++++++++++++----- Python/thread_pthread.h | 2 ++ 8 files changed, 28 insertions(+), 11 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2021-09-30-08-57-50.bpo-41710.JMsPAW.rst diff --git a/Include/cpython/pytime.h b/Include/cpython/pytime.h index b5a351349f85b3..db3adfab6a9aba 100644 --- a/Include/cpython/pytime.h +++ b/Include/cpython/pytime.h @@ -14,7 +14,9 @@ extern "C" { store a duration, and so indirectly a date (related to another date, like UNIX epoch). */ typedef int64_t _PyTime_t; +// _PyTime_MIN nanoseconds is around -292.3 years #define _PyTime_MIN INT64_MIN +// _PyTime_MAX nanoseconds is around +292.3 years #define _PyTime_MAX INT64_MAX #define _SIZEOF_PYTIME_T 8 diff --git a/Include/pythread.h b/Include/pythread.h index bb9d86412218ad..cf4cc9a7473f1a 100644 --- a/Include/pythread.h +++ b/Include/pythread.h @@ -61,9 +61,11 @@ PyAPI_FUNC(int) _PyThread_at_fork_reinit(PyThread_type_lock *lock); convert microseconds to nanoseconds. */ # define PY_TIMEOUT_MAX (LLONG_MAX / 1000) #elif defined (NT_THREADS) - /* In the NT API, the timeout is a DWORD and is expressed in milliseconds */ -# if 0xFFFFFFFFLL * 1000 < LLONG_MAX -# define PY_TIMEOUT_MAX (0xFFFFFFFFLL * 1000) + /* In the NT API, the timeout is a DWORD and is expressed in milliseconds, + * a positive number between 0 and 0x7FFFFFFF (see WaitForSingleObject() + * documentation). */ +# if 0x7FFFFFFFLL * 1000 < LLONG_MAX +# define PY_TIMEOUT_MAX (0x7FFFFFFFLL * 1000) # else # define PY_TIMEOUT_MAX LLONG_MAX # endif diff --git a/Misc/NEWS.d/next/Library/2021-09-30-08-57-50.bpo-41710.JMsPAW.rst b/Misc/NEWS.d/next/Library/2021-09-30-08-57-50.bpo-41710.JMsPAW.rst new file mode 100644 index 00000000000000..516214a619e8eb --- /dev/null +++ b/Misc/NEWS.d/next/Library/2021-09-30-08-57-50.bpo-41710.JMsPAW.rst @@ -0,0 +1,3 @@ +Fix :data:`_thread.TIMEOUT_MAX` value on Windows: the maximum timeout is +0x7FFFFFFF milliseconds (around 24.9 days), not 0xFFFFFFFF milliseconds (around +49.7 days). diff --git a/Modules/_queuemodule.c b/Modules/_queuemodule.c index 58772d7f0f5234..b6769e6b7764ef 100644 --- a/Modules/_queuemodule.c +++ b/Modules/_queuemodule.c @@ -223,7 +223,7 @@ _queue_SimpleQueue_get_impl(simplequeueobject *self, PyTypeObject *cls, } microseconds = _PyTime_AsMicroseconds(timeout_val, _PyTime_ROUND_CEILING); - if (microseconds >= PY_TIMEOUT_MAX) { + if (microseconds > PY_TIMEOUT_MAX) { PyErr_SetString(PyExc_OverflowError, "timeout value is too large"); return NULL; diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 5b5d2c5b03ec3f..fa7e6d0e09d182 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -164,7 +164,7 @@ lock_acquire_parse_args(PyObject *args, PyObject *kwds, _PyTime_t microseconds; microseconds = _PyTime_AsMicroseconds(*timeout, _PyTime_ROUND_TIMEOUT); - if (microseconds >= PY_TIMEOUT_MAX) { + if (microseconds > PY_TIMEOUT_MAX) { PyErr_SetString(PyExc_OverflowError, "timeout value is too large"); return -1; diff --git a/Modules/faulthandler.c b/Modules/faulthandler.c index 350f4cf6b8edff..868b4f4f42de68 100644 --- a/Modules/faulthandler.c +++ b/Modules/faulthandler.c @@ -710,7 +710,7 @@ faulthandler_dump_traceback_later(PyObject *self, return NULL; } /* Limit to LONG_MAX seconds for format_timeout() */ - if (timeout_us >= PY_TIMEOUT_MAX || timeout_us / SEC_TO_US >= LONG_MAX) { + if (timeout_us > PY_TIMEOUT_MAX || timeout_us / SEC_TO_US > LONG_MAX) { PyErr_SetString(PyExc_OverflowError, "timeout value is too large"); return NULL; diff --git a/Python/thread_nt.h b/Python/thread_nt.h index b2f111e4ba115e..9846f911616db6 100644 --- a/Python/thread_nt.h +++ b/Python/thread_nt.h @@ -292,6 +292,10 @@ PyThread_free_lock(PyThread_type_lock aLock) FreeNonRecursiveMutex(aLock) ; } +// WaitForSingleObject() documentation: "The time-out value needs to be a +// positive number between 0 and 0x7FFFFFFF." INFINITE is equal to 0xFFFFFFFF. +const DWORD TIMEOUT_MS_MAX = 0x7FFFFFFF; + /* * Return 1 on success if the lock was acquired * @@ -309,16 +313,20 @@ PyThread_acquire_lock_timed(PyThread_type_lock aLock, if (microseconds >= 0) { milliseconds = microseconds / 1000; - if (microseconds % 1000 > 0) - ++milliseconds; - if (milliseconds > PY_DWORD_MAX) { + // Round milliseconds away from zero (+inf) + if (microseconds % 1000 > 0) { + milliseconds++; + } + if (milliseconds > TIMEOUT_MS_MAX) { // bpo-41710: PyThread_acquire_lock_timed() cannot report timeout // overflow to the caller, so clamp the timeout to - // [0, PY_DWORD_MAX] milliseconds. + // [0, TIMEOUT_MS_MAX] milliseconds. + // + // TIMEOUT_MS_MAX milliseconds is around 24.9 days. // // _thread.Lock.acquire() and _thread.RLock.acquire() raise an // OverflowError if microseconds is greater than PY_TIMEOUT_MAX. - milliseconds = PY_DWORD_MAX; + milliseconds = TIMEOUT_MS_MAX; } } else { diff --git a/Python/thread_pthread.h b/Python/thread_pthread.h index ac9ebc73fba99d..3815ffae20c017 100644 --- a/Python/thread_pthread.h +++ b/Python/thread_pthread.h @@ -449,6 +449,8 @@ PyThread_acquire_lock_timed(PyThread_type_lock lock, PY_TIMEOUT_T microseconds, // overflow to the caller, so clamp the timeout to // [_PyTime_MIN, _PyTime_MAX]. // + // _PyTime_MAX nanoseconds is around 292.3 years. + // // _thread.Lock.acquire() and _thread.RLock.acquire() raise an // OverflowError if microseconds is greater than PY_TIMEOUT_MAX. ns = _PyTime_MAX; From 01f5238fa003026823e6d97bfaf33b35df974054 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Thu, 30 Sep 2021 09:27:20 +0200 Subject: [PATCH 4/5] Fix typo --- Python/thread_nt.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/thread_nt.h b/Python/thread_nt.h index 9846f911616db6..7e7efcad6d7246 100644 --- a/Python/thread_nt.h +++ b/Python/thread_nt.h @@ -313,7 +313,7 @@ PyThread_acquire_lock_timed(PyThread_type_lock aLock, if (microseconds >= 0) { milliseconds = microseconds / 1000; - // Round milliseconds away from zero (+inf) + // Round milliseconds away from zero if (microseconds % 1000 > 0) { milliseconds++; } From 60bc5817c896dbf2dc3ccdb396f8192dddbbcb06 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Thu, 30 Sep 2021 09:45:31 +0200 Subject: [PATCH 5/5] Fix signed/unsigned comparison --- Python/thread_nt.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/thread_nt.h b/Python/thread_nt.h index 7e7efcad6d7246..0beb3d3af2fd35 100644 --- a/Python/thread_nt.h +++ b/Python/thread_nt.h @@ -317,7 +317,7 @@ PyThread_acquire_lock_timed(PyThread_type_lock aLock, if (microseconds % 1000 > 0) { milliseconds++; } - if (milliseconds > TIMEOUT_MS_MAX) { + if (milliseconds > (PY_TIMEOUT_T)TIMEOUT_MS_MAX) { // bpo-41710: PyThread_acquire_lock_timed() cannot report timeout // overflow to the caller, so clamp the timeout to // [0, TIMEOUT_MS_MAX] milliseconds.