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) * |
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) * |
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) * |
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.
|
msg393153 - (view) |
Author: Senthil Kumaran (orsenthil) * |
Date: 2021-05-06 22:34 |
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
```
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:14 | admin | set | github: 55675 |
2021-05-06 22:34:08 | orsenthil | set | status: open -> closed resolution: fixed messages:
+ msg393153
stage: resolved |
2012-07-24 14:26:36 | anton.barkovsky | set | files:
+ closewarning.patch |
2012-07-24 14:26:22 | anton.barkovsky | set | files:
- closewarning.patch |
2012-07-24 14:22:01 | anton.barkovsky | set | files:
+ closewarning.patch
messages:
+ msg166297 |
2012-07-23 17:28:55 | anton.barkovsky | set | nosy:
+ anton.barkovsky messages:
+ msg166240
|
2011-09-17 16:28:12 | sdaoden | set | nosy:
- sdaoden
|
2011-04-08 19:55:52 | sdaoden | set | messages:
+ msg133337 |
2011-03-24 14:54:13 | sdaoden | set | files:
+ 11466.6.patch
messages:
+ msg131980 |
2011-03-24 14:34:34 | orsenthil | set | assignee: orsenthil
nosy:
+ orsenthil |
2011-03-19 22:40:58 | sdaoden | set | files:
+ 11466.5.patch
messages:
+ msg131453 |
2011-03-19 13:50:47 | sdaoden | set | messages:
+ msg131416 |
2011-03-19 13:29:27 | eric.araujo | set | messages:
+ msg131412 |
2011-03-14 14:21:39 | sdaoden | set | files:
+ 11466.4.patch
messages:
+ msg130816 |
2011-03-12 15:34:45 | sdaoden | set | files:
+ 11466.3.patch
messages:
+ msg130685 |
2011-03-11 21:56:47 | sdaoden | set | messages:
+ msg130627 |
2011-03-11 21:51:30 | eric.araujo | set | messages:
+ msg130625 |
2011-03-11 21:47:25 | sdaoden | set | messages:
+ msg130620 |
2011-03-11 21:20:13 | eric.araujo | set | files:
+ getpass_fdclose.patch
messages:
+ msg130614 |
2011-03-11 21:06:39 | eric.araujo | set | files:
- getpass_fdclose.patch |
2011-03-11 13:16:34 | sdaoden | set | files:
+ 11466.2.patch
messages:
+ msg130564 |
2011-03-11 12:21:12 | pitrou | set | nosy:
+ gregory.p.smith
|
2011-03-11 11:54:51 | sdaoden | create | |