Skip to content

Off-by-one error in _winapi.WaitForMultipleObjects #94242

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
randomascii opened this issue Jun 24, 2022 · 4 comments
Open

Off-by-one error in _winapi.WaitForMultipleObjects #94242

randomascii opened this issue Jun 24, 2022 · 4 comments
Labels
3.10 only security fixes 3.11 only security fixes 3.12 only security fixes OS-windows stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@randomascii
Copy link

randomascii commented Jun 24, 2022

Bug report

_winapi_WaitForMultipleObjects_impl in _winapi.c creates an array of MAXIMUM_WAIT_OBJECTS handles but then arbitrarily enforces a cap of MAXIMUM_WAIT_OBJECTS - 1 handles. This is incorrect. WaitForMultipleObjects is clearly documented as being able to handle MAXIMUM_WAIT_OBJECTS (64) handles. This simple C++ sample code demonstrates this:

#include <stdio.h>
#include <windows.h>

#include

int main(int argc, char* argv[]) {
for (DWORD count = 60; count < 66; ++count) {
printf("About to wait on %d handles.\n", count);
std::vector handles(count);
for (int i = 0; i < count; ++i)
handles[i] = CreateEventW(nullptr, FALSE, TRUE, nullptr);
DWORD wait_result = WaitForMultipleObjects(count, handles.data(), TRUE, INFINITE);
if (wait_result == WAIT_OBJECT_0)
printf(" Wait satisfied by object 0.\n");
else if (wait_result == WAIT_FAILED)
printf(" Wait failed.\n");
else
printf(" Result was %lu.\n", wait_result);
for (int i = 0; i < count; ++i)
CloseHandle(handles[i]);
}
}

This error also shows up here:
3988986#diff-eb5cd13d65d2f215d8dc0d95eb1a1341ff69b226ac56ec6099e39582edecc8cfR219
and here:
3988986#diff-84a955ea642d41d9f164e9f94499d8eb33c357ec7f4362a341951f6f67ab21a1R117

Note that this is in addition to the off-by-one error that was tracked by this issue:
#84444

That is, there used to be an off-by-two error (maximum of 60 processes could be waited on) and now there is an off-by-one error (maximum of 61 processes can be waited on). In Python 3.8.10 the limit is still 60, but in Python 3.9 the limit is (as far as I can tell) 61. It should be 62.

Your environment

Python 3.8.10 (tags/v3.8.10:3d8993a, May 3 2021, 11:48:03) [MSC v.1928 64 bit (AMD64)] on win32
Windows 10

Linked PRs

@eryksun
Copy link
Contributor

eryksun commented Jun 24, 2022

If the wait is for any kernel object to be signaled, instead of waiting for all objects to be signaled, and it's the main thread, then the limit must be MAXIMUM_WAIT_OBJECTS - 1 (63) objects because the last index is reserved for _PyOS_SigintEvent(), the kernel event object that gets set by Python's SIGINT handler:

    /* If this is the main thread then make the wait interruptible
       by Ctrl-C unless we are waiting for *all* handles */
    if (!wait_flag && _PyOS_IsMainThread()) {
        sigint_event = _PyOS_SigintEvent();
        assert(sigint_event != NULL);
        handles[nhandles++] = sigint_event;
    }

I assume the same limit is applied for all threads for the sake of simplicity. Technically, it could be extended to 64 handles if the condition !wait_flag && _PyOS_IsMainThread() is false.

@randomascii
Copy link
Author

Gotcha. The comments are incorrect still since they say that the Windows limit is 63 and that Python uses two handles, but in fact the Windows limit is 64 and Python uses three handles.

I think having the limit as 61 instead of 62 is fine. I don't know whether the misleading comments matter or not.

@eryksun eryksun added OS-windows stdlib Python modules in the Lib dir 3.11 only security fixes 3.10 only security fixes 3.12 only security fixes labels Jun 26, 2022
@eryksun
Copy link
Contributor

eryksun commented Jun 26, 2022

Did you want to submit a PR to update the comment in Lib/concurrent/futures/process.py? Otherwise this can be closed.

randomascii added a commit to randomascii/cpython that referenced this issue Jun 26, 2022
The calculation of _MAX_WINDOWS_WORKERS contained two off-by-one errors.
They cancelled each other out so this does not change the value of
_MAX_WINDOWS_WORKERS, just how it is determined. This could avoid future
confusion about where the 61 value comes from.

Refs python#94242
@randomascii
Copy link
Author

Did you want to submit a PR to update the comment in Lib/concurrent/futures/process.py? Otherwise this can be closed.

Sure. In progress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.10 only security fixes 3.11 only security fixes 3.12 only security fixes OS-windows stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

2 participants