bpo-20104: Fix memory leaks and error handling in posix spawn #5418

pablogsal · 2018-01-29T08:36:45Z

https://bugs.python.org/issue20104

pablogsal · 2018-01-29T08:37:09Z

vadmium · 2018-01-29T09:22:04Z

Modules/posixmodule.c

+                    }
+                    if(!posix_spawn_file_actions_addclose(file_actionsp, close_fd)){
+                        goto exit;
+                        PyErr_SetString(PyExc_OSError,"An error ocurred when initializing a close fileaction");


Put goto last?

vadmium · 2018-01-29T09:22:06Z

Modules/posixmodule.c

+                        goto exit;
+                    }
+                    if(!posix_spawn_file_actions_addopen(file_actionsp, open_fd, open_path, open_oflag, open_mode)){
+                        PyErr_SetString(PyExc_OSError,"An error ocurred when initializing an open fileaction");


Spelling: occurred

vadmium · 2018-01-29T09:41:07Z

Modules/posixmodule.c

+
+    if(!posix_spawn_file_actions_destroy(file_actionsp)){
+        PyErr_SetString(PyExc_OSError,"An error ocurred when cleaning the file actions object");
+        goto exit;


If this ever fails, you have an infinite loop.

file_actionsp is only initialized halfway down. In C99 I understand the goto statements above it are allowed to bypass the initialization, but they leave the pointer uninitialized.

Also, I haven’t seen any explicit mention of destroy accepting a null pointer. Did you test this? Even if it works on one platform, it seems risky to rely on it on all Posix platforms.

vadmium · 2018-01-29T09:42:26Z

Modules/posixmodule.c


    if (argvlist) {
        free_string_array(argvlist, argc);
    }
-    return NULL;
+
+    return result;


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?

@vadmium Sorry, there was a commit missing here for some reason, I will amend with the latest changes.

vadmium · 2018-01-29T09:43:18Z

Modules/posixmodule.c

+        PyErr_SetString(PyExc_OSError,"An error ocurred when cleaning the file actions object");
+        goto exit;
+    }
+

    free_string_array(envlist, envc);


envlist could be uninitialized if there is an early failure

pablogsal · 2018-01-29T10:16:44Z

@vadmium There was a missing commit due to a failed push. I have amended the last commit to including all the fixes. In cb74d14 you will find the correct initial implementation. Sorry for the confusion.

vadmium · 2018-01-29T10:34:14Z

Modules/posixmodule.c

+    if(err_code) {
+        PyErr_SetString(PyExc_OSError,"posix_spawn call exited");
+    }
+    result = PyLong_FromPid(pid);


Pid uninitialized again (in error case). Maybe add another goto exit.

Done in 47cec89

Also, now there is always cleanup for the result of PySequence_Fast.

pablogsal · 2018-01-29T12:04:09Z

@vstinner Can you take a general look at this PR (if you have the time) to check that we are not missing something else? (The Docs are not included in this PR)

gpshead

thanks for the cleanup!

gpshead · 2018-01-29T16:58:40Z

Modules/posixmodule.c

    _Py_END_SUPPRESS_IPH
+    if(err_code) {
+        PyErr_SetString(PyExc_OSError,"posix_spawn call exited");


"posix_spawn call failed" (it always exits)

Done in 1b75f89

gpshead · 2018-01-29T17:06:14Z

Modules/posixmodule.c


-fail:
+    if(file_actionsp && posix_spawn_file_actions_destroy(file_actionsp)) {
+        PyErr_SetString(PyExc_OSError,"Error cleaning file actions object");


The only possible error from posix_spawn_file_actions_destroy() is EINVAL if the supplied file_actionsp was invalid. That will only happen here during the "goto exit" path after a failure from posix_spawn_file_actions_init(), meaning this exception could clobber that one. I'd just leave the_destroy() call unchecked, there isn't anything we can do if it fails anyways.

Done in 1b75f89

bedevere-bot · 2018-01-29T17:06:38Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

pablogsal · 2018-01-29T17:15:49Z

I have made the requested changes; please review again

bedevere-bot · 2018-01-29T17:15:50Z

Thanks for making the requested changes!

@gpshead: please review the changes made to this pull request.

matrixise · 2018-01-29T18:16:06Z

Modules/posixmodule.c

- * https://android-review.googlesource.com/c/platform/bionic/+/504842
- **/
-#ifndef __ANDROID__
-#define HAVE_POSIX_SPAWN 1


just for my own info, why do you remove this condition? you won't support all the versions of Android... is it right?

The check is done now in the configuration phase and it will be automatically detected if the system has this function.

the-knights-who-say-ni added the CLA signed label Jan 29, 2018

bedevere-bot added the awaiting review label Jan 29, 2018

vadmium reviewed Jan 29, 2018

View reviewed changes

pablogsal force-pushed the bpo20104 branch 2 times, most recently from 2512695 to cb74d14 Compare January 29, 2018 10:14

pablogsal force-pushed the bpo20104 branch from 9700dd4 to e14d562 Compare January 29, 2018 10:24

vadmium reviewed Jan 29, 2018

View reviewed changes

pablogsal force-pushed the bpo20104 branch from a68ae31 to be104cb Compare January 29, 2018 10:36

pablogsal added 7 commits January 29, 2018 06:58

Fix memory leaks and error handling in posix spawn

84de22c

Improve error handling when destroying the file_actions object

5d8264c

Py_DECREF the result of PySequence_Fast on error

06444fd

Handle uninitialized pid

3c2c4e8

Use OSError if file actions fails to initialize

57b6303

Move _file_actions to outer scope to avoid undefined behaviour

78ea6d5

Remove HAVE_POSIX_SPAWN define in Modules/posixmodule.c

a1245a2

pablogsal force-pushed the bpo20104 branch from 7db7a9e to a1245a2 Compare January 29, 2018 12:01

pablogsal mentioned this pull request Jan 29, 2018

bpo-32705: Current Android does not have posix_spawn #5413

Merged

gpshead requested changes Jan 29, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Jan 29, 2018

gpshead added skip news type-bug An unexpected behavior, bug, or error labels Jan 29, 2018

Unshadow exception and clean error message

1b75f89

bedevere-bot removed the awaiting changes label Jan 29, 2018

bedevere-bot added the awaiting change review label Jan 29, 2018

gpshead approved these changes Jan 29, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jan 29, 2018

Fix whitespace issues

4e9e54e

matrixise reviewed Jan 29, 2018

View reviewed changes

gpshead merged commit 0cd6bca into python:master Jan 29, 2018

bedevere-bot removed the awaiting merge label Jan 29, 2018

pablogsal deleted the bpo20104 branch January 29, 2018 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-20104: Fix memory leaks and error handling in posix spawn #5418

bpo-20104: Fix memory leaks and error handling in posix spawn #5418

pablogsal commented Jan 29, 2018 •

edited by bedevere-bot

Loading

pablogsal commented Jan 29, 2018

vadmium Jan 29, 2018

vadmium Jan 29, 2018

vadmium Jan 29, 2018

vadmium Jan 29, 2018

pablogsal Jan 29, 2018

vadmium Jan 29, 2018

pablogsal commented Jan 29, 2018 •

edited

Loading

vadmium Jan 29, 2018

pablogsal Jan 29, 2018

pablogsal Jan 29, 2018

pablogsal commented Jan 29, 2018 •

edited

Loading

gpshead left a comment

gpshead Jan 29, 2018

pablogsal Jan 29, 2018

gpshead Jan 29, 2018

pablogsal Jan 29, 2018

bedevere-bot commented Jan 29, 2018

pablogsal commented Jan 29, 2018

bedevere-bot commented Jan 29, 2018

matrixise Jan 29, 2018

pablogsal Jan 29, 2018

bpo-20104: Fix memory leaks and error handling in posix spawn #5418

bpo-20104: Fix memory leaks and error handling in posix spawn #5418

Conversation

pablogsal commented Jan 29, 2018 • edited by bedevere-bot Loading

pablogsal commented Jan 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablogsal commented Jan 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablogsal commented Jan 29, 2018 • edited Loading

gpshead left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Jan 29, 2018

pablogsal commented Jan 29, 2018

bedevere-bot commented Jan 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablogsal commented Jan 29, 2018 •

edited by bedevere-bot

Loading

pablogsal commented Jan 29, 2018 •

edited

Loading

pablogsal commented Jan 29, 2018 •

edited

Loading