msg195434 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2013-08-16 22:23 |
Currently, when passed close_fds=True, the subprocess module closes FDs before calling preexec_fn (if provided). This can be an issue if preexec_fn opens some file descriptors, which would then be inherited in the child process.
Here's a patch with test.
|
msg195442 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2013-08-16 23:37 |
I don't see a patch attached, but I do not recall any good reason off the top of my head for preexec_fn to be called as late as it is. Moving it up to be called before the fd closing loop makes sense as a bug fix.
All bets are off when it comes to safe behavior for preexec_fn users anyways. :)
|
msg195504 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2013-08-17 18:55 |
With patch :)
|
msg195828 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2013-08-21 22:26 |
Your change looks perfectly valid, except this minor nit:
+ self.assertTrue(remaining_fds <= set([0, 1, 2]))
Using self.assertLessEqual() would provide better message on error.
Note: with the PEP 446, you would not have to care of closing file descriptors :-D
|
msg195829 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2013-08-21 22:26 |
You don't want to fix Python 2.7 and 3.3? It is a bug in my opinion.
|
msg195923 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2013-08-22 21:23 |
> Using self.assertLessEqual() would provide better message on error.
Done.
> You don't want to fix Python 2.7 and 3.3? It is a bug in my opinion.
Patch for 2.7 attached (without test though, because 2.7 lacks a
fd_status.py helper, which would make this quite complicated).
|
msg195994 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2013-08-23 17:46 |
Here's a patch with a more robust test: the previous test worked, but assume that only stdin, stdout and stderr FDs would be open in the child process. This might not hold anymore in a near future (/dev/urandom persistent FD).
The new test is much more robust.
|
msg195995 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2013-08-23 17:57 |
I don't understand why os.dup2() is more reliable than os.pipe() for a unit test?
Is subprocess_close-default-1.diff portable? test_os uses hasattr(os, "dup2"). In Modules/posixmodule.c, it looks like the function is always defined!?
|
msg196000 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2013-08-23 18:48 |
> I don't understand why os.dup2() is more reliable than os.pipe() for a unit
> test?
I use dup2() because it allows me to specify a target FD, so the
parent can know precisely which FD was opened by the preexec hook, and
check it's closed in the child process. With pipe(), the FDs returned
are arbitrary, so the parent can't check them explicitly, and has to
check that no FD > 2 is open in the child, which is fragile.
> Is subprocess_close-default-1.diff portable? test_os uses hasattr(os,
> "dup2"). In Modules/posixmodule.c, it looks like the function is always
> defined!?
Maybe it's not available on Windows, but it's definitely available on Unix.
|
msg196001 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2013-08-23 18:54 |
"the parent can know precisely which FD was opened by
the preexec hook, and check it's closed in the child process."
Oh ok, I see:
+ self.assertNotIn(fd, remaining_fds)
You might also add a check:
self.assertLessEqual(remaining_fds, {0, 1, 2})
|
msg196023 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2013-08-23 20:33 |
> You might also add a check:
>
> self.assertLessEqual(remaining_fds, {0, 1, 2})
Well no, because the interpreter might have other FDs open than stdin,
stdout and stderr.
|
msg196040 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2013-08-23 21:51 |
Close_fds is not supposed to close them?
|
msg196043 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2013-08-23 21:58 |
> Close_fds is not supposed to close them?
Yes, but some new fds might be open in the child process, e.g. for /dev/urandom.
That's why it's better to check that this precise FD is closed. Of
course, if the /dev/urandom FD gets assigned the same FD as the one
provided by dup() in the parent, we're screwed, but that shouldn't
happen (since /dev/urandom is already open in the parent).
|
msg196048 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2013-08-23 22:02 |
> Yes, but some new fds might be open in the child process, e.g. for /dev/urandom.
The issue #18756 is not implemented not, and I'm still opposed to change os.random() to use persistent FD :)
Ok, no problem for not checking others FD, it's not the purpose of the test anyway.
|
msg196152 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2013-08-25 16:29 |
New changeset f2b023135b1b by Charles-François Natali in branch '2.7':
Issue #18763: subprocess: The file descriptors are now closed after calling the
http://hg.python.org/cpython/rev/f2b023135b1b
New changeset 4c52d4bac03c by Charles-François Natali in branch '3.3':
Issue #18763: subprocess: The file descriptors are now closed after calling the
http://hg.python.org/cpython/rev/4c52d4bac03c
New changeset 748a5d39e7ce by Charles-François Natali in branch 'default':
Issue #18763: subprocess: The file descriptors are now closed after calling the
http://hg.python.org/cpython/rev/748a5d39e7ce
|
msg196263 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2013-08-27 06:19 |
Thanks! Backported to subprocess32 in https://code.google.com/p/python-subprocess32/source/detail?r=4ba30d9c64296ea0d2959790ab22d0f1a2678064
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:49 | admin | set | github: 62963 |
2013-08-27 06:19:52 | gregory.p.smith | set | messages:
+ msg196263 |
2013-08-25 18:43:49 | neologix | set | status: open -> closed stage: commit review -> resolved resolution: fixed versions:
+ Python 2.7, Python 3.3 |
2013-08-25 16:29:41 | python-dev | set | nosy:
+ python-dev messages:
+ msg196152
|
2013-08-23 22:02:35 | vstinner | set | messages:
+ msg196048 |
2013-08-23 21:58:38 | neologix | set | messages:
+ msg196043 |
2013-08-23 21:51:09 | vstinner | set | messages:
+ msg196040 |
2013-08-23 20:33:13 | neologix | set | messages:
+ msg196023 |
2013-08-23 18:54:32 | vstinner | set | messages:
+ msg196001 |
2013-08-23 18:48:54 | neologix | set | messages:
+ msg196000 |
2013-08-23 17:57:32 | vstinner | set | messages:
+ msg195995 |
2013-08-23 17:46:27 | neologix | set | files:
+ subprocess_close-default-1.diff
messages:
+ msg195994 stage: patch review -> commit review |
2013-08-22 21:23:53 | neologix | set | files:
+ subprocess_close-default.diff, subprocess_close-27.diff
messages:
+ msg195923 |
2013-08-21 22:26:45 | vstinner | set | messages:
+ msg195829 |
2013-08-21 22:26:12 | vstinner | set | nosy:
+ vstinner messages:
+ msg195828
|
2013-08-21 17:16:35 | neologix | set | nosy:
+ sbt
|
2013-08-17 18:55:52 | neologix | set | files:
+ subprocess_close.diff
messages:
+ msg195504 |
2013-08-16 23:37:03 | gregory.p.smith | set | messages:
+ msg195442 |
2013-08-16 22:23:55 | neologix | create | |