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.

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. :)

With patch :)

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

You don't want to fix Python 2.7 and 3.3? It is a bug in my opinion.

> 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).

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.

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!?

> 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.

"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})

> 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.

Close_fds is not supposed to close them?

> 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).

> 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.

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

Thanks!  Backported to subprocess32 in https://code.google.com/p/python-subprocess32/source/detail?r=4ba30d9c64296ea0d2959790ab22d0f1a2678064