bpo-20104: Improve error handling and fix a reference leak in os.posix_spawn(). (#6332)

5 files changed, 313 insertions, 152 deletions

diff --git a/Doc/library/os.rst b/Doc/library/os.rst
index 2e81459..4b4e931 100644
--- a/Doc/library/os.rst
+++ b/Doc/library/os.rst
@@ -3365,25 +3365,41 @@ written in Python, such as a mail server's external command delivery program.
 
 .. function:: posix_spawn(path, argv, env, file_actions=None)
 
-   Wraps the posix_spawn() C library API for use from Python.
+   Wraps the :c:func:`posix_spawn` C library API for use from Python.
 
-   Most users should use :class:`subprocess.run` instead of posix_spawn.
+   Most users should use :func:`subprocess.run` instead of :func:`posix_spawn`.
 
    The *path*, *args*, and *env* arguments are similar to :func:`execve`.
 
    The *file_actions* argument may be a sequence of tuples describing actions
    to take on specific file descriptors in the child process between the C
-   library implementation's fork and exec steps.  The first item in each tuple
-   must be one of the three type indicator listed below describing the
-   remaining tuple elements:
+   library implementation's :c:func:`fork` and :c:func:`exec` steps.
+   The first item in each tuple must be one of the three type indicator
+   listed below describing the remaining tuple elements:
 
-   (os.POSIX_SPAWN_OPEN, fd, path, open flags, mode)
-   (os.POSIX_SPAWN_CLOSE, fd)
-   (os.POSIX_SPAWN_DUP2, fd, new_fd)
+   .. data:: POSIX_SPAWN_OPEN
 
-   These tuples correspond to the C library posix_spawn_file_actions_addopen,
-   posix_spawn_file_actions_addclose, and posix_spawn_file_actions_adddup2 API
-   calls used to prepare for the posix_spawn call itself.
+      (``os.POSIX_SPAWN_OPEN``, *fd*, *path*, *flags*, *mode*)
+
+      Performs ``os.dup2(os.open(path, flags, mode), fd)``.
+
+   .. data:: POSIX_SPAWN_CLOSE
+
+      (``os.POSIX_SPAWN_CLOSE``, *fd*)
+
+      Performs ``os.close(fd)``.
+
+   .. data:: POSIX_SPAWN_DUP2
+
+      (``os.POSIX_SPAWN_DUP2``, *fd*, *new_fd*)
+
+      Performs ``os.dup2(fd, new_fd)``.
+
+   These tuples correspond to the C library
+   :c:func:`posix_spawn_file_actions_addopen`,
+   :c:func:`posix_spawn_file_actions_addclose`, and
+   :c:func:`posix_spawn_file_actions_adddup2` API calls used to prepare
+   for the :c:func:`posix_spawn` call itself.
 
    .. versionadded:: 3.7
 
diff --git a/Lib/test/test_posix.py b/Lib/test/test_posix.py
index 8ada0e3..b94da3f 100644
--- a/Lib/test/test_posix.py
+++ b/Lib/test/test_posix.py
@@ -177,22 +177,6 @@ class PosixTester(unittest.TestCase):
             os.close(fp)
 
 
-    @unittest.skipUnless(hasattr(os, 'posix_spawn'), "test needs os.posix_spawn")
-    def test_posix_spawn(self):
-        pid = posix.posix_spawn(sys.executable, [sys.executable, "-c", "pass"], os.environ,[])
-        self.assertEqual(os.waitpid(pid,0),(pid,0))
-
-
-    @unittest.skipUnless(hasattr(os, 'posix_spawn'), "test needs os.posix_spawn")
-    def test_posix_spawn_file_actions(self):
-        file_actions = []
-        file_actions.append((0,3,os.path.realpath(__file__),0,0))
-        file_actions.append((os.POSIX_SPAWN_CLOSE,2))
-        file_actions.append((os.POSIX_SPAWN_DUP2,1,4))
-        pid = posix.posix_spawn(sys.executable, [sys.executable, "-c", "pass"], os.environ, file_actions)
-        self.assertEqual(os.waitpid(pid,0),(pid,0))
-
-
     @unittest.skipUnless(hasattr(posix, 'waitid'), "test needs posix.waitid()")
     @unittest.skipUnless(hasattr(os, 'fork'), "test needs os.fork()")
     def test_waitid(self):
@@ -1437,9 +1421,168 @@ class PosixGroupsTester(unittest.TestCase):
             posix.setgroups(groups)
             self.assertListEqual(groups, posix.getgroups())
 
+
+@unittest.skipUnless(hasattr(os, 'posix_spawn'), "test needs os.posix_spawn")
+class TestPosixSpawn(unittest.TestCase):
+    def test_returns_pid(self):
+        pidfile = support.TESTFN
+        self.addCleanup(support.unlink, pidfile)
+        script = f"""if 1:
+            import os
+            with open({pidfile!r}, "w") as pidfile:
+                pidfile.write(str(os.getpid()))
+            """
+        pid = posix.posix_spawn(sys.executable,
+                                [sys.executable, '-c', script],
+                                os.environ)
+        self.assertEqual(os.waitpid(pid, 0), (pid, 0))
+        with open(pidfile) as f:
+            self.assertEqual(f.read(), str(pid))
+
+    def test_no_such_executable(self):
+        no_such_executable = 'no_such_executable'
+        try:
+            pid = posix.posix_spawn(no_such_executable,
+                                    [no_such_executable],
+                                    os.environ)
+        except FileNotFoundError as exc:
+            self.assertEqual(exc.filename, no_such_executable)
+        else:
+            pid2, status = os.waitpid(pid, 0)
+            self.assertEqual(pid2, pid)
+            self.assertNotEqual(status, 0)
+
+    def test_specify_environment(self):
+        envfile = support.TESTFN
+        self.addCleanup(support.unlink, envfile)
+        script = f"""if 1:
+            import os
+            with open({envfile!r}, "w") as envfile:
+                envfile.write(os.environ['foo'])
+        """
+        pid = posix.posix_spawn(sys.executable,
+                                [sys.executable, '-c', script],
+                                {'foo': 'bar'})
+        self.assertEqual(os.waitpid(pid, 0), (pid, 0))
+        with open(envfile) as f:
+            self.assertEqual(f.read(), 'bar')
+
+    def test_empty_file_actions(self):
+        pid = posix.posix_spawn(
+            sys.executable,
+            [sys.executable, '-c', 'pass'],
+            os.environ,
+            []
+        )
+        self.assertEqual(os.waitpid(pid, 0), (pid, 0))
+
+    def test_multiple_file_actions(self):
+        file_actions = [
+            (os.POSIX_SPAWN_OPEN, 3, os.path.realpath(__file__), os.O_RDONLY, 0),
+            (os.POSIX_SPAWN_CLOSE, 0),
+            (os.POSIX_SPAWN_DUP2, 1, 4),
+        ]
+        pid = posix.posix_spawn(sys.executable,
+                                [sys.executable, "-c", "pass"],
+                                os.environ, file_actions)
+        self.assertEqual(os.waitpid(pid, 0), (pid, 0))
+
+    def test_bad_file_actions(self):
+        with self.assertRaises(TypeError):
+            posix.posix_spawn(sys.executable,
+                              [sys.executable, "-c", "pass"],
+                              os.environ, [None])
+        with self.assertRaises(TypeError):
+            posix.posix_spawn(sys.executable,
+                              [sys.executable, "-c", "pass"],
+                              os.environ, [()])
+        with self.assertRaises(TypeError):
+            posix.posix_spawn(sys.executable,
+                              [sys.executable, "-c", "pass"],
+                              os.environ, [(None,)])
+        with self.assertRaises(TypeError):
+            posix.posix_spawn(sys.executable,
+                              [sys.executable, "-c", "pass"],
+                              os.environ, [(12345,)])
+        with self.assertRaises(TypeError):
+            posix.posix_spawn(sys.executable,
+                              [sys.executable, "-c", "pass"],
+                              os.environ, [(os.POSIX_SPAWN_CLOSE,)])
+        with self.assertRaises(TypeError):
+            posix.posix_spawn(sys.executable,
+                              [sys.executable, "-c", "pass"],
+                              os.environ, [(os.POSIX_SPAWN_CLOSE, 1, 2)])
+        with self.assertRaises(TypeError):
+            posix.posix_spawn(sys.executable,
+                              [sys.executable, "-c", "pass"],
+                              os.environ, [(os.POSIX_SPAWN_CLOSE, None)])
+        with self.assertRaises(ValueError):
+            posix.posix_spawn(sys.executable,
+                              [sys.executable, "-c", "pass"],
+                              os.environ,
+                              [(os.POSIX_SPAWN_OPEN, 3, __file__ + '\0',
+                                os.O_RDONLY, 0)])
+
+    def test_open_file(self):
+        outfile = support.TESTFN
+        self.addCleanup(support.unlink, outfile)
+        script = """if 1:
+            import sys
+            sys.stdout.write("hello")
+            """
+        file_actions = [
+            (os.POSIX_SPAWN_OPEN, 1, outfile,
+                os.O_WRONLY | os.O_CREAT | os.O_TRUNC,
+                stat.S_IRUSR | stat.S_IWUSR),
+        ]
+        pid = posix.posix_spawn(sys.executable,
+                                [sys.executable, '-c', script],
+                                os.environ, file_actions)
+        self.assertEqual(os.waitpid(pid, 0), (pid, 0))
+        with open(outfile) as f:
+            self.assertEqual(f.read(), 'hello')
+
+    def test_close_file(self):
+        closefile = support.TESTFN
+        self.addCleanup(support.unlink, closefile)
+        script = f"""if 1:
+            import os
+            try:
+                os.fstat(0)
+            except OSError as e:
+                with open({closefile!r}, 'w') as closefile:
+                    closefile.write('is closed %d' % e.errno)
+            """
+        pid = posix.posix_spawn(sys.executable,
+                                [sys.executable, '-c', script],
+                                os.environ,
+                                [(os.POSIX_SPAWN_CLOSE, 0),])
+        self.assertEqual(os.waitpid(pid, 0), (pid, 0))
+        with open(closefile) as f:
+            self.assertEqual(f.read(), 'is closed %d' % errno.EBADF)
+
+    def test_dup2(self):
+        dupfile = support.TESTFN
+        self.addCleanup(support.unlink, dupfile)
+        script = """if 1:
+            import sys
+            sys.stdout.write("hello")
+            """
+        with open(dupfile, "wb") as childfile:
+            file_actions = [
+                (os.POSIX_SPAWN_DUP2, childfile.fileno(), 1),
+            ]
+            pid = posix.posix_spawn(sys.executable,
+                                    [sys.executable, '-c', script],
+                                    os.environ, file_actions)
+            self.assertEqual(os.waitpid(pid, 0), (pid, 0))
+        with open(dupfile) as f:
+            self.assertEqual(f.read(), 'hello')
+
+
 def test_main():
     try:
-        support.run_unittest(PosixTester, PosixGroupsTester)
+        support.run_unittest(PosixTester, PosixGroupsTester, TestPosixSpawn)
     finally:
         support.reap_children()
 
diff --git a/Misc/NEWS.d/next/Library/2018-04-01-19-21-04.bpo-20104.-AKcGa.rst b/Misc/NEWS.d/next/Library/2018-04-01-19-21-04.bpo-20104.-AKcGa.rst
new file mode 100644
index 0000000..150401d
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2018-04-01-19-21-04.bpo-20104.-AKcGa.rst
@@ -0,0 +1 @@
+Improved error handling and fixed a reference leak in :func:`os.posix_spawn()`.
diff --git a/Modules/clinic/posixmodule.c.h b/Modules/clinic/posixmodule.c.h
index 4054389..e4bbd08 100644
--- a/Modules/clinic/posixmodule.c.h
+++ b/Modules/clinic/posixmodule.c.h
@@ -1742,7 +1742,7 @@ PyDoc_STRVAR(os_posix_spawn__doc__,
 "  env\n"
 "    Dictionary of strings mapping to strings.\n"
 "  file_actions\n"
-"    FileActions object.");
+"    A sequence of file action tuples.");
 
 #define OS_POSIX_SPAWN_METHODDEF    \
     {"posix_spawn", (PyCFunction)os_posix_spawn, METH_FASTCALL, os_posix_spawn__doc__},
@@ -6589,4 +6589,4 @@ exit:
 #ifndef OS_GETRANDOM_METHODDEF
     #define OS_GETRANDOM_METHODDEF
 #endif /* !defined(OS_GETRANDOM_METHODDEF) */
-/*[clinic end generated code: output=fc603214822bdda6 input=a9049054013a1b77]*/
+/*[clinic end generated code: output=8d3d9dddf254c3c2 input=a9049054013a1b77]*/
diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c
index e672103..4ac6e76 100644
--- a/Modules/posixmodule.c
+++ b/Modules/posixmodule.c
@@ -5114,6 +5114,111 @@ enum posix_spawn_file_actions_identifier {
     POSIX_SPAWN_DUP2
 };
 
+static int
+parse_file_actions(PyObject *file_actions,
+                   posix_spawn_file_actions_t *file_actionsp)
+{
+    PyObject *seq;
+    PyObject *file_action = NULL;
+    PyObject *tag_obj;
+
+    seq = PySequence_Fast(file_actions,
+                          "file_actions must be a sequence or None");
+    if (seq == NULL) {
+        return -1;
+    }
+
+    errno = posix_spawn_file_actions_init(file_actionsp);
+    if (errno) {
+        posix_error();
+        Py_DECREF(seq);
+        return -1;
+    }
+
+    for (int i = 0; i < PySequence_Fast_GET_SIZE(seq); ++i) {
+        file_action = PySequence_Fast_GET_ITEM(seq, i);
+        Py_INCREF(file_action);
+        if (!PyTuple_Check(file_action) || !PyTuple_GET_SIZE(file_action)) {
+            PyErr_SetString(PyExc_TypeError,
+                "Each file_actions element must be a non-empty tuple");
+            goto fail;
+        }
+        long tag = PyLong_AsLong(PyTuple_GET_ITEM(file_action, 0));
+        if (tag == -1 && PyErr_Occurred()) {
+            goto fail;
+        }
+
+        /* Populate the file_actions object */
+        switch (tag) {
+            case POSIX_SPAWN_OPEN: {
+                int fd, oflag;
+                PyObject *path;
+                unsigned long mode;
+                if (!PyArg_ParseTuple(file_action, "OiO&ik"
+                        ";A open file_action tuple must have 5 elements",
+                        &tag_obj, &fd, PyUnicode_FSConverter, &path,
+                        &oflag, &mode))
+                {
+                    goto fail;
+                }
+                errno = posix_spawn_file_actions_addopen(file_actionsp,
+                        fd, PyBytes_AS_STRING(path), oflag, (mode_t)mode);
+                Py_DECREF(path);  /* addopen copied it. */
+                if (errno) {
+                    posix_error();
+                    goto fail;
+                }
+                break;
+            }
+            case POSIX_SPAWN_CLOSE: {
+                int fd;
+                if (!PyArg_ParseTuple(file_action, "Oi"
+                        ";A close file_action tuple must have 2 elements",
+                        &tag_obj, &fd))
+                {
+                    goto fail;
+                }
+                errno = posix_spawn_file_actions_addclose(file_actionsp, fd);
+                if (errno) {
+                    posix_error();
+                    goto fail;
+                }
+                break;
+            }
+            case POSIX_SPAWN_DUP2: {
+                int fd1, fd2;
+                if (!PyArg_ParseTuple(file_action, "Oii"
+                        ";A dup2 file_action tuple must have 3 elements",
+                        &tag_obj, &fd1, &fd2))
+                {
+                    goto fail;
+                }
+                errno = posix_spawn_file_actions_adddup2(file_actionsp,
+                                                         fd1, fd2);
+                if (errno) {
+                    posix_error();
+                    goto fail;
+                }
+                break;
+            }
+            default: {
+                PyErr_SetString(PyExc_TypeError,
+                                "Unknown file_actions identifier");
+                goto fail;
+            }
+        }
+        Py_DECREF(file_action);
+    }
+    Py_DECREF(seq);
+    return 0;
+
+fail:
+    Py_DECREF(seq);
+    Py_DECREF(file_action);
+    (void)posix_spawn_file_actions_destroy(file_actionsp);
+    return -1;
+}
+
 /*[clinic input]
 
 os.posix_spawn
@@ -5124,7 +5229,7 @@ os.posix_spawn
     env: object
         Dictionary of strings mapping to strings.
     file_actions: object = None
-        FileActions object.
+        A sequence of file action tuples.
     /
 
 Execute the program specified by path in a new process.
@@ -5133,22 +5238,22 @@ Execute the program specified by path in a new process.
 static PyObject *
 os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv,
                     PyObject *env, PyObject *file_actions)
-/*[clinic end generated code: output=d023521f541c709c input=0ec9f1cfdc890be5]*/
+/*[clinic end generated code: output=d023521f541c709c input=a3db1021d33230dc]*/
 {
     EXECV_CHAR **argvlist = NULL;
     EXECV_CHAR **envlist = NULL;
-    posix_spawn_file_actions_t _file_actions;
+    posix_spawn_file_actions_t file_actions_buf;
     posix_spawn_file_actions_t *file_actionsp = NULL;
     Py_ssize_t argc, envc;
-    PyObject* result = NULL;
-    PyObject* seq = NULL;
- 
+    PyObject *result = NULL;
+    pid_t pid;
+    int err_code;
 
     /* posix_spawn has three arguments: (path, argv, env), where
-   argv is a list or tuple of strings and env is a dictionary
+       argv is a list or tuple of strings and env is a dictionary
        like posix.environ. */
 
-    if (!PySequence_Check(argv)) {
+    if (!PyList_Check(argv) && !PyTuple_Check(argv)) {
         PyErr_SetString(PyExc_TypeError,
                         "posix_spawn: argv must be a tuple or list");
         goto exit;
@@ -5180,139 +5285,35 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv,
         goto exit;
     }
 
-    pid_t pid;
-    if (file_actions != NULL && file_actions != Py_None) {
-        if(posix_spawn_file_actions_init(&_file_actions) != 0){
-            PyErr_SetString(PyExc_OSError,
-                            "Error initializing file actions");
-            goto exit;
-        }
-
-        file_actionsp = &_file_actions;
-
-        seq = PySequence_Fast(file_actions, "file_actions must be a sequence");
-        if(seq == NULL) {
+    if (file_actions != Py_None) {
+        if (parse_file_actions(file_actions, &file_actions_buf)) {
             goto exit;
         }
-        PyObject* file_actions_obj;
-        PyObject* mode_obj;
-
-        for (int i = 0; i < PySequence_Fast_GET_SIZE(seq); ++i) {
-            file_actions_obj = PySequence_Fast_GET_ITEM(seq, i);
-
-            if(!PySequence_Check(file_actions_obj) | !PySequence_Size(file_actions_obj)) {
-                PyErr_SetString(PyExc_TypeError,"Each file_action element must be a non empty sequence");
-                goto exit;
-            }
-
-
-            mode_obj = PySequence_Fast_GET_ITEM(file_actions_obj, 0);
-            int mode = PyLong_AsLong(mode_obj);
-
-            /* Populate the file_actions object */
-
-            switch(mode) {
-
-                case POSIX_SPAWN_OPEN:
-                    if(PySequence_Size(file_actions_obj) != 5) {
-                        PyErr_SetString(PyExc_TypeError,"A open file_action object must have 5 elements");
-                        goto exit;
-                    }
-
-                    long open_fd = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 1));
-                    if(PyErr_Occurred()) {
-                        goto exit;
-                    }
-                    const char* open_path = PyUnicode_AsUTF8(PySequence_GetItem(file_actions_obj, 2));
-                    if(open_path == NULL) {
-                        goto exit;
-                    }
-                    long open_oflag = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 3));
-                    if(PyErr_Occurred()) {
-                        goto exit;
-                    }
-                    long open_mode = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 4));
-                    if(PyErr_Occurred()) {
-                        goto exit;
-                    }
-                    if(posix_spawn_file_actions_addopen(file_actionsp, open_fd, open_path, open_oflag, open_mode)) {
-                        PyErr_SetString(PyExc_OSError,"Failed to add open file action");
-                        goto exit;
-                    }
-   
-                    break;
-
-                case POSIX_SPAWN_CLOSE:
-                    if(PySequence_Size(file_actions_obj) != 2){
-                        PyErr_SetString(PyExc_TypeError,"A close file_action object must have 2 elements");
-                        goto exit;
-                    }
-
-                    long close_fd = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 1));
-                    if(PyErr_Occurred()) {
-                        goto exit;
-                    }
-                    if(posix_spawn_file_actions_addclose(file_actionsp, close_fd)) {
-                        PyErr_SetString(PyExc_OSError,"Failed to add close file action");
-                        goto exit;
-                    }
-                    break;
-
-                case POSIX_SPAWN_DUP2:
-                    if(PySequence_Size(file_actions_obj) != 3){
-                        PyErr_SetString(PyExc_TypeError,"A dup2 file_action object must have 3 elements");
-                        goto exit;
-                    }
-
-                    long fd1 = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 1));
-                    if(PyErr_Occurred()) {
-                        goto exit;
-                    }
-                    long fd2 = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 2));
-                    if(PyErr_Occurred()) {
-                        goto exit;
-                    }
-                    if(posix_spawn_file_actions_adddup2(file_actionsp, fd1, fd2)) {
-                        PyErr_SetString(PyExc_OSError,"Failed to add dup2 file action");
-                        goto exit;
-                    }
-                    break;
-
-                default:
-                    PyErr_SetString(PyExc_TypeError,"Unknown file_actions identifier");
-                    goto exit;
-            }
-        }
+        file_actionsp = &file_actions_buf;
     }
 
     _Py_BEGIN_SUPPRESS_IPH
-    int err_code = posix_spawn(&pid, path->narrow, file_actionsp, NULL, argvlist, envlist);
+    err_code = posix_spawn(&pid, path->narrow,
+                           file_actionsp, NULL, argvlist, envlist);
     _Py_END_SUPPRESS_IPH
-    if(err_code) {
-        PyErr_SetString(PyExc_OSError,"posix_spawn call failed");
+    if (err_code) {
+        errno = err_code;
+        PyErr_SetFromErrnoWithFilenameObject(PyExc_OSError, path->object);
         goto exit;
     }
     result = PyLong_FromPid(pid);
 
 exit:
-
-    Py_XDECREF(seq);
-
-    if(file_actionsp) {
-        posix_spawn_file_actions_destroy(file_actionsp);
+    if (file_actionsp) {
+        (void)posix_spawn_file_actions_destroy(file_actionsp);
     }
-    
     if (envlist) {
         free_string_array(envlist, envc);
     }
-
     if (argvlist) {
         free_string_array(argvlist, argc);
     }
-
     return result;
-
-
 }
 #endif /* HAVE_POSIX_SPAWN */