Repository navigation
bpo-20104: Fix memory leaks and error handling in posix spawn #5418
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Jump to
Filter by extension
Filter by extension
.c
(1)
All 1 file type selected
Viewed files
Viewed files
There are no files selected for viewing
This file contains hidden or 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 |
|---|---|---|
| Expand Up | @@ -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 | ||
| Expand Down Expand Up | @@ -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) { | ||
| Expand All | @@ -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; | ||
| } | ||
|
|
||
|
|
||
| Expand All | @@ -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; | ||
|
Comment thread
Outdated
Copy link
Copy Markdown
Member
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?
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
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.
Sorry, something went wrong. |
||
|
|
||
|
|
||
| } | ||
| Expand Down | ||
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.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Qualityjust for my own info, why do you remove this condition? you won't support all the versions of Android... is it right?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThe check is done now in the configuration phase and it will be automatically detected if the system has this function.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.