bpo-20104: Fix memory leaks and error handling in posix spawn #5418
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
| #define HAVE_FORK 1 | ||
| #if defined(__USLC__) && defined(__SCO_VERSION__) /* SCO UDK Compiler */ | ||
| #define HAVE_FORK1 1 | ||
|
|
@@ -5139,17 +5131,22 @@ 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 = NULL; | ||
| posix_spawn_file_actions_t _file_actions; | ||
| 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 | ||
| like posix.environ. */ | ||
|
|
||
| if (!PySequence_Check(argv)) { | ||
| PyErr_SetString(PyExc_TypeError, | ||
| "posix_spawn: argv must be a tuple or list"); | ||
| goto exit; | ||
| } | ||
| argc = PySequence_Size(argv); | ||
| if (argc < 1) { | ||
|
|
@@ -5160,50 +5157,47 @@ 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 exit; | ||
| } | ||
|
|
||
| argvlist = parse_arglist(argv, &argc); | ||
| if (argvlist == NULL) { | ||
| goto exit; | ||
| } | ||
| if (!argvlist[0][0]) { | ||
| PyErr_SetString(PyExc_ValueError, | ||
| "posix_spawn: argv first element cannot be empty"); | ||
| goto exit; | ||
| } | ||
|
|
||
| envlist = parse_envlist(env, &envc); | ||
| if (envlist == NULL) { | ||
| 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) { | ||
| 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; | ||
| } | ||
|
|
||
|
|
||
|
|
@@ -5215,83 +5209,103 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, | |
| 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; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| _Py_BEGIN_SUPPRESS_IPH | ||
| 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 failed"); | ||
| goto exit; | ||
| } | ||
| result = PyLong_FromPid(pid); | ||
|
|
||
| exit: | ||
|
|
||
| Py_XDECREF(seq); | ||
|
|
||
| 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 result; | ||
|
There was a problem hiding this comment. It looks to me like result will be uninitialized in the failure cases. But surely this would be obvious by testing. Am I missing something? There was a problem hiding this comment. @vadmium Sorry, there was a commit missing here for some reason, I will amend with the latest changes. |
||
|
|
||
|
|
||
| } | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just for my own info, why do you remove this condition? you won't support all the versions of Android... is it right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check is done now in the configuration phase and it will be automatically detected if the system has this function.