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

2 files changed, 125 insertions, 124 deletions

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 */