classification
Title: Subprocess descriptor debacle
Type: resource usage Stage: patch review
Components: Library (Lib) Versions: Python 3.1, Python 3.2, Python 2.7, Python 2.6
process
Status: closed Resolution: duplicate
Dependencies: Superseder: Subprocess error if fds 0,1,2 are closed
View: 10806
Assigned To: gregory.p.smith Nosy List: BreamoreBoy, Yaniv.Aknin, astrand, christian.heimes, georg.brandl, gregory.p.smith, pitrou
Priority: normal Keywords: easy, needs review, patch

Created on 2009-07-31 00:27 by christian.heimes, last changed 2011-01-03 18:31 by georg.brandl. This issue is now closed.

Files
File name Uploaded Description Edit
test_subprocess_with_standard_fds_closed.diff Yaniv.Aknin, 2010-04-07 13:08 subprocess test for a coprocess spawned when standard fds are closed review
Messages (7)
msg91118 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2009-07-31 00:27
The subprocess module may suffer from a minor design flaw that is
described at http://unixwiz.net/techtips/remap-pipe-fds.html under the
heading "Descriptor Debacle". The problem can occur under rare
conditions when a subprocess is created from a Python daemon process.

Proposed solution:

Create a function os.duprange(*args) that takes one or more tuple pairs
of file descriptors. The function takes care of the necessary order and
dubbing of fds.
msg102536 - (view) Author: Yaniv Aknin (Yaniv.Aknin) Date: 2010-04-07 13:08
It seems to me that subprocess is protected against this flaw. Python 2.x has a pure-Python implementation of the child logic (which is susceptible to an unrelated issue). Python 3.x has a C implementation which falls back to pure-Python if the former is not available.

Both implementations test explicitly that they're not closing standard file descriptors after the dup2() call in the child. It is my understanding that test is sufficient, I couldn't reproduce the bug in Python, and thus I think this issue should be closed.

All that said, there was no coverage in subprocess' test for this particular case (spawning a coprocess when the standard fds closed). I'm attaching a patch which adds a testcase to cover it.
msg105623 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2010-05-13 06:00
Thanks for the test!  I'll take a look and likely commit this later.
msg105707 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-05-14 13:35
Would the test still be correct if it didn't close stderr? I feel closing stderr is very bad from a debuggability standpoint.
msg105774 - (view) Author: Yaniv Aknin (Yaniv.Aknin) Date: 2010-05-14 22:16
I think if the test is conducted without closing stderr, it will only check that stdin/stdout are handled correctly (you could assume that if one handled stdin/stdout correctly, they did the same with stderr).

However, since I've used a context manager (_NoStandardFds) to handle the closing/restoration of the standard fds, I think the benefit (fuller test coverage) outweighs the cost (potentially harder debugging if there's a problem with the test); if I'm not mistaken, the context manager should restore your fds before the default exception handler writes to stdout (at least in the parent and the child prior to exec()).

n.b.: I've also created a Rietveld issue for this patch: http://codereview.appspot.com/1227041/show
msg110941 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-07-20 18:13
The patch is small, is there any solid reason for not committing it?
msg125210 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2011-01-03 18:31
Superseded by #10806.
History
Date User Action Args
2011-01-03 18:31:05georg.brandlsetstatus: open -> closed

nosy: + georg.brandl
messages: + msg125210

superseder: Subprocess error if fds 0,1,2 are closed
resolution: duplicate
2010-07-20 18:13:30BreamoreBoysetnosy: + BreamoreBoy
messages: + msg110941
2010-05-14 22:16:36Yaniv.Akninsetmessages: + msg105774
2010-05-14 13:35:38pitrousetmessages: + msg105707
2010-05-13 06:00:38gregory.p.smithsetassignee: gregory.p.smith
messages: + msg105623
2010-05-12 13:24:00pitrousetnosy: + gregory.p.smith
2010-05-12 12:55:37ezio.melottisetnosy: + astrand, pitrou
stage: needs patch -> patch review

versions: - Python 3.0
2010-04-07 13:08:52Yaniv.Akninsetfiles: + test_subprocess_with_standard_fds_closed.diff

nosy: + Yaniv.Aknin
messages: + msg102536

keywords: + patch
2009-07-31 00:27:03christian.heimescreate