classification
Title: getpass.getpass doesn't close tty file
Type: behavior Stage:
Components: Library (Lib) Versions: Python 3.3
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: orsenthil Nosy List: anton.barkovsky, eric.araujo, gregory.p.smith, orsenthil
Priority: normal Keywords: patch

Created on 2011-03-11 11:54 by sdaoden, last changed 2012-07-24 14:26 by anton.barkovsky.

Files
File name Uploaded Description Edit
11466.2.patch sdaoden, 2011-03-11 13:16
getpass_fdclose.patch eric.araujo, 2011-03-11 21:20
11466.3.patch sdaoden, 2011-03-12 15:34 review
11466.4.patch sdaoden, 2011-03-14 14:21 review
11466.5.patch sdaoden, 2011-03-19 22:40 review
11466.6.patch sdaoden, 2011-03-24 14:54 review
closewarning.patch anton.barkovsky, 2012-07-24 14:26 Close the file object explicitly if the wrapping fails review
Messages (15)
msg130560 - (view) Author: Steffen Daode Nurpmeso (sdaoden) Date: 2011-03-11 11:54
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.
msg130564 - (view) Author: Steffen Daode Nurpmeso (sdaoden) Date: 2011-03-11 13:16
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.
msg130614 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-03-11 21:20
Sorry, accidentally removed the first patch.

-1 on the second patch: there’s another issue for that, and your patch is not uncontroversial.
msg130620 - (view) Author: Steffen Daode Nurpmeso (sdaoden) Date: 2011-03-11 21:47
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?
msg130625 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-03-11 21:51
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.
msg130627 - (view) Author: Steffen Daode Nurpmeso (sdaoden) Date: 2011-03-11 21:56
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.
msg130685 - (view) Author: Steffen Daode Nurpmeso (sdaoden) Date: 2011-03-12 15:34
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
msg130816 - (view) Author: Steffen Daode Nurpmeso (sdaoden) Date: 2011-03-14 14:21
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.
msg131412 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-03-19 13:29
> 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.
msg131416 - (view) Author: Steffen Daode Nurpmeso (sdaoden) Date: 2011-03-19 13:50
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
msg131453 - (view) Author: Steffen Daode Nurpmeso (sdaoden) Date: 2011-03-19 22:40
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!
msg131980 - (view) Author: Steffen Daode Nurpmeso (sdaoden) Date: 2011-03-24 14:54
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!
msg133337 - (view) Author: Steffen Daode Nurpmeso (sdaoden) Date: 2011-04-08 19:55
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?
msg166240 - (view) Author: Anton Barkovsky (anton.barkovsky) * Date: 2012-07-23 17:28
The issue is still there. I hope someone fixes it before the release.
msg166297 - (view) Author: Anton Barkovsky (anton.barkovsky) * Date: 2012-07-24 14:22
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.
History
Date User Action Args
2012-07-24 14:26:36anton.barkovskysetfiles: + closewarning.patch
2012-07-24 14:26:22anton.barkovskysetfiles: - closewarning.patch
2012-07-24 14:22:01anton.barkovskysetfiles: + closewarning.patch

messages: + msg166297
2012-07-23 17:28:55anton.barkovskysetnosy: + anton.barkovsky
messages: + msg166240
2011-09-17 16:28:12sdaodensetnosy: - sdaoden
2011-04-08 19:55:52sdaodensetmessages: + msg133337
2011-03-24 14:54:13sdaodensetfiles: + 11466.6.patch

messages: + msg131980
2011-03-24 14:34:34orsenthilsetassignee: orsenthil

nosy: + orsenthil
2011-03-19 22:40:58sdaodensetfiles: + 11466.5.patch

messages: + msg131453
2011-03-19 13:50:47sdaodensetmessages: + msg131416
2011-03-19 13:29:27eric.araujosetmessages: + msg131412
2011-03-14 14:21:39sdaodensetfiles: + 11466.4.patch

messages: + msg130816
2011-03-12 15:34:45sdaodensetfiles: + 11466.3.patch

messages: + msg130685
2011-03-11 21:56:47sdaodensetmessages: + msg130627
2011-03-11 21:51:30eric.araujosetmessages: + msg130625
2011-03-11 21:47:25sdaodensetmessages: + msg130620
2011-03-11 21:20:13eric.araujosetfiles: + getpass_fdclose.patch

messages: + msg130614
2011-03-11 21:06:39eric.araujosetfiles: - getpass_fdclose.patch
2011-03-11 13:16:34sdaodensetfiles: + 11466.2.patch

messages: + msg130564
2011-03-11 12:21:12pitrousetnosy: + gregory.p.smith
2011-03-11 11:54:51sdaodencreate