bpo-20104: Fix memory leaks and error handling in posix spawn by pablogsal · Pull Request #5418 · python/cpython · GitHub

https://bugs.python.org/issue20104

Put goto last?

Spelling: occurred

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.

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.

envlist could be uninitialized if there is an early failure

@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.

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.

@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)

thanks for the cleanup!

"posix_spawn call failed" (it always exits)

Done in 1b75f89

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

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!

I have made the requested changes; please review again

Thanks for making the requested changes!

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

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

+                  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");

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

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

bpo-20104: Fix memory leaks and error handling in posix spawn by pablogsal · Pull Request #5418 · python/cpython · GitHub

Repository navigation

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

bpo-20104: Fix memory leaks and error handling in posix spawn#5418gpshead merged 9 commits intopython:masterpython/cpython:masterfrom pablogsal:bpo20104pablogsal/cpython:bpo20104Copy head branch name to clipboard

Conversation

pablogsal commented Jan 29, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablogsal commented Jan 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gpshead left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jan 29, 2018

Uh oh!

pablogsal commented Jan 29, 2018

Uh oh!

bedevere-bot commented Jan 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bpo-20104: Fix memory leaks and error handling in posix spawn#5418
gpshead merged 9 commits into
python:masterfrom
pablogsal:bpo20104

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

pablogsal commented Jan 29, 2018 •
edited
Loading

pablogsal commented Jan 29, 2018 •
edited
Loading