Skip to content

Wrong use of ctypes.GetLastError() in windows_console.py #132888

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
chris-eibl opened this issue Apr 24, 2025 · 2 comments
Open

Wrong use of ctypes.GetLastError() in windows_console.py #132888

chris-eibl opened this issue Apr 24, 2025 · 2 comments
Labels
OS-windows stdlib Python modules in the Lib dir topic-repl Related to the interactive shell type-bug An unexpected behavior, bug, or error

Comments

@chris-eibl
Copy link
Member

chris-eibl commented Apr 24, 2025

Bug report

Bug description:

Everywhere ctypes.GetLastError() is used, even though use_last_error is specified

_KERNEL32 = WinDLL("kernel32", use_last_error=True)

This will always return the "swapped" error, as can be seen here

#ifdef MS_WIN32
if (flags & FUNCFLAG_USE_LASTERROR) {
int temp = space[1];
space[1] = GetLastError();
SetLastError(temp);
}
#endif
}
result = PyObject_Vectorcall(callable, args, nargs, NULL);
if (result == NULL) {
PyErr_FormatUnraisable("Exception ignored while "
"calling ctypes callback function %R",
callable);
}
#ifdef MS_WIN32
if (flags & FUNCFLAG_USE_LASTERROR) {
int temp = space[1];
space[1] = GetLastError();
SetLastError(temp);
}

or carefully reading the docs (the code was easier to read for me :)

The rule is easy: use_last_error=True implies using ctypes.get_last_error(), which @eryksun has for example explained here: https://discuss.python.org/t/a-ctypes-function-to-list-all-loaded-shared-libraries/36370/10

CPython versions tested on:

CPython main branch

Operating systems tested on:

Windows

@chris-eibl chris-eibl added type-bug An unexpected behavior, bug, or error stdlib Python modules in the Lib dir topic-repl Related to the interactive shell labels Apr 24, 2025
@chris-eibl
Copy link
Member Author

The easiest fix would be to search/replace, but I'd like to take the opportunity to also introduce errcheck callables like

from functools import partial

def errcheck(result, func, args, error_val):
    if result == error_val:
        raise WinError(get_last_error())
    return result

SetConsoleMode.errcheck = partial(errcheck, error_val=False)
WaitForSingleObject.errcheck = partial(errcheck, error_val=WAIT_FAILED)

This will make the code much cleaner and "automatically check everywhere". E.g., currently none of the SetConsoleMode or GetConsoleMode calls is checked - and thus a search/replace would not fix them.

This makes the diff slightly larger but IMHO still qualifies for backport, since 3.13 is affected, too.
And the missing checks are bugs IMHO, so we'd have to touch those code parts, anyway, and will most probably end up with not really less diff lines ...

I will do a PR if the repl maintainer agree with the above (@ambv, @lysnikolaou, @pablogsal).

@zooba
Copy link
Member

zooba commented Apr 29, 2025

My preference is still that we ought to be actively migrating from ctypes to a proper native module for these. Performance and reliability will thank us.

Beyond that, I don't have a real preference, though I'd vote for the simpler fix just because it's not worth investing heavily in code that I think should be completely rewritten 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OS-windows stdlib Python modules in the Lib dir topic-repl Related to the interactive shell type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

2 participants