getpass.getpass on Windows fallback detection is incomplete

[matejcik]

BPO	44762
Nosy	@terryjreedy, @pfmoore, @tjguk, @matejcik, @zware, @eryksun, @zooba

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2021-07-28.13:08:08.808>
labels = ['type-bug', '3.9', '3.10', '3.11', 'library', 'OS-windows']
title = 'getpass.getpass on Windows fallback detection is incomplete'
updated_at = <Date 2021-08-05.13:22:40.975>
user = 'https://github.com/matejcik'

bugs.python.org fields:

activity = <Date 2021-08-05.13:22:40.975>
actor = 'eryksun'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)', 'Windows']
creation = <Date 2021-07-28.13:08:08.808>
creator = 'matejcik'
dependencies = []
files = []
hgrepos = []
issue_num = 44762
keywords = []
message_count = 11.0
messages = ['398370', '398372', '398374', '398399', '398431', '398509', '398510', '398511', '398519', '398962', '399004']
nosy_count = 7.0
nosy_names = ['terry.reedy', 'paul.moore', 'tim.golden', 'matejcik', 'zach.ware', 'eryksun', 'steve.dower']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'needs patch'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue44762'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

[matejcik]

The fallback detection for win_getpass checks that sys.stdin is different from sys.__stdin__. If yes, it assumes that it's incapable of disabling echo, and calls default_getpass which reads from stdin.

If they are the same object, it assumes it's in a terminal and uses msvcrt to read characters.

I was not able to find any rationale for this check, it seems to have been introduced, oh wow, in 2001, to fix something IDLE-related.

Anyway, the check is trivially incorrect. It fails when such script is launched from subprocess.Popen(stdin=PIPE), where both sys.stdin and sys.__stdin__ are the same instance of TextIOWrapper. Same actually happens in MinTTY (Git Bash etc.) which is not a true terminal as far as I was able to find out?

It seems that a better check would be, e.g., sys.stdin.isatty(), which correctly returns False both in subprocesses and in MinTTY.

[matejcik]

...this is a problem because:

When the check incorrectly infers that it can use msvcrt while its stdin is a pipe, the calls to putwch and getwch are going into the void and the program effectively freezes waiting for input that never comes.

See also:
https://stackoverflow.com/questions/49858821/python-getpass-doesnt-work-on-windows-git-bash-mingw64/54046572
ipython/ipython#854

[matejcik]

For that matter, in standard Windows Command Prompt sys.stdin and sys.__stdin__ are also identical, but isatty() reports True.

I suspect is that the code has drifted and sys.stdin is always identical to sys.__stdin__ ?

[eryksun]

When the check incorrectly infers that it can use msvcrt while
its stdin is a pipe, the calls to putwch and getwch are going
into the void and the program effectively freezes waiting for
input that never comes.

The C runtime's getwch() and putwch() functions use the console I/O files "CONIN$" and "CONOUT$". If "CONIN$" can't be opened because the process has no console, then getwch() fails and returns U+FFFF. But mintty does have a console, albeit with a hidden window, which gets inherited by bash and child processes. So getwch() ends up waiting for a key to be pressed in an inaccessible, hidden window.

It should suffice to check that sys.stdin isn't None and that sys.stdin.isatty() is True, since it's reasonable to assume that the console has a visible window in this case. Currently this isn't a bulletproof check, however, because the Windows C runtime doesn't implement isatty() correctly. It returns true for any character device, which includes the common case of redirecting standard I/O to the "NUL" device. io.FileIO.isatty() could be updated to return True only if both C isatty() is true and GetConsoleMode() succeeds. This would filter out false positives for non-console character devices, particularly "NUL".

[zooba]

We could also provide a better check in WindowsConsoleIO.isatty, since that should have already handled most of the previous checks (I wouldn't want to do a typecheck in getpass, though). But yeah, this seems like "sys.stdin and sys.stdin.isatty()" is the right condition.

[eryksun]

It should be noted that changing win_getpass() to check sys.stdin.isatty() makes it inconsistent with unix_getpass(). The latter tries to open "/dev/tty" regardless of sys.stdin. To be consistent, win_getpass() would be implemented to call fallback_getpass() only if "CONIN$" is inaccessible.

We'd still have the problem that's reported here. A classic console session (not ConPTY) can have no window, or an invisible window, and thus no way for a user to enter text. I don't see any reports that reading from "/dev/tty" makes getpass() wait forever in POSIX, so this situation is probably unique to Windows, or at least it's far more likely in Windows, which may justify introducing an inconsistency.

[eryksun]

We could also provide a better check in WindowsConsoleIO.isatty,

For isatty(), my concern is a false positive if the file is the "NUL" device, which should be an io.FileIO object. I suppose checking the file type in io._WindowsConsoleIO.isatty() could be useful in case the file descriptor gets reassigned to a non-console file (e.g. via os.close or os.dup2).

I'd prefer to implement the check in a common _Py_isatty() function that supports the optional errno values EBADF and ENOTTY [1]. For example:

    int
    _Py_isatty(int fd)
    {
        DWORD mode;
        HANDLE handle = _Py_get_osfhandle_noraise(fd);

        if (handle == INVALID_HANDLE_VALUE) {
            errno = EBADF;
            return 0;
        }

        switch (GetFileType(handle)) {
        case FILE_TYPE_CHAR:
            break;
        case FILE_TYPE_DISK:
        case FILE_TYPE_PIPE:
            errno = ENOTTY;
            return 0;
        default:
            errno = EBADF;
            return 0;
        }
        
        if (!GetConsoleMode(handle, &mode) && 
              GetLastError() != ERROR_ACCESS_DENIED) {
            errno = ENOTTY;
            return 0;
        }
        
        return 1;
    }

---

[1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/isatty.html

[zooba]

I suppose checking the file type in io._WindowsConsoleIO.isatty() could be useful in case the file descriptor gets reassigned to a non-console file (e.g. via os.close or os.dup2).

Pretty sure we currently don't support these anyway. WindowsConsoleIO doesn't actually use the standard file descriptors for stdin/out/err - it creates new ones based on the real handles just in case a caller requests them. In my opinion, correctness of "write to console" outweighs correctness of "manually close the console FD and wait for a new one with the same number to be opened" ;)

If we have WindowsConsoleIO as the type, it shouldn't be possible for it to refer to any kind of file other than the console or it would be a FileIO. If that's no longer true, we should fix that first. WindowsConsoleIO.isatty ought to be "return True".

[eryksun]

WindowsConsoleIO doesn't actually use the standard file descriptors
for stdin/out/err

To resolve bpo-30555, _WindowsConsoleIO was changed to eliminate self->handle and get the underlying handle dynamically via _get_osfhandle(). It's thus susceptible to close() and dup2() calls in Python or a C library. This consequence was noted. The alternative was to duplicate the OS handle that self->fd wraps, isolating it from self->handle. But then self->fd could get closed, or duped to another file. Either way there are file and file-type sync problems. It's not going to work in the seamless way that people are used to in POSIX, which only uses a single io.FileIO type. I think a better resolution will be possible in the context of your side project, if you're still working on it...

[terryjreedy]

The rationale for the __stdin__ and stdin check is this: When python starts, both are usually set an io.TextIOWrapper wrapping a system console. (At least on Windows, both may instead be None instead.) __stdin__ should never be altered. Getpass.getpass knows how to turn off echo on a system terminal. So if stdin is not None and is not __stdin__, stdin is very likely not a system terminal, and if so, getpass does not know to turn off echo, if indeed this is possible.

As far as I know, the tk and hence tkinter text widget do not come with a option to not display key presses that are stored in the widget. An application would have to intercept keypresses and store them elsewhere.
But this is quite different issue.

This issue is not about echo, but about interactivity. Testing that with isatty is *NOT* a substitute for testing whether an interactive device is the system terminal or not. (IDLE's stdio connected to Shell passes isatty.) Any new test would have to added without deleting the current test.

[eryksun]

The sys.stdin is not sys.__stdin__ check is not relevant information. We need to know whether msvcrt.getwch() works and that the user should be able to type the password in the console window. sys.stdin could be a file object for the NUL device, a pipe, or a file.

Also, this check has never been implemented in POSIX. If I run python -m idlelib in Linux, getpass() still reads the password from the terminal instead of sys.stdin.

IDLE's stdio connected to Shell passes isatty

IOBase.isatty() is more abstract than I expected, since it can be true for any stream that's "interactive". While FileIO.isatty() and os.isatty() are definitely wrong in Windows (i.e. they should not be true for NUL or a serial/parallel port), I see now that fixing isatty() doesn't solve the problem.

We need to directly check whether sys.stdin is a console. If so, we can reasonably assume that there's a visible, accessible console window. The check would be something like:

try:
    _winapi.GetConsoleMode(msvcrt.get_osfhandle(sys.stdin.fileno()))
except (AttributeError, OSError):
    return fallback_getpass(prompt, stream)

_winapi.GetConsoleMode() would need to be implemented.

This is inconsistent with Unix since it's not trying to open "CONIN$". But relying on the latter is problematic in Windows since it succeeds in any process that's attached to a console session, even if the console window is hidden or never created (i.e. CREATE_NO_WINDOW).