getpass.unix_getpass() always fallback to sys.stdin

[nikratio]

BPO	18116
Nosy	@gpshead, @pitrou, @benjaminp, @alex, @bitdancer, @hynek, @vadmium, @serhiy-storchaka, @vajrasky
Files	issue18116.diff getpass_fix_resourcewarning.patch: Patch to fix resourcewarning warning when using getpass getpass_fix_resourcewarning_pass_the_test.patch: Patch to fix resourcewarning warning when using getpass and pass the test getpass_tty_with_encoding_and_test.patch: Patch to fix resourcewarning warning when using getpass with encoding from os module buffered_open_fd_leak.py getpass_fix_works_with_stringio_and_stdout_stream.patch: Patch to fix resourcewarning warning when using getpass and works with stringio and stdout stream getpass_tty.patch

^{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 = <Date 2013-07-10.21:13:57.895>
created_at = <Date 2013-06-01.23:50:06.702>
labels = ['type-bug', 'library', 'expert-IO']
title = 'getpass.unix_getpass() always fallback to sys.stdin'
updated_at = <Date 2014-04-19.03:25:13.040>
user = 'https://bugs.python.org/nikratio'

bugs.python.org fields:

activity = <Date 2014-04-19.03:25:13.040>
actor = 'martin.panter'
assignee = 'none'
closed = True
closed_date = <Date 2013-07-10.21:13:57.895>
closer = 'r.david.murray'
components = ['Library (Lib)', 'IO']
creation = <Date 2013-06-01.23:50:06.702>
creator = 'nikratio'
dependencies = []
files = ['30442', '30444', '30445', '30463', '30482', '30483', '30495']
hgrepos = []
issue_num = 18116
keywords = ['patch']
message_count = 22.0
messages = ['190458', '190459', '190460', '190461', '190462', '190468', '190471', '190472', '190473', '190520', '190603', '190706', '190710', '190711', '190714', '190716', '190740', '190746', '192836', '192837', '206999', '216839']
nosy_count = 12.0
nosy_names = ['gregory.p.smith', 'pitrou', 'benjamin.peterson', 'stutzbach', 'alex', 'r.david.murray', 'nikratio', 'python-dev', 'hynek', 'martin.panter', 'serhiy.storchaka', 'vajrasky']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue18116'
versions = ['Python 3.4']

[nikratio]

[0] nikratio@vostro:~/tmp$ cat bugme.py
#!python
import getpass
import warnings

warnings.simplefilter('default')
getpass.getpass("What's up?")

[0] nikratio@vostro:~/tmp$ python3 --version
Python 3.3.2

[0] nikratio@vostro:~/tmp$ python3 bugme.py
/usr/lib/python3.3/os.py:1043: ResourceWarning: unclosed file <_io.FileIO name=3 mode='rb+'>
return io.open(fd, *args, **kwargs)
What's up?

[alex]

Attached patch should fix this issue.

[nikratio]

No, it doesn't.

[alex]

Are you sure you applied it correctly? With and without:

Alexanders-MacBook-Pro:cpython alex_gaynor$ ./python.exe x.py
What's up?
Alexanders-MacBook-Pro:cpython alex_gaynor$ hg revert --all --no-backup
reverting Lib/getpass.py
Alexanders-MacBook-Pro:cpython alex_gaynor$ ./python.exe x.py
What's up?
x.py:6: ResourceWarning: unclosed file <_io.TextIOWrapper name=3 mode='w+' encoding='UTF-8'>
getpass.getpass("What's up?")

[nikratio]

Yes, I'm pretty sure:

[0] nikratio@vostro:/tmp$ cp /usr/lib/python3.3/getpass.py .
[0] nikratio@vostro:/tmp$ patch -p2 < bpo-18116.diff
patching file getpass.py
Hunk #1 succeeded at 57 (offset -1 lines).

[0] nikratio@vostro:~/tmp$ python3 bugme.py
/usr/lib/python3.3/os.py:1043: ResourceWarning: unclosed file <_io.FileIO name=3 mode='rb+'>
return io.open(fd, *args, **kwargs)
What's up?

# Test if we're using the patched getpass.py...
[0] nikratio@vostro:/tmp$ vim getpass.py
[0] nikratio@vostro:/tmp$ python3 bugme.py
Hello
/usr/lib/python3.3/os.py:1043: ResourceWarning: unclosed file <_io.FileIO name=3 mode='rb+'>
return io.open(fd, *args, **kwargs)
What's up?

[vajrasky]

This bug happens in Python 3.4 as well.

[sky@localhost cpython]$ ./python --version
Python 3.4.0a0
[sky@localhost cpython]$ ./python /tmp/bugme.py
/home/sky/Code/python/programming_language/cpython/Lib/os.py:1025: ResourceWarning: unclosed file <_io.FileIO name=3 mode='rb+'>
return io.open(fd, *args, **kwargs)
What's up?

I tried to apply the patch manually (by copying, cutting and pasting) from Alex but Nikolaus is right. The patch does not work. The bug still happens

[vajrasky]

I isolate the bug. It happens in these lines:

        # Always try reading and writing directly on the tty first.
        fd = os.open('/dev/tty', os.O_RDWR|os.O_NOCTTY)
        tty = os.fdopen(fd, 'w+', 1)

So to produce the bug more specifically, you can try this python file:

# bugme2.py
import os

fd = os.open('/dev/tty', os.O_RDWR|os.O_NOCTTY)
os.fdopen(fd, 'w+', 1)
# end of bugme2.py

In Linux Fedora 18, I would get this error:

/home/sky/Code/python/programming_language/cpython/Lib/os.py:1025: ResourceWarning: unclosed file <_io.FileIO name=3 mode='rb+'>
  return io.open(fd, *args, **kwargs)
Traceback (most recent call last):
  File "/tmp/bugme2.py", line 4, in <module>
    os.fdopen(fd, 'w+', 1)
  File "/home/sky/Code/python/programming_language/cpython/Lib/os.py", line 1025, in fdopen
    return io.open(fd, *args, **kwargs)
io.UnsupportedOperation: File or stream is not seekable.

[vajrasky]

I have investigated this problem and come up with the patch to fix the problem. This patch does the job. Caution: only for Python 3.4. But translating this patch to Python 3.3 should be straightforward.

I hope this patch could be the foundation for better programmers to create better patch.

Some of the issues with this patch are: I am not sure how to handle encoding and where the best place to close tty is.

Reference:
stefanholek/term#1
http://stackoverflow.com/questions/5471158/typeerror-str-does-not-support-the-buffer-interface

[vajrasky]

Sorry,

My previous patch breaks the test. This one should pass the test and fix the bug.

Still, there are ugly code in the patch that I hope better programmers could fix.

[benjaminp]

This code is pretty broken. I don't think ttys are ever seekable, so the os.fdopen has probably been always failing since 3.0. It thus always leaks an fd to '/dev/tty' if the first os.open succeeds. The whole function should probably be rewriten to work with byte streams encoding the prompt with os.device_encoding(tty_fd) falling back on locale.getpreferredencoding().

[vajrasky]

Based on the input from Benjamin Peterson, I grab encoding from the os module in the getpass module.

I put some test as well.

Until the whole function is rewritten, I hope this patch will suffice and do the job properly.

[serhiy-storchaka]

>>> getpass.getpass('Password: ', sys.stdout)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/serhiy/py/cpython/Lib/getpass.py", line 72, in unix_getpass
    passwd = _raw_input(prompt, stream, input=input)
  File "/home/serhiy/py/cpython/Lib/getpass.py", line 146, in _raw_input
    stream.write(bytes(prompt, tty_encoding))
TypeError: must be str, not bytes

>>> getpass.getpass('Password: ', io.StringIO())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/serhiy/py/cpython/Lib/getpass.py", line 72, in unix_getpass
    passwd = _raw_input(prompt, stream, input=input)
  File "/home/serhiy/py/cpython/Lib/getpass.py", line 146, in _raw_input
    stream.write(bytes(prompt, tty_encoding))
TypeError: string argument expected, got 'bytes'

[serhiy-storchaka]

The problem is in io.open, not in getpass. Here is a test script.

$ ./python buffered_open_fd_leak.py 
buffered_open_fd_leak.py:7: ResourceWarning: unclosed file <_io.FileIO name=3 mode='rb+'>
  tty = io.open(fd, 'w+', 1)

[vajrasky]

So the correct fix should be:

1. Make sure we can open /dev/tty in non-binary mode,
2. We can write string to /dev/tty,
3. Close the leak if we can not open /dev/tty.

Is it?

[vajrasky]

Fixing IO leak resource would fix this bug but leave another bug opened which I try to fix as well.

These statements with Python3 under Linux will always fail because we need to open /dev/tty file in binary mode (for whatever reason).

fd = os.open('/dev/tty', os.O_RDWR|os.O_NOCTTY)
tty = os.fdopen(fd, 'w+', 1)

So I guess the correct fix would be to open /dev/tty in binary mode (and set buffering off) and go on from there.

Of course, one can argue whether this bug should be separated from the original bug (resource warning). I am not sure either.

Anyway, here is the patch that will work with stream StringIO and stdout.

Thank you for Serhiy for pointing out my mistakes.

[serhiy-storchaka]

>>> getpass.getpass('Password: ', open('/dev/stdout', 'w'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/serhiy/py/cpython/Lib/getpass.py", line 72, in unix_getpass
    passwd = _raw_input(prompt, stream, input=input)
  File "/home/serhiy/py/cpython/Lib/getpass.py", line 143, in _raw_input
    stream.write(bytes(prompt, tty_encoding))
TypeError: must be str, not bytes

It seems that you are moving in the wrong direction. No need to test explicitly for stdout/stderr/etc, the code should work with arbitrary text stream.

[serhiy-storchaka]

Here is a patch which fixes getpass bug: unix_getpass() always fallback to sys.stdin. As side effect it also fixes resource warning in getpass().

I'm not sure I have correctly changed tests. David, could you please review the patch?

[bitdancer]

The test changes look correct to me, but it sure would be nice to come up with less fragile tests. For a function like this, though, it probably isn't possible.

[python-dev]

New changeset 70f55dc9d43f by R David Murray in branch 'default':
bpo-18116: getpass no longer always falls back to stdin.
http://hg.python.org/cpython/rev/70f55dc9d43f

[bitdancer]

I played around with this for a bit, but I couldn't come up with any test improvements, or any way to test the bug that is being fixed. So I just committed it as is. Thanks, Serhiy. And thanks Vajrasky for giving it a try and figuring out some of the stuff that *doesn't* work as a fix :)

I decided to only commit this to 3.4. I don't think the risk of an unexpected behavior change in a maintenance release is worth the relatively small benefit that this fix provides.

[python-dev]

New changeset 100f632d4306 by R David Murray in branch '3.3':
bpo-18116: backport fix to 3.3 since real-world failure mode demonstrated.
http://hg.python.org/cpython/rev/100f632d4306

New changeset 29a5a5b39dd6 by R David Murray in branch 'default':
Mostly-null merge of bpo-18116 backport (updated NEWS entry).
http://hg.python.org/cpython/rev/29a5a5b39dd6

[vadmium]

I opened bpo-21310 about a ResourceWarning from open() which I suspect is the same as what was originally described here.