homepage

Here is a patch which cures the following 
ResourceWarning: unclosed file <_io.TextIOWrapper name=3 mode='w+'
encoding='UTF-8'>
(This only fixes the bug, not the rest of this code...)
I did not try it out, but according to some hg cat's this also
applies to at least v3.2.

This new patch (11466.2.patch) also includes the #11236 patch, 
because if ^C would work as expected then that would not close the 
fd either. 
It's identical to the first patch beside that.

Sorry, accidentally removed the first patch.
-1 on the second patch: there’s another issue for that, and your patch is not uncontroversial.

On Fri, Mar 11, 2011 at 09:20:14PM +0000, Éric Araujo wrote:
> your patch is not uncontroversial.
The code is very ugly, but i think that somewhat reflects 
the code flow of the entire function. 
At least a bit. 
I've forced all mis-uses i could imagine and the patch you see was 
the only solution to avoid the ResourceWarning for all of them. 
Do you disagree in avoiding that warning, or have i missed an 
error??
(But maybe this entire function should be cleaned up a bit to get 
rid of these interlocked try: blocks, which would make it easier 
to write the newline and also close the stream.)
> -1 on the second patch: there’s another issue for that
Well, ok about that.
However, msg128824 seems to indicate that you are willing to 
accept that termios.ISIG shall not be set. 
If you want to treat this as two commits then of course one of the 
patches (for #11236 and #11466) needs to be adjusted after the 
other has been patched in.
So, what is your suggestion? 
Shall i write a patch for #11236 which assumes that 
getpass_fdclose.patch has been integrated yet?

If you can write a patch that cleans up the code and closes the files without being too hard to review, it could get committed soon. Then you could adapt your patch on the other bug and defend it.

On Fri, Mar 11, 2011 at 09:51:32PM +0000, Éric Araujo wrote:
> If you can write a patch that cleans up the code and closes the 
> files without being too hard to review
I'll try to do so tomorrow.

This patch makes getpass.getpass() comply with the documented 
behaviour and fixes #11466. 
It will require no further adjustments for #11236 (except what 
valhallasw's patch does, of course). 
It applies cleanly to:
16:32 ~/src/cpython $ hg identify
ee259a4f3eee tip

Hello, Éric and Gregory, this patch also addresses the problem 
that 'one newline too much' may be written in case of errors. 
The problem is already present in the unpatched code, 
and i admit that 11466.3.patch doesn't fix it.
All of this is written under the assumption that i may touch only 
unix_getpass(), not the rest of this file; it would be easier if 
_raw_input() would take a terminal_setup=False argument and 
encapsulate echoing of a final newline ...
About security: i think that you, Éric, have referred to this 
when you've said "your patch is not uncontroversial". 
There is http://mail.python.org/pipermail/python-dev/2003-December/040579.html, 
and, after looking into OpenBSD:lib/libc/gen/readpassphrase.c, 
i must admit that it would possibly be much better to use a native 
getpass(3) implementation if one is available.
(OpenBSD's getpass() *does not* set ISIG, it just takes care about 
signals and re-kill(2)s them as necessary; it restarts the entire 
getpass() cycle if it's TSTP/TTIN/TTOU and re-kill(2) returns. 
But this belongs to #11236, i guess.)
The mail on #dev is more than seven years old, however, and still 
this getpass.getpass() uses it's naive (compared to OpenBSD, say) 
approach. And that does not get worse with my patch in the end.
I also want to note that getpass.getpass() may throw IOError 
undocumented, with and without 11466.4.patch applied; 
it does so cleanly upon a49bda5ff3d5. 
And finally i am thankful for all the feedback i can get.

> About security: i think that you, Éric, have referred to this
> when you've said "your patch is not uncontroversial".
No, I was only referring to the fact that one unrelated change was present in a patch while it was still being discussed in another issue ("This new patch (11466.2.patch) also includes the #11236 patch").
> it would be easier if _raw_input() would take a terminal_setup=False argument
> and encapsulate echoing of a final newline
It’s a private function, it that makes the patch smaller let’s change it. The current patch looks rather complicated just to close a file object.

On Sat, Mar 19, 2011 at 01:29:28PM +0000, Éric Araujo wrote:
> It’s a private function, it that makes the patch smaller let’s change it.
You get a new patch from me tomorrow evening at the latest

On Sat, Mar 19, 2011 at 01:29:28PM +0000, Éric Araujo wrote:
> It’s a private function, if that makes the patch smaller let’s change it.
The promised 11466.5.patch. It:
 - Fixes #11466 resource warning.
 - Fixes bogus newline which would have been written before 
 in case the fallback implementation needs to be used.
 + _raw_input() has been renamed to _user_input() because that 
 is what it actually does (wether it's raw depends on caller). 
 It will now encapsulate the complete user prompting, 
 thus including the mentioned final newline.
 - Allows patch-in of #11236 patch without any further 
 adjustments (i.e. no additional catch of another resource 
 warning necessary).
 - It cleans up the control flow and the comments a bit which 
 i think was the reason that the first two items above could 
 actually become introduced in the code at all.
But - it's even larger than 11466.4.patch!

On Thu, Mar 24, 2011 at 02:34:34PM +0000, Senthil Kumaran wrote:
> assignee: -> orsenthil
Here is yet another patch which only passes valid streams into 
_user_input(), so that this does not need to take care about that 
at all. 
Would you please review that instead of all the others ;/? 
(It's identical to .5. beside that.) 
Thanks for looking at this issue!

Future Buddha to Guru..
Future Buddha to Guru..
My code is like a two-wheeled indian Bullet, royal purple.
Wouldn't you agree with that?

The issue is still there. I hope someone fixes it before the release.

I think I've found the root cause.
On my system (also tested in 3.2) /dev/tty is opened successfully, but wrapping it in io.BufferedRandom fails. By the time the exception is raised, FileIO object is already created, and then it immediately gets deleted.
I'm attaching a patch that closes the file explicitly in this case.
Be extra careful when reviewing though - this is the first time I'm touching Python's C code.

This was fixed in https://github.com/python/cpython/commit/16dbbae2981c96c7c9b1ae81e1708d54b08c10ac
Since Python 3.4
And tests do not raise any ResourceWarning now.
```
$ ../../python -Vs
Python 3.11.0a0
$ ../../python -m unittest test_getpass.py -v
test_username_falls_back_to_pwd (test_getpass.GetpassGetuserTest) ... ok
test_username_priorities_of_env_values (test_getpass.GetpassGetuserTest) ... ok
test_username_takes_username_from_env (test_getpass.GetpassGetuserTest) ... ok
test_flushes_stream_after_prompt (test_getpass.GetpassRawinputTest) ... ok
test_raises_on_empty_input (test_getpass.GetpassRawinputTest) ... ok
test_trims_trailing_newline (test_getpass.GetpassRawinputTest) ... ok
test_uses_stderr_as_default (test_getpass.GetpassRawinputTest) ... ok
test_uses_stdin_as_default_input (test_getpass.GetpassRawinputTest) ... ok
test_uses_stdin_as_different_locale (test_getpass.GetpassRawinputTest) ... ok
test_falls_back_to_fallback_if_termios_raises (test_getpass.UnixGetpassTest) ... ok
test_falls_back_to_stdin (test_getpass.UnixGetpassTest) ... ok
test_flushes_stream_after_input (test_getpass.UnixGetpassTest) ... ok
test_resets_termios (test_getpass.UnixGetpassTest) ... ok
test_uses_tty_directly (test_getpass.UnixGetpassTest) ... ok
----------------------------------------------------------------------
Ran 14 tests in 0.041s
OK
```