* Replaces the internals of the subprocess module from fork through exec on

  POSIX systems with a C extension module.  This is required in order for
  the subprocess module to be made thread safe.

  The pure python implementation is retained so that it can continue to be
  used if for some reason the _posixsubprocess extension module is not
  available.

  The unittest executes tests on both code paths to guarantee compatibility.

* Moves PyLong_FromPid and PyLong_AsPid from posixmodule.c into longobject.h.

Code reviewed by jeffrey.yasskin at http://codereview.appspot.com/223077/show

Doc/library/subprocess.rst

@@ -28,7 +28,7 @@ Using the subprocess Module
 This module defines one class called :class:`Popen`:
 
 
-.. class:: Popen(args, bufsize=0, executable=None, stdin=None, stdout=None, stderr=None, preexec_fn=None, close_fds=False, shell=False, cwd=None, env=None, universal_newlines=False, startupinfo=None, creationflags=0)
+.. class:: Popen(args, bufsize=0, executable=None, stdin=None, stdout=None, stderr=None, preexec_fn=None, close_fds=False, shell=False, cwd=None, env=None, universal_newlines=False, startupinfo=None, creationflags=0, restore_signals=True, start_new_session=False)
 
    Arguments are:
 
@@ -41,7 +41,8 @@ This module defines one class called :class:`Popen`:
    name for the executing program in utilities such as :program:`ps`.
 
    On Unix, with *shell=False* (default): In this case, the Popen class uses
-   :meth:`os.execvp` to execute the child program. *args* should normally be a
+   :meth:`os.execvp` like behavior to execute the child program.
+   *args* should normally be a
    sequence.  If a string is specified for *args*, it will be used as the name
    or path of the program to execute; this will only work if the program is
    being given no arguments.
@@ -108,7 +109,23 @@ This module defines one class called :class:`Popen`:
    applications should be captured into the same file handle as for stdout.
 
    If *preexec_fn* is set to a callable object, this object will be called in the
-   child process just before the child is executed. (Unix only)
+   child process just before the child is executed.
+   (Unix only)
+
+   .. warning::
+
+      The *preexec_fn* parameter is not safe to use in the presence of threads
+      in your application.  The child process could deadlock before exec is
+      called.
+      If you must use it, keep it trivial!  Minimize the number of libraries
+      you call into.
+
+   .. note::
+
+      If you need to modify the environment for the child use the *env*
+      parameter rather than doing it in a *preexec_fn*.
+      The *start_new_session* parameter can take the place of a previously
+      common use of *preexec_fn* to call os.setsid() in the child.
 
    If *close_fds* is true, all file descriptors except :const:`0`, :const:`1` and
    :const:`2` will be closed before the child process is executed. (Unix only).
@@ -124,9 +141,23 @@ This module defines one class called :class:`Popen`:
    searching the executable, so you can't specify the program's path relative to
    *cwd*.
 
+   If *restore_signals* is True (the default) all signals that Python has set to
+   SIG_IGN are restored to SIG_DFL in the child process before the exec.
+   Currently this includes the SIGPIPE, SIGXFZ and SIGXFSZ signals.
+   (Unix only)
+
+   .. versionchanged:: 3.2
+      *restore_signals* was added.
+
+   If *start_new_session* is True the setsid() system call will be made in the
+   child process prior to the execution of the subprocess.  (Unix only)
+
+   .. versionchanged:: 3.2
+      *start_new_session* was added.
+
    If *env* is not ``None``, it must be a mapping that defines the environment
-   variables for the new process; these are used instead of inheriting the current
-   process' environment, which is the default behavior.
+   variables for the new process; these are used instead of the default
+   behavior of inheriting the current process' environment.
 
    .. note::
 

Include/abstract.h

@@ -1232,6 +1232,9 @@ PyAPI_FUNC(int) _PyObject_RealIsInstance(PyObject *inst, PyObject *cls);
 
 PyAPI_FUNC(int) _PyObject_RealIsSubclass(PyObject *derived, PyObject *cls);
 
+PyAPI_FUNC(char *const *) _PySequence_BytesToCharpArray(PyObject* self);
+
+PyAPI_FUNC(void) _Py_FreeCharPArray(char *const array[]);
 
 #ifdef __cplusplus
 }

Include/longobject.h

@@ -41,6 +41,23 @@ PyAPI_FUNC(PyObject *) PyLong_GetInfo(void);
 #define PyLong_AsSocket_t(fd) (SOCKET_T)PyLong_AsLongLong(fd)
 #endif
 
+/* Issue #1983: pid_t can be longer than a C long on some systems */
+#if !defined(SIZEOF_PID_T) || SIZEOF_PID_T == SIZEOF_INT
+#define PARSE_PID "i"
+#define PyLong_FromPid PyLong_FromLong
+#define PyLong_AsPid PyLong_AsLong
+#elif SIZEOF_PID_T == SIZEOF_LONG
+#define PARSE_PID "l"
+#define PyLong_FromPid PyLong_FromLong
+#define PyLong_AsPid PyLong_AsLong
+#elif defined(SIZEOF_LONG_LONG) && SIZEOF_PID_T == SIZEOF_LONG_LONG
+#define PARSE_PID "L"
+#define PyLong_FromPid PyLong_FromLongLong
+#define PyLong_AsPid PyLong_AsLongLong
+#else
+#error "sizeof(pid_t) is neither sizeof(int), sizeof(long) or sizeof(long long)"
+#endif /* SIZEOF_PID_T */
+
 /* For use by intobject.c only */
 PyAPI_DATA(unsigned char) _PyLong_DigitValue[256];
 

Include/pythonrun.h

@@ -81,6 +81,9 @@ PyAPI_FUNC(int) Py_AtExit(void (*func)(void));
 
 PyAPI_FUNC(void) Py_Exit(int);
 
+/* Restore signals that the interpreter has called SIG_IGN on to SIG_DFL. */
+PyAPI_FUNC(void) _Py_RestoreSignals(void);
+
 PyAPI_FUNC(int) Py_FdIsInteractive(FILE *, const char *);
 
 /* Bootstrap */

Lib/test/test_subprocess.py

@@ -568,12 +568,53 @@ class POSIXProcessTestCase(unittest.TestCase):
         self.assertFalse(subprocess._active, "subprocess._active not empty")
 
     def test_exceptions(self):
-        # caught & re-raised exceptions
-        with self.assertRaises(OSError) as c:
+        nonexistent_dir = "/_this/pa.th/does/not/exist"
+        try:
+            os.chdir(nonexistent_dir)
+        except OSError as e:
+            # This avoids hard coding the errno value or the OS perror()
+            # string and instead capture the exception that we want to see
+            # below for comparison.
+            desired_exception = e
+        else:
+            self.fail("chdir to nonexistant directory %s succeeded." %
+                      nonexistent_dir)
+
+        # Error in the child re-raised in the parent.
+        try:
             p = subprocess.Popen([sys.executable, "-c", ""],
-                                 cwd="/this/path/does/not/exist")
-        # The attribute child_traceback should contain "os.chdir" somewhere.
-        self.assertIn("os.chdir", c.exception.child_traceback)
+                                 cwd=nonexistent_dir)
+        except OSError as e:
+            # Test that the child process chdir failure actually makes
+            # it up to the parent process as the correct exception.
+            self.assertEqual(desired_exception.errno, e.errno)
+            self.assertEqual(desired_exception.strerror, e.strerror)
+        else:
+            self.fail("Expected OSError: %s" % desired_exception)
+
+    def test_restore_signals(self):
+        # Code coverage for both values of restore_signals to make sure it
+        # at least does not blow up.
+        # A test for behavior would be complex.  Contributions welcome.
+        subprocess.call([sys.executable, "-c", ""], restore_signals=True)
+        subprocess.call([sys.executable, "-c", ""], restore_signals=False)
+
+    def test_start_new_session(self):
+        # For code coverage of calling setsid().  We don't care if we get an
+        # EPERM error from it depending on the test execution environment, that
+        # still indicates that it was called.
+        try:
+            output = subprocess.check_output(
+                    [sys.executable, "-c",
+                     "import os; print(os.getpgid(os.getpid()))"],
+                    start_new_session=True)
+        except OSError as e:
+            if e.errno != errno.EPERM:
+                raise
+        else:
+            parent_pgid = os.getpgid(os.getpid())
+            child_pgid = int(output)
+            self.assertNotEqual(parent_pgid, child_pgid)
 
     def test_run_abort(self):
         # returncode handles signal termination
@@ -584,7 +625,8 @@ class POSIXProcessTestCase(unittest.TestCase):
         self.assertEqual(-p.returncode, signal.SIGABRT)
 
     def test_preexec(self):
-        # preexec function
+        # DISCLAIMER: Setting environment variables is *not* a good use
+        # of a preexec_fn.  This is merely a test.
         p = subprocess.Popen([sys.executable, "-c",
                               'import sys,os;'
                               'sys.stdout.write(os.getenv("FRUIT"))'],
@@ -592,6 +634,22 @@ class POSIXProcessTestCase(unittest.TestCase):
                              preexec_fn=lambda: os.putenv("FRUIT", "apple"))
         self.assertEqual(p.stdout.read(), b"apple")
 
+    def test_preexec_exception(self):
+        def raise_it():
+            raise ValueError("What if two swallows carried a coconut?")
+        try:
+            p = subprocess.Popen([sys.executable, "-c", ""],
+                                 preexec_fn=raise_it)
+        except RuntimeError as e:
+            self.assertTrue(
+                    subprocess._posixsubprocess,
+                    "Expected a ValueError from the preexec_fn")
+        except ValueError as e:
+            self.assertIn("coconut", e.args[0])
+        else:
+            self.fail("Exception raised by preexec_fn did not make it "
+                      "to the parent process.")
+
     def test_args_string(self):
         # args is a string
         fd, fname = mkstemp()
@@ -836,6 +894,20 @@ class ProcessTestCaseNoPoll(ProcessTestCase):
         ProcessTestCase.tearDown(self)
 
 
+@unittest.skipUnless(getattr(subprocess, '_posixsubprocess', False),
+                     "_posixsubprocess extension module not found.")
+class ProcessTestCasePOSIXPurePython(ProcessTestCase, POSIXProcessTestCase):
+    def setUp(self):
+        subprocess._posixsubprocess = None
+        ProcessTestCase.setUp(self)
+        POSIXProcessTestCase.setUp(self)
+
+    def tearDown(self):
+        subprocess._posixsubprocess = sys.modules['_posixsubprocess']
+        POSIXProcessTestCase.tearDown(self)
+        ProcessTestCase.tearDown(self)
+
+
 class HelperFunctionTests(unittest.TestCase):
     @unittest.skipIf(mswindows, "errno and EINTR make no sense on windows")
     def test_eintr_retry_call(self):
@@ -859,6 +931,7 @@ def test_main():
     unit_tests = (ProcessTestCase,
                   POSIXProcessTestCase,
                   Win32ProcessTestCase,
+                  ProcessTestCasePOSIXPurePython,
                   CommandTests,
                   ProcessTestCaseNoPoll,
                   HelperFunctionTests)

Modules/posixmodule.c

@@ -303,23 +303,6 @@ extern int lstat(const char *, struct stat *);
 #define WAIT_STATUS_INT(s) (s)
 #endif /* UNION_WAIT */
 
-/* Issue #1983: pid_t can be longer than a C long on some systems */
-#if !defined(SIZEOF_PID_T) || SIZEOF_PID_T == SIZEOF_INT
-#define PARSE_PID "i"
-#define PyLong_FromPid PyLong_FromLong
-#define PyLong_AsPid PyLong_AsLong
-#elif SIZEOF_PID_T == SIZEOF_LONG
-#define PARSE_PID "l"
-#define PyLong_FromPid PyLong_FromLong
-#define PyLong_AsPid PyLong_AsLong
-#elif defined(SIZEOF_LONG_LONG) && SIZEOF_PID_T == SIZEOF_LONG_LONG
-#define PARSE_PID "L"
-#define PyLong_FromPid PyLong_FromLongLong
-#define PyLong_AsPid PyLong_AsLongLong
-#else
-#error "sizeof(pid_t) is neither sizeof(int), sizeof(long) or sizeof(long long)"
-#endif /* SIZEOF_PID_T */
-
 /* Don't use the "_r" form if we don't need it (also, won't have a
    prototype for it, at least on Solaris -- maybe others as well?). */
 #if defined(HAVE_CTERMID_R) && defined(WITH_THREAD)

Objects/abstract.c

@@ -2722,3 +2722,63 @@ PyIter_Next(PyObject *iter)
 		PyErr_Clear();
 	return result;
 }
+
+
+/*
+ * Flatten a sequence of bytes() objects into a C array of
+ * NULL terminated string pointers with a NULL char* terminating the array.
+ * (ie: an argv or env list)
+ *
+ * Memory allocated for the returned list is allocated using malloc() and MUST
+ * be freed by the caller using a free() loop or _Py_FreeCharPArray().
+ */
+char *const *
+_PySequence_BytesToCharpArray(PyObject* self)
+{
+	char **array;
+	Py_ssize_t i, argc;
+
+	argc = PySequence_Size(self);
+	if (argc == -1)
+		return NULL;
+
+	array = malloc((argc + 1) * sizeof(char *));
+	if (array == NULL) {
+		PyErr_NoMemory();
+		return NULL;
+	}
+	for (i = 0; i < argc; ++i) {
+		char *data;
+		PyObject *item = PySequence_GetItem(self, i);
+		data = PyBytes_AsString(item);
+		if (data == NULL) {
+			/* NULL terminate before freeing. */
+			array[i] = NULL;
+			goto fail;
+		}
+		array[i] = strdup(data);
+		if (!array[i]) {
+			PyErr_NoMemory();
+			goto fail;
+		}
+	}
+	array[argc] = NULL;
+
+	return array;
+
+fail:
+	_Py_FreeCharPArray(array);
+	return NULL;
+}
+
+
+/* Free's a NULL terminated char** array of C strings. */
+void
+_Py_FreeCharPArray(char *const array[])
+{
+	Py_ssize_t i;
+	for (i = 0; array[i] != NULL; ++i) {
+		free(array[i]);
+	}
+	free((void*)array);
+}

Python/pythonrun.c

@@ -2130,6 +2130,27 @@ initsigs(void)
 }
 
 
+/* Restore signals that the interpreter has called SIG_IGN on to SIG_DFL.
+ *
+ * All of the code in this function must only use async-signal-safe functions,
+ * listed at `man 7 signal` or
+ * http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html.
+ */
+void
+_Py_RestoreSignals(void)
+{
+#ifdef SIGPIPE
+	PyOS_setsig(SIGPIPE, SIG_DFL);
+#endif
+#ifdef SIGXFZ
+	PyOS_setsig(SIGXFZ, SIG_DFL);
+#endif
+#ifdef SIGXFSZ
+	PyOS_setsig(SIGXFSZ, SIG_DFL);
+#endif
+}
+
+
 /*
  * The file descriptor fd is considered ``interactive'' if either
  *   a) isatty(fd) is TRUE, or
@@ -2223,6 +2244,11 @@ PyOS_getsig(int sig)
 #endif
 }
 
+/*
+ * All of the code in this function must only use async-signal-safe functions,
+ * listed at `man 7 signal` or
+ * http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html.
+ */
 PyOS_sighandler_t
 PyOS_setsig(int sig, PyOS_sighandler_t handler)
 {

setup.py

@@ -546,6 +546,9 @@ class PyBuildExt(build_ext):
         # CSV files
         exts.append( Extension('_csv', ['_csv.c']) )
 
+        # POSIX subprocess module helper.
+        exts.append( Extension('_posixsubprocess', ['_posixsubprocess.c']) )
+
         # socket(2)
         exts.append( Extension('_socket', ['socketmodule.c'],
                                depends = ['socketmodule.h']) )