classification
Title: subprocess: file descriptors should be closed after preexec_fn is called
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.3, Python 3.4, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: gregory.p.smith, neologix, pitrou, python-dev, sbt, vstinner
Priority: normal Keywords: needs review, patch

Created on 2013-08-16 22:23 by neologix, last changed 2013-08-27 06:19 by gregory.p.smith. This issue is now closed.

Files
File name Uploaded Description Edit
subprocess_close.diff neologix, 2013-08-17 18:55 review
subprocess_close-default.diff neologix, 2013-08-22 21:23 review
subprocess_close-27.diff neologix, 2013-08-22 21:23 review
subprocess_close-default-1.diff neologix, 2013-08-23 17:46 review
Messages (16)
msg195434 - (view) Author: Charles-François Natali (neologix) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2013-08-17 18:55
With patch :)
msg195828 - (view) Author: STINNER Victor (vstinner) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2013-08-23 21:51
Close_fds is not supposed to close them?
msg196043 - (view) Author: Charles-François Natali (neologix) * (Python committer) 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) * (Python committer) 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) (Python triager) 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) * (Python committer) Date: 2013-08-27 06:19
Thanks!  Backported to subprocess32 in https://code.google.com/p/python-subprocess32/source/detail?r=4ba30d9c64296ea0d2959790ab22d0f1a2678064
History
Date User Action Args
2013-08-27 06:19:52gregory.p.smithsetmessages: + msg196263
2013-08-25 18:43:49neologixsetstatus: open -> closed
stage: commit review -> resolved
resolution: fixed
versions: + Python 2.7, Python 3.3
2013-08-25 16:29:41python-devsetnosy: + python-dev
messages: + msg196152
2013-08-23 22:02:35vstinnersetmessages: + msg196048
2013-08-23 21:58:38neologixsetmessages: + msg196043
2013-08-23 21:51:09vstinnersetmessages: + msg196040
2013-08-23 20:33:13neologixsetmessages: + msg196023
2013-08-23 18:54:32vstinnersetmessages: + msg196001
2013-08-23 18:48:54neologixsetmessages: + msg196000
2013-08-23 17:57:32vstinnersetmessages: + msg195995
2013-08-23 17:46:27neologixsetfiles: + subprocess_close-default-1.diff

messages: + msg195994
stage: patch review -> commit review
2013-08-22 21:23:53neologixsetfiles: + subprocess_close-default.diff, subprocess_close-27.diff

messages: + msg195923
2013-08-21 22:26:45vstinnersetmessages: + msg195829
2013-08-21 22:26:12vstinnersetnosy: + vstinner
messages: + msg195828
2013-08-21 17:16:35neologixsetnosy: + sbt
2013-08-17 18:55:52neologixsetfiles: + subprocess_close.diff

messages: + msg195504
2013-08-16 23:37:03gregory.p.smithsetmessages: + msg195442
2013-08-16 22:23:55neologixcreate