classification
Title: subprocess.Popen() fails if 0, 1 or 2 descriptor is closed
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.4, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: Arfrever, asvetlov, cvrebert, ezio.melotti, gregory.p.smith, haypo, pitrou, python-dev, rosslagerwall, sarum9in, sbt
Priority: normal Keywords: patch

Created on 2012-08-28 13:03 by sarum9in, last changed 2013-12-02 04:02 by gregory.p.smith. This issue is now closed.

Files
File name Uploaded Description Edit
issue15798.patch rosslagerwall, 2012-08-29 09:20
issue15798-fix-gps01.diff gregory.p.smith, 2012-08-29 18:21 review
issue15798_v2.patch rosslagerwall, 2012-08-30 06:26
issue15798_alternate-pass_fds-gps01.diff gregory.p.smith, 2013-11-25 08:29 review
Messages (18)
msg169276 - (view) Author: Aleksey Filippov (sarum9in) Date: 2012-08-28 13:03
System info:
kernel: 3.4.8-1-ARCH
dist: Arch linux
python: 3.2.3

subprocess.Popen() fails if python interpreter is started with closed 0, 1 or 2 descriptor.

Traceback (most recent call last):
  File "<string>", line 14, in <module>
  File "/usr/lib/python3.2/subprocess.py", line 745, in __init__
    restore_signals, start_new_session)
  File "/usr/lib/python3.2/subprocess.py", line 1197, in _execute_child
    restore_signals, start_new_session, preexec_fn)
ValueError: errpipe_write must be >= 3
msg169277 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2012-08-28 13:12
#10806 seems related.
msg169344 - (view) Author: Ross Lagerwall (rosslagerwall) (Python committer) Date: 2012-08-29 06:58
It's caused by the following check in _posixsubprocess.c:
    if (close_fds && errpipe_write < 3) {  /* precondition */
        PyErr_SetString(PyExc_ValueError, "errpipe_write must be >= 3");
        return NULL;
    }

which was written by Gregory P. Smith in 2010 (adding to nosy list).

I'm not entirely sure why this check is here, presumably its due to the way close_fds=True is handled.

The close fds logic is also hardcoded to close fds from 3 upwards,.
msg169350 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-08-29 08:32
easy enough to reproduce...

$ ./python.exe -c 'import os, subprocess as s; os.close(0); os.close(1); s.Popen(["/bin/true"])'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/gps/python/hg/default/Lib/subprocess.py", line 818, in __init__
    restore_signals, start_new_session)
  File "/Users/gps/python/hg/default/Lib/subprocess.py", line 1363, in _execute_child
    restore_signals, start_new_session, preexec_fn)
ValueError: errpipe_write must be >= 3

Examining the code, it looks like that restriction is to prevent the dup2's for any passed in stdin, stdout or stderr pipes from overwriting errpipe_write in Modules/_posixsubprocess.c's child_exec() function.

First guess at a fix: child_exec() needs to detect this situation and dup(errpipe_write) until it gets a fd not in the 0..2 range before the dup2(...) calls that could otherwise blindly clobber it.  This could possibly be done by the parent process's _create_pipe() in Lib/subprocess.py when allocating the errpipe_read and errpipe_write fds.

Suggested Workaround: for now for any code running into this (Python daemons launching subprocesses?) - Call os.pipe() twice at the start of your program to burn 4 fds.  That'll guarantee 0, 1 and 2 will not be used for this pipe.
msg169353 - (view) Author: Ross Lagerwall (rosslagerwall) (Python committer) Date: 2012-08-29 09:20
The attached patch + test seems to fix the issue.

It's not very elegant.
msg169393 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-08-29 16:34
Yes, something along the lines of that patch is what I was thinking.  BTW, this is only necessary for the errpipe_write fd.  errpipe_read is for the parent process.

I'm going to do it within _create_pipe so that the optimal _posixsubprocess.cloexec_pipe pipe2() based implementation can be used when possible rather than needing to call _set_cloexec() on the dup'ed fd.

There are some recent Linux specific possibilities such as fcntl with F_DUPFD, or better F_DUPFD_CLOEXEC, that would make this a single call. Using that may be overkill for this situation but it looks easy enough while I'm in there.
msg169401 - (view) Author: Aleksey Filippov (sarum9in) Date: 2012-08-29 17:48
It may be implemented simplier.
fcntl(fd, F_DUPFD_CLOEXEC, min_fd_number) will allocate new fd at least min_fd_number. So, it is not necessary to do a lot of dup() calls.
msg169402 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-08-29 17:51
F_DUPFD_CLOEXEC appears exclusive to modern Linux kernels.  Any idea how wide spread support for plain F_DUPFD is?  If that is "everywhere" the code I've just whipped up could lose a lot of loops...
msg169405 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-08-29 18:21
Here's my initial fix.

If fcntl(errpipe_write, F_DUPFD, 3) is widely available this could be shrunk a bit to avoid the for loop potentially calling dup a few times and tracking the wasted fds to close later.

Otherwise if it isn't I'd rather not bother with F_DUPFD as this code takes the optimal path when F_DUPFD_CLOEXEC is supported (true on all modern linux systems) and should cause no harm beyond a couple extra dup and close syscalls otherwise.

Note: Linux pipe2() support appears in kernels a few revisions after F_DUPFD_CLOEXEC support so the good behavior when pipe2 exists will be kept in this situation as that also means F_DUPFD_CLOEXEC support should exist.  The code will work regardless of that (incase someone has a frankenkernel, or other OSes choose to implement one but not the other).
msg169429 - (view) Author: Ross Lagerwall (rosslagerwall) (Python committer) Date: 2012-08-30 06:26
I sent a review through on rietveld; I'm attaching a patch with the changes so that it compiles and passes the tests.
msg169447 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-08-30 10:36
I haven't tested Ross's latest patch, but it looks ok to me.
msg169449 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-08-30 11:08
Would it simplify matters to stop treating 0,1,2 specially and just add them to pass_fds instead?
msg204308 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2013-11-25 08:29
adding {0,1,2} to fds_to_keep (populated from pass_fds) is indeed an alternate approach.  A variant of an alternate patch doing that attached.

This actually simplifies code.  Is there anything this would hurt that i'm not seeing?

I suppose it adds minor overhead to the fork_exec() call by passing more in and lookups into the fds_to_keep list now that it will always contain at least 4 values instead of the previous 1.  I doubt that is matters (I haven't measured anything).
msg204879 - (view) Author: Roundup Robot (python-dev) Date: 2013-12-01 03:04
New changeset c4cd891cf167 by Gregory P. Smith in branch '3.3':
Fixes Issue #15798 - subprocess.Popen() no longer fails if file
http://hg.python.org/cpython/rev/c4cd891cf167

New changeset 0387054b2038 by Gregory P. Smith in branch 'default':
Fixes Issue #15798 - subprocess.Popen() no longer fails if file
http://hg.python.org/cpython/rev/0387054b2038
msg204889 - (view) Author: Roundup Robot (python-dev) Date: 2013-12-01 08:13
New changeset efcdf2a70f2a by Gregory P. Smith in branch '3.3':
Undo supposed fix for Issue #15798 until I understand why this is
http://hg.python.org/cpython/rev/efcdf2a70f2a

New changeset ddbf9632795b by Gregory P. Smith in branch 'default':
Undo supposed fix for Issue #15798 until I understand why this is
http://hg.python.org/cpython/rev/ddbf9632795b
msg204979 - (view) Author: Roundup Robot (python-dev) Date: 2013-12-02 00:03
New changeset 2df5e1f537b0 by Gregory P. Smith in branch 'default':
Fixes issue #15798: subprocess.Popen() no longer fails if file
http://hg.python.org/cpython/rev/2df5e1f537b0
msg204987 - (view) Author: Roundup Robot (python-dev) Date: 2013-12-02 01:28
New changeset 07425df887b5 by Gregory P. Smith in branch '3.3':
Fixes issue #15798: subprocess.Popen() no longer fails if file
http://hg.python.org/cpython/rev/07425df887b5
msg204990 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2013-12-02 04:02
i went with the less invasive in terms of behavior change approach of making sure that the errpipe_write fd is always >= 3.  In Python 3.4 the code change was different and much simpler and on the Python only side as all fd's are opened O_CLOEXEC by default.

I'm not entirely sure why the test_multiprocessing_forkserver and test_multiprocessing_spawn failures happened on 3.4 with the earlier change that attempted to always list 0,1,2 in the fds_to_keep (derived from pass_fds) list so there _may_ be a bug lurking elsewhere there but I suspect the bug is actually that we don't always want to blindly list them in fds_to_keep as some programs may have reused some of them for other non-stdio purposes but still want them closed.

The change has been backported to the python-subprocess32 repo.
History
Date User Action Args
2013-12-02 04:02:25gregory.p.smithsetstatus: open -> closed
resolution: fixed
messages: + msg204990

stage: commit review -> resolved
2013-12-02 01:28:39python-devsetmessages: + msg204987
2013-12-02 00:03:43python-devsetmessages: + msg204979
2013-12-01 08:16:17Arfreversetnosy: + Arfrever
2013-12-01 08:14:29gregory.p.smithsetstatus: closed -> open
resolution: fixed -> (no value)
stage: resolved -> commit review
2013-12-01 08:13:45python-devsetmessages: + msg204889
2013-12-01 03:05:15gregory.p.smithsetstatus: open -> closed
stage: patch review -> resolved
resolution: fixed
versions: + Python 3.4, - Python 3.2
2013-12-01 03:04:12python-devsetnosy: + python-dev
messages: + msg204879
2013-11-25 08:29:14gregory.p.smithsetfiles: + issue15798_alternate-pass_fds-gps01.diff

messages: + msg204308
2013-08-13 22:21:29hayposetnosy: + haypo
2012-08-30 11:08:02sbtsetnosy: + sbt
messages: + msg169449
2012-08-30 10:55:05asvetlovsetnosy: + asvetlov
2012-08-30 10:36:50pitrousetmessages: + msg169447
stage: patch review
2012-08-30 06:26:58rosslagerwallsetfiles: + issue15798_v2.patch

messages: + msg169429
2012-08-29 18:21:11gregory.p.smithsetfiles: + issue15798-fix-gps01.diff

messages: + msg169405
2012-08-29 17:51:48gregory.p.smithsetmessages: + msg169402
2012-08-29 17:48:17sarum9insetmessages: + msg169401
2012-08-29 16:34:44gregory.p.smithsetmessages: + msg169393
2012-08-29 09:20:16rosslagerwallsetfiles: + issue15798.patch
keywords: + patch
messages: + msg169353
2012-08-29 08:32:31gregory.p.smithsetassignee: gregory.p.smith
messages: + msg169350
versions: + Python 3.3
2012-08-29 08:00:14cvrebertsetnosy: + cvrebert
2012-08-29 06:58:21rosslagerwallsetnosy: + gregory.p.smith
messages: + msg169344
2012-08-28 13:12:44ezio.melottisetnosy: + ezio.melotti, rosslagerwall, pitrou
messages: + msg169277

components: + Library (Lib)
type: behavior
2012-08-28 13:03:49sarum9increate