bpo-41710: PyThread_acquire_lock_timed() clamps the timout (GH-28643)

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().

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).

Author: vstinner

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
 

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

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.

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).

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;

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;

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;

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,10 +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) {
-            Py_FatalError("Timeout larger than PY_TIMEOUT_MAX");
+        // Round milliseconds away from zero
+        if (microseconds % 1000 > 0) {
+            milliseconds++;
+        }
+        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.
+            //
+            // 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 = TIMEOUT_MS_MAX;
         }
     }
     else {

Python/thread_pthread.h

@@ -433,33 +433,47 @@ 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].
+            //
+            // _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;
+        }
+        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 +486,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");
         }