From 84de22cbb266cf3457c9bab46b052a465078bc3f Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Mon, 29 Jan 2018 05:09:39 -0500 Subject: [PATCH 1/9] Fix memory leaks and error handling in posix spawn --- Modules/posixmodule.c | 83 ++++++++++++++++++++++++++----------------- 1 file changed, 51 insertions(+), 32 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 8b11e981f11518..26f512b4dc0a76 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5139,8 +5139,10 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, /*[clinic end generated code: output=d023521f541c709c input=0ec9f1cfdc890be5]*/ { EXECV_CHAR **argvlist = NULL; - EXECV_CHAR **envlist; + EXECV_CHAR **envlist = NULL; + posix_spawn_file_actions_t *file_actionsp = NULL; Py_ssize_t argc, envc; + PyObject* result = NULL; /* posix_spawn has three arguments: (path, argv, env), where argv is a list or tuple of strings and env is a dictionary @@ -5149,7 +5151,7 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, if (!PySequence_Check(argv)){ PyErr_SetString(PyExc_TypeError, "posix_spawn: argv must be a tuple or list"); - goto fail; + goto exit; } argc = PySequence_Size(argv); if (argc < 1) { @@ -5160,31 +5162,30 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, if (!PyMapping_Check(env)) { PyErr_SetString(PyExc_TypeError, "posix_spawn: environment must be a mapping object"); - goto fail; + goto exit; } argvlist = parse_arglist(argv, &argc); if (argvlist == NULL) { - goto fail; + goto exit; } if (!argvlist[0][0]) { PyErr_SetString(PyExc_ValueError, "posix_spawn: argv first element cannot be empty"); - goto fail; + goto exit; } envlist = parse_envlist(env, &envc); if (envlist == NULL) - goto fail; + goto exit; pid_t pid; - posix_spawn_file_actions_t *file_actionsp = NULL; if (file_actions != NULL && file_actions != Py_None){ posix_spawn_file_actions_t _file_actions; if(posix_spawn_file_actions_init(&_file_actions) != 0){ PyErr_SetString(PyExc_TypeError, "Error initializing file actions"); - goto fail; + goto exit; } @@ -5193,7 +5194,7 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, PyObject* seq = PySequence_Fast(file_actions, "file_actions must be a sequence"); if(seq == NULL){ - goto fail; + goto exit; } PyObject* file_actions_obj; PyObject* mode_obj; @@ -5203,7 +5204,7 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, 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 fail; + goto exit; } @@ -5217,81 +5218,99 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, 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 fail; + goto exit; } long open_fd = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 1)); if(PyErr_Occurred()) { - goto fail; + goto exit; } const char* open_path = PyUnicode_AsUTF8(PySequence_GetItem(file_actions_obj, 2)); if(open_path == NULL){ - goto fail; + goto exit; } long open_oflag = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 3)); if(PyErr_Occurred()) { - goto fail; + goto exit; } long open_mode = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 4)); if(PyErr_Occurred()) { - goto fail; + goto exit; } - posix_spawn_file_actions_addopen(file_actionsp, open_fd, open_path, open_oflag, open_mode); + 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 fail; + goto exit; } long close_fd = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 1)); if(PyErr_Occurred()) { - goto fail; + goto exit; + } + if(posix_spawn_file_actions_addclose(file_actionsp, close_fd)) { + PyErr_SetString(PyExc_OSError,"Failed to add close file action"); + goto exit; } - posix_spawn_file_actions_addclose(file_actionsp, close_fd); 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 fail; + goto exit; } long fd1 = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 1)); if(PyErr_Occurred()) { - goto fail; + goto exit; } long fd2 = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 2)); if(PyErr_Occurred()) { - goto fail; + goto exit; } - posix_spawn_file_actions_adddup2(file_actionsp, fd1, fd2); + 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 fail; + goto exit; } } Py_DECREF(seq); -} + } _Py_BEGIN_SUPPRESS_IPH - posix_spawn(&pid, path->narrow, file_actionsp, NULL, argvlist, envlist); - return PyLong_FromPid(pid); + int 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 exited"); + } + result = PyLong_FromPid(pid); - path_error(path); - - free_string_array(envlist, envc); +exit: -fail: + if(file_actionsp) { + posix_spawn_file_actions_destroy(file_actionsp); + } + + if (envlist) { + free_string_array(envlist, envc); + } if (argvlist) { free_string_array(argvlist, argc); } - return NULL; + + return result; } From 5d8264cce281fddb9685149e122fd7b94a41a4a9 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Mon, 29 Jan 2018 05:19:20 -0500 Subject: [PATCH 2/9] Improve error handling when destroying the file_actions object --- Modules/posixmodule.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 26f512b4dc0a76..9277157b10483a 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5238,9 +5238,9 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, 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"); + PyErr_SetString(PyExc_OSError,"Failed to add open file action"); goto exit; - } + } break; @@ -5255,7 +5255,7 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, goto exit; } if(posix_spawn_file_actions_addclose(file_actionsp, close_fd)) { - PyErr_SetString(PyExc_OSError,"Failed to add close file action"); + PyErr_SetString(PyExc_OSError,"Failed to add close file action"); goto exit; } break; @@ -5275,9 +5275,9 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, goto exit; } if(posix_spawn_file_actions_adddup2(file_actionsp, fd1, fd2)) { - PyErr_SetString(PyExc_OSError,"Failed to add dup2 file action"); + PyErr_SetString(PyExc_OSError,"Failed to add dup2 file action"); goto exit; - } + } break; default: @@ -5298,8 +5298,8 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, exit: - if(file_actionsp) { - posix_spawn_file_actions_destroy(file_actionsp); + if(file_actionsp && posix_spawn_file_actions_destroy(file_actionsp)) { + PyErr_SetString(PyExc_OSError,"Error cleaning file actions object"); } if (envlist) { From 06444fd86f62403dd3ce4565f220f1af0954305d Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Mon, 29 Jan 2018 05:26:33 -0500 Subject: [PATCH 3/9] Py_DECREF the result of PySequence_Fast on error --- Modules/posixmodule.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 9277157b10483a..aa9a36f2ae4007 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5143,6 +5143,8 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, posix_spawn_file_actions_t *file_actionsp = NULL; Py_ssize_t argc, envc; PyObject* result = NULL; + PyObject* seq = NULL; + /* posix_spawn has three arguments: (path, argv, env), where argv is a list or tuple of strings and env is a dictionary @@ -5192,7 +5194,7 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, file_actionsp = &_file_actions; - PyObject* seq = PySequence_Fast(file_actions, "file_actions must be a sequence"); + seq = PySequence_Fast(file_actions, "file_actions must be a sequence"); if(seq == NULL){ goto exit; } @@ -5285,7 +5287,6 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, goto exit; } } - Py_DECREF(seq); } _Py_BEGIN_SUPPRESS_IPH @@ -5298,6 +5299,8 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, exit: + Py_XDECREF(seq); + if(file_actionsp && posix_spawn_file_actions_destroy(file_actionsp)) { PyErr_SetString(PyExc_OSError,"Error cleaning file actions object"); } From 3c2c4e8f547998f6fa38cf424f3f73fb176c1e46 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Mon, 29 Jan 2018 05:37:29 -0500 Subject: [PATCH 4/9] Handle uninitialized pid --- Modules/posixmodule.c | 1 + 1 file changed, 1 insertion(+) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index aa9a36f2ae4007..3b614fe4b3c6ee 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5294,6 +5294,7 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, _Py_END_SUPPRESS_IPH if(err_code) { PyErr_SetString(PyExc_OSError,"posix_spawn call exited"); + goto exit; } result = PyLong_FromPid(pid); From 57b630315d32c6ed815733e2aa40406ec40667dc Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Mon, 29 Jan 2018 05:46:24 -0500 Subject: [PATCH 5/9] Use OSError if file actions fails to initialize --- Modules/posixmodule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 3b614fe4b3c6ee..e56b9e9930f4bd 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5185,7 +5185,7 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, if (file_actions != NULL && file_actions != Py_None){ posix_spawn_file_actions_t _file_actions; if(posix_spawn_file_actions_init(&_file_actions) != 0){ - PyErr_SetString(PyExc_TypeError, + PyErr_SetString(PyExc_OSError, "Error initializing file actions"); goto exit; } From 78ea6d5a9adf1fcc1f952fdb14315d0f00bf8177 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Mon, 29 Jan 2018 05:48:43 -0500 Subject: [PATCH 6/9] Move _file_actions to outer scope to avoid undefined behaviour --- Modules/posixmodule.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index e56b9e9930f4bd..7f81f90a8d1371 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5140,6 +5140,7 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, { EXECV_CHAR **argvlist = NULL; EXECV_CHAR **envlist = NULL; + posix_spawn_file_actions_t _file_actions; posix_spawn_file_actions_t *file_actionsp = NULL; Py_ssize_t argc, envc; PyObject* result = NULL; @@ -5183,17 +5184,14 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, pid_t pid; if (file_actions != NULL && file_actions != Py_None){ - posix_spawn_file_actions_t _file_actions; 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){ goto exit; From a1245a22b5bc1867eeae9f02713dc09d09ce1d2d Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Mon, 29 Jan 2018 06:59:24 -0500 Subject: [PATCH 7/9] Remove HAVE_POSIX_SPAWN define in Modules/posixmodule.c --- Modules/posixmodule.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 7f81f90a8d1371..5b94a93ff166e9 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -176,14 +176,6 @@ corresponding Unix manual entries for more information on calls."); #else /* Unix functions that the configure script doesn't check for */ #define HAVE_EXECV 1 -/* bpo-32705: Current Android does not have posix_spawn - * Most likely posix_spawn will be available in next Android version (Android - * P, API 28). Need revisit then. See - * https://android-review.googlesource.com/c/platform/bionic/+/504842 - **/ -#ifndef __ANDROID__ -#define HAVE_POSIX_SPAWN 1 -#endif #define HAVE_FORK 1 #if defined(__USLC__) && defined(__SCO_VERSION__) /* SCO UDK Compiler */ #define HAVE_FORK1 1 From 1b75f8979e67425e358b43c83d2eae74fb40748d Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Mon, 29 Jan 2018 12:13:54 -0500 Subject: [PATCH 8/9] Unshadow exception and clean error message --- Modules/posixmodule.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 5b94a93ff166e9..ea6d499ce5ee0c 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5143,7 +5143,7 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, argv is a list or tuple of strings and env is a dictionary like posix.environ. */ - if (!PySequence_Check(argv)){ + if (!PySequence_Check(argv)) { PyErr_SetString(PyExc_TypeError, "posix_spawn: argv must be a tuple or list"); goto exit; @@ -5171,11 +5171,12 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, } envlist = parse_envlist(env, &envc); - if (envlist == NULL) + if (envlist == NULL) { goto exit; + } pid_t pid; - if (file_actions != NULL && file_actions != Py_None){ + 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"); @@ -5185,7 +5186,7 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, file_actionsp = &_file_actions; seq = PySequence_Fast(file_actions, "file_actions must be a sequence"); - if(seq == NULL){ + if(seq == NULL) { goto exit; } PyObject* file_actions_obj; @@ -5194,7 +5195,7 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, 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)){ + 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; } @@ -5208,7 +5209,7 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, switch(mode) { case POSIX_SPAWN_OPEN: - if(PySequence_Size(file_actions_obj) != 5){ + if(PySequence_Size(file_actions_obj) != 5) { PyErr_SetString(PyExc_TypeError,"A open file_action object must have 5 elements"); goto exit; } @@ -5218,7 +5219,7 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, goto exit; } const char* open_path = PyUnicode_AsUTF8(PySequence_GetItem(file_actions_obj, 2)); - if(open_path == NULL){ + if(open_path == NULL) { goto exit; } long open_oflag = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 3)); @@ -5283,7 +5284,7 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, int 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 exited"); + PyErr_SetString(PyExc_OSError,"posix_spawn call failed"); goto exit; } result = PyLong_FromPid(pid); @@ -5292,8 +5293,8 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, Py_XDECREF(seq); - if(file_actionsp && posix_spawn_file_actions_destroy(file_actionsp)) { - PyErr_SetString(PyExc_OSError,"Error cleaning file actions object"); + if(file_actionsp) { + posix_spawn_file_actions_destroy(file_actionsp); } if (envlist) { From 4e9e54e0d7043fa53b041e398f7d574ab611638d Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Mon, 29 Jan 2018 12:44:56 -0500 Subject: [PATCH 9/9] Fix whitespace issues --- Modules/posixmodule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index ea6d499ce5ee0c..46f3adaf4dc320 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5294,7 +5294,7 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, Py_XDECREF(seq); if(file_actionsp) { - posix_spawn_file_actions_destroy(file_actionsp); + posix_spawn_file_actions_destroy(file_actionsp); } if (envlist) {