[issue18763] subprocess: file descriptors should be closed after preexec_fn is called

[Gregory P. Smith]

Gregory P. Smith added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue18763] subprocess: file descriptors should be closed after preexec_fn is called

[Charles-François Natali]

Changes by Charles-François Natali :


--
resolution:  -> fixed
stage: commit review -> committed/rejected
status: open -> closed
versions: +Python 2.7, Python 3.3

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue18763] subprocess: file descriptors should be closed after preexec_fn is called

[Roundup Robot]

Roundup Robot added the comment:

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

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue18763] subprocess: file descriptors should be closed after preexec_fn is called

[STINNER Victor]

STINNER Victor added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue18763] subprocess: file descriptors should be closed after preexec_fn is called

[Charles-François Natali]

Charles-François Natali added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue18763] subprocess: file descriptors should be closed after preexec_fn is called

[STINNER Victor]

STINNER Victor added the comment:

Close_fds is not supposed to close them?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue18763] subprocess: file descriptors should be closed after preexec_fn is called

[Charles-François Natali]

Charles-François Natali added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue18763] subprocess: file descriptors should be closed after preexec_fn is called

[STINNER Victor]

STINNER Victor added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue18763] subprocess: file descriptors should be closed after preexec_fn is called

[Charles-François Natali]

Charles-François Natali added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue18763] subprocess: file descriptors should be closed after preexec_fn is called

[STINNER Victor]

STINNER Victor added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue18763] subprocess: file descriptors should be closed after preexec_fn is called

[Charles-François Natali]

Charles-François Natali added the comment:

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.

--
stage: patch review -> commit review
Added file: http://bugs.python.org/file31443/subprocess_close-default-1.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue18763] subprocess: file descriptors should be closed after preexec_fn is called

[Charles-François Natali]

Charles-François Natali added the comment:

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

--
Added file: http://bugs.python.org/file31424/subprocess_close-default.diff
Added file: http://bugs.python.org/file31425/subprocess_close-27.diff

___
Python tracker 

___diff -r 49e23a3adf26 Lib/test/test_subprocess.py
--- a/Lib/test/test_subprocess.py   Wed Aug 21 13:26:34 2013 +0200
+++ b/Lib/test/test_subprocess.py   Thu Aug 22 21:19:52 2013 +0200
@@ -1972,6 +1972,19 @@
 self.assertRaises(OSError, os.waitpid, pid, 0)
 self.assertNotIn(ident, [id(o) for o in subprocess._active])
 
+def test_close_fds_after_preexec(self):
+fd_status = support.findfile("fd_status.py", subdir="subprocessdata")
+
+# FDs created by preexec_fn should be closed in the child
+p = subprocess.Popen([sys.executable, fd_status],
+ stdout=subprocess.PIPE, close_fds=True,
+ preexec_fn=os.pipe)
+output, ignored = p.communicate()
+
+remaining_fds = set(map(int, output.split(b',')))
+
+self.assertLessEqual(remaining_fds, set([0, 1, 2]))
+
 
 @unittest.skipUnless(mswindows, "Windows specific tests")
 class Win32ProcessTestCase(BaseTestCase):
diff -r 49e23a3adf26 Modules/_posixsubprocess.c
--- a/Modules/_posixsubprocess.cWed Aug 21 13:26:34 2013 +0200
+++ b/Modules/_posixsubprocess.cThu Aug 22 21:19:52 2013 +0200
@@ -412,17 +412,6 @@
 POSIX_CALL(close(errwrite));
 }
 
-if (close_fds) {
-int local_max_fd = max_fd;
-#if defined(__NetBSD__)
-local_max_fd = fcntl(0, F_MAXFD);
-if (local_max_fd < 0)
-local_max_fd = max_fd;
-#endif
-/* TODO HP-UX could use pstat_getproc() if anyone cares about it. */
-_close_open_fd_range(3, local_max_fd, py_fds_to_keep);
-}
-
 if (cwd)
 POSIX_CALL(chdir(cwd));
 
@@ -451,6 +440,18 @@
 /* Py_DECREF(result); - We're about to exec so why bother? */
 }
 
+/* close FDs after executing preexec_fn, which might open FDs */
+if (close_fds) {
+int local_max_fd = max_fd;
+#if defined(__NetBSD__)
+local_max_fd = fcntl(0, F_MAXFD);
+if (local_max_fd < 0)
+local_max_fd = max_fd;
+#endif
+/* TODO HP-UX could use pstat_getproc() if anyone cares about it. */
+_close_open_fd_range(3, local_max_fd, py_fds_to_keep);
+}
+
 /* This loop matches the Lib/os.py _execvpe()'s PATH search when */
 /* given the executable_list generated by Lib/subprocess.py. */
 saved_errno = 0;
diff -r 84d74eb7a341 Lib/subprocess.py
--- a/Lib/subprocess.py Wed Aug 21 18:25:00 2013 +0200
+++ b/Lib/subprocess.py Thu Aug 22 23:20:40 2013 +0200
@@ -1247,16 +1247,17 @@
 os.close(fd)
 closed.add(fd)
 
-# Close all other fds, if asked for
-if close_fds:
-self._close_fds(but=errpipe_write)
-
 if cwd is not None:
 os.chdir(cwd)
 
 if preexec_fn:
 preexec_fn()
 
+# Close all other fds, if asked for - after
+# preexec_fn(), which might open FDs
+if close_fds:
+self._close_fds(but=errpipe_write)
+
 if env is None:
 os.execvp(executable, args)
 else:
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue18763] subprocess: file descriptors should be closed after preexec_fn is called

[STINNER Victor]

STINNER Victor added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue18763] subprocess: file descriptors should be closed after preexec_fn is called

[STINNER Victor]

STINNER Victor added the comment:

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

--
nosy: +haypo

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue18763] subprocess: file descriptors should be closed after preexec_fn is called

[Charles-François Natali]

Changes by Charles-François Natali :


--
nosy: +sbt

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue18763] subprocess: file descriptors should be closed after preexec_fn is called

[Charles-François Natali]

Charles-François Natali added the comment:

With patch :)

--
Added file: http://bugs.python.org/file31342/subprocess_close.diff

___
Python tracker 

___diff -r 5d4fe1da2c90 Lib/test/test_subprocess.py
--- a/Lib/test/test_subprocess.py   Fri Aug 16 23:19:56 2013 +0200
+++ b/Lib/test/test_subprocess.py   Sat Aug 17 20:53:01 2013 +0200
@@ -1972,6 +1972,19 @@
 self.assertRaises(OSError, os.waitpid, pid, 0)
 self.assertNotIn(ident, [id(o) for o in subprocess._active])
 
+def test_close_fds_after_preexec(self):
+fd_status = support.findfile("fd_status.py", subdir="subprocessdata")
+
+# FDs created by preexec_fn should be closed in the child
+p = subprocess.Popen([sys.executable, fd_status],
+ stdout=subprocess.PIPE, close_fds=True,
+ preexec_fn=os.pipe)
+output, ignored = p.communicate()
+
+remaining_fds = set(map(int, output.split(b',')))
+
+self.assertTrue(remaining_fds <= set([0, 1, 2]))
+
 
 @unittest.skipUnless(mswindows, "Windows specific tests")
 class Win32ProcessTestCase(BaseTestCase):
diff -r 5d4fe1da2c90 Modules/_posixsubprocess.c
--- a/Modules/_posixsubprocess.cFri Aug 16 23:19:56 2013 +0200
+++ b/Modules/_posixsubprocess.cSat Aug 17 20:53:01 2013 +0200
@@ -412,17 +412,6 @@
 POSIX_CALL(close(errwrite));
 }
 
-if (close_fds) {
-int local_max_fd = max_fd;
-#if defined(__NetBSD__)
-local_max_fd = fcntl(0, F_MAXFD);
-if (local_max_fd < 0)
-local_max_fd = max_fd;
-#endif
-/* TODO HP-UX could use pstat_getproc() if anyone cares about it. */
-_close_open_fd_range(3, local_max_fd, py_fds_to_keep);
-}
-
 if (cwd)
 POSIX_CALL(chdir(cwd));
 
@@ -451,6 +440,18 @@
 /* Py_DECREF(result); - We're about to exec so why bother? */
 }
 
+/* close FDs after executing preexec_fn, which might open FDs */
+if (close_fds) {
+int local_max_fd = max_fd;
+#if defined(__NetBSD__)
+local_max_fd = fcntl(0, F_MAXFD);
+if (local_max_fd < 0)
+local_max_fd = max_fd;
+#endif
+/* TODO HP-UX could use pstat_getproc() if anyone cares about it. */
+_close_open_fd_range(3, local_max_fd, py_fds_to_keep);
+}
+
 /* This loop matches the Lib/os.py _execvpe()'s PATH search when */
 /* given the executable_list generated by Lib/subprocess.py. */
 saved_errno = 0;
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue18763] subprocess: file descriptors should be closed after preexec_fn is called

[Gregory P. Smith]

Gregory P. Smith added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue18763] subprocess: file descriptors should be closed after preexec_fn is called

[Charles-François Natali]

New submission from Charles-François Natali:

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.

--
components: Library (Lib)
keywords: needs review, patch
messages: 195434
nosy: gregory.p.smith, neologix, pitrou
priority: normal
severity: normal
stage: patch review
status: open
title: subprocess: file descriptors should be closed after preexec_fn is called
type: behavior
versions: Python 3.4

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com