FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-30768: Recompute timeout on interrupted lock by vstinner · Pull Request #4103 · python/cpython · GitHub

/ cpython Public
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension .c  (3) .h  (5) .rst  (1) All 3 file types selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
3 changes: 3 additions & 0 deletions Include/pyport.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -787,6 +787,9 @@ extern _invalid_parameter_handler _Py_silent_invalid_parameter_handler;
#include <android/api-level.h>
#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.
Expand Down
21 changes: 14 additions & 7 deletions Include/pythread.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Which integer width does the C preprocessor use? :-) Can 0xFFFFFFFFLL * 1000 be truncated?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I didn't write this code, I only moved it.

The "LL" suffix is for "long long" which should be at least 64-bit signed integer, no?

Are you suggesting to replace the code with "0xFFFFFFFFLL < PY_LLONG_MAX / 1000" to prevent any risk of integer overflow?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ah, ok. No, nevermind.

# 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.
Expand Down
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
@@ -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`.
6 changes: 4 additions & 2 deletions Modules/_threadmodule.c
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
10 changes: 4 additions & 6 deletions Modules/_winapi.c
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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"

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions Modules/clinic/_winapi.c.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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]*/
4 changes: 1 addition & 3 deletions Modules/posixmodule.c
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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;
}
Expand Down
9 changes: 5 additions & 4 deletions Python/thread_nt.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
55 changes: 49 additions & 6 deletions Python/thread_pthread.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -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) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Oh, this function raises an exception on overflow. This bug is now fixed in the new commit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Which new commit? :-)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

4th commit of this PR, commit 7c428b7 which added the following code at the top of the function:

+    if (microseconds > PY_TIMEOUT_MAX) {
+        Py_FatalError("Timeout larger than PY_TIMEOUT_MAX");
+    }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ok, thank you for adding the comments :-)

/* 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)) {
Expand Down

Back | FazBrowse Home | New Git URL