Subprocess descriptor debacle

[tiran]

BPO	6610
Nosy	@birkenfeld, @gpshead, @pitrou, @tiran
Superseder	bpo-10806: Subprocess error if fds 0,1,2 are closed
Files	test_subprocess_with_standard_fds_closed.diff: subprocess test for a coprocess spawned when standard fds are closed

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = 'https://github.com/gpshead' closed_at = created_at = labels = ['easy', 'library', 'performance'] title = 'Subprocess descriptor debacle' updated_at = user = 'https://github.com/tiran' ``` bugs.python.org fields: ```python activity = actor = 'georg.brandl' assignee = 'gregory.p.smith' closed = True closed_date = closer = 'georg.brandl' components = ['Library (Lib)'] creation = creator = 'christian.heimes' dependencies = [] files = ['16801'] hgrepos = [] issue_num = 6610 keywords = ['patch', 'easy', 'needs review'] message_count = 7.0 messages = ['91118', '102536', '105623', '105707', '105774', '110941', '125210'] nosy_count = 7.0 nosy_names = ['georg.brandl', 'gregory.p.smith', 'astrand', 'pitrou', 'christian.heimes', 'Yaniv.Aknin', 'BreamoreBoy'] pr_nums = [] priority = 'normal' resolution = 'duplicate' stage = 'patch review' status = 'closed' superseder = '10806' type = 'resource usage' url = 'https://bugs.python.org/issue6610' versions = ['Python 2.6', 'Python 3.1', 'Python 2.7', 'Python 3.2'] ```

[tiran]

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.

[aa3203a7-2d25-4619-bcf7-1a8f18a9cd6a]

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.

[gpshead]

Thanks for the test! I'll take a look and likely commit this later.

[pitrou]

Would the test still be correct if it didn't close stderr? I feel closing stderr is very bad from a debuggability standpoint.

[aa3203a7-2d25-4619-bcf7-1a8f18a9cd6a]

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

[83d2e70e-e599-4a04-b820-3814bbdb9bef]

The patch is small, is there any solid reason for not committing it?

[birkenfeld]

Superseded by bpo-10806.