bpo-20104: Improve error handling and fix a reference leak in os.posix_spawn(). by serhiy-storchaka · Pull Request #6332 · python/cpython

serhiy-storchaka · 2018-04-01T16:23:36Z

https://bugs.python.org/issue20104

pablogsal · 2018-04-01T16:27:06Z

Should I close #6331 in favour of this one?

gpshead

I made a couple of commits to fixup trivial issues.

gpshead · 2018-04-01T17:44:16Z

@@ -0,0 +1 @@
+Fixed numerous issues with :func:`os.posix_spawn()`.


This is not specific and meaningful to the reader. Explain what Python user visible changes were made that a reader would encounter. examples:

"Improved error handling in os.posix_spawn"
"Fixes a potential crash in the new os.posix_spawn implementation" (is that true?)

I'd go with Improved error handling and fixed a reference leak in :func:`os.posix_spawn()`.

gpshead · 2018-04-01T17:51:13Z

       like posix.environ. */

-    if (!PySequence_Check(argv)) {
+    if (!PyList_Check(argv) && !PyTuple_Check(argv)) {


PySequence_Check was correct. There is no need to force this to be exactly a list or a tuple as parse_file_actions uses the sequence APIs.

The docstring says that it should a tuple or a list, and other similar APIs accept only tuples and lists. If there is a need of supporting other sequences, it is better to implement this feature consistently for all APIs and in a separate issue.

Actually accepting arbitrary sequences can have negative consequences. str and bytes are sequences, and passing them by mistake will cause raising errors with unhelpful messages.

gpshead · 2018-04-01T17:56:36Z

+            goto fail;
+        }
+        int overflow;
+        long tag = PyLong_AsLongAndOverflow(PyTuple_GET_ITEM(file_action, 0),


You never use overflow. Just call PyLong_AsLong() as the original code did.

I just don't like if different exceptions are raised for different incorrect values of the same type. But this is not critical.

Actually I think that it would be better to make POSIX_SPAWN_OPEN etc just singletons and test for identity instead of converting them to C long.

gpshead · 2018-04-01T18:15:00Z

+
+    for (int i = 0; i < PySequence_Fast_GET_SIZE(seq); ++i) {
+        file_action = PySequence_Fast_GET_ITEM(seq, i);
+        Py_INCREF(file_action);


Why are you INCREFing this? Borrowing the reference is the normal thing to do when looping over a sequence in C.

Get rid of the INCREF and you don't have to worry about the DECREFs.

This is a borrowed reference. The code inside a loop can call an arbitrary Python code, and this can invalidate borrowed references.

bedevere-bot · 2018-04-01T18:23:36Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

zware

Added my $0.02USD on the blurb contents below, and @gpshead's suggestions look sound to me as well. Otherwise, this looks like a nice cleanup.

zware · 2018-04-10T02:18:31Z

@@ -0,0 +1 @@
+Fixed numerous issues with :func:`os.posix_spawn()`.


I'd go with Improved error handling and fixed a reference leak in :func:`os.posix_spawn()`.

serhiy-storchaka

Thank you for fixing these issues and for documenting the new function @gpshead.

But increfing a borrowed reference is neccessary, and supporting other sequences as argv can open a can of worms. It should be discussed separately if there is a need of this feature.

serhiy-storchaka · 2018-04-10T12:19:53Z

+
+    for (int i = 0; i < PySequence_Fast_GET_SIZE(seq); ++i) {
+        file_action = PySequence_Fast_GET_ITEM(seq, i);
+        Py_INCREF(file_action);


This is a borrowed reference. The code inside a loop can call an arbitrary Python code, and this can invalidate borrowed references.

serhiy-storchaka · 2018-04-10T12:25:51Z

       like posix.environ. */

-    if (!PySequence_Check(argv)) {
+    if (!PyList_Check(argv) && !PyTuple_Check(argv)) {


The docstring says that it should a tuple or a list, and other similar APIs accept only tuples and lists. If there is a need of supporting other sequences, it is better to implement this feature consistently for all APIs and in a separate issue.

Actually accepting arbitrary sequences can have negative consequences. str and bytes are sequences, and passing them by mistake will cause raising errors with unhelpful messages.

serhiy-storchaka · 2018-04-10T12:31:20Z

+            goto fail;
+        }
+        int overflow;
+        long tag = PyLong_AsLongAndOverflow(PyTuple_GET_ITEM(file_action, 0),


I just don't like if different exceptions are raised for different incorrect values of the same type. But this is not critical.

Actually I think that it would be better to make POSIX_SPAWN_OPEN etc just singletons and test for identity instead of converting them to C long.

serhiy-storchaka · 2018-04-10T13:12:40Z

Added new tests (mainly inspired by tests for the third-party module https://github.com/projectcalico/posix_spawn) and improved the documentation.

vstinner · 2018-04-26T11:30:44Z

@gpshead: @serhiy-storchaka updated his PR and replied to your comments, would you mind to review it again?

@serhiy-storchaka: Travis CI is unhappy. Did you check why?

This PR fixes a reference leak which makes the Linux Refleaks buildbot red: https://bugs.python.org/issue33357

cc @pablogsal

Note: I didn't review the PR.

bedevere-bot · 2018-05-01T13:45:07Z

@serhiy-storchaka: Please replace # with GH- in the commit message next time. Thanks!

miss-islington · 2018-05-01T13:45:07Z

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

…x_spawn(). (pythonGH-6332) (cherry picked from commit ef34753) Co-authored-by: Serhiy Storchaka <[email protected]>

bedevere-bot · 2018-05-01T13:45:22Z

GH-6673 is a backport of this pull request to the 3.7 branch.

…x_spawn(). (GH-6332) (cherry picked from commit ef34753) Co-authored-by: Serhiy Storchaka <[email protected]>

bpo-20104: Fix numerous issues with os.posix_spawn().

98ca32d

serhiy-storchaka added type-bug An unexpected behavior, bug, or error DO-NOT-MERGE labels Apr 1, 2018

the-knights-who-say-ni added the CLA signed label Apr 1, 2018

bedevere-bot added the awaiting merge label Apr 1, 2018

gpshead added 2 commits April 1, 2018 10:58

just use PyLong_AsLong, overflow wasn't used.

7437d25

Always Py_DECREF(path), not just on error.

a9d86bf

gpshead requested changes Apr 1, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Apr 1, 2018

gpshead assigned gpshead and unassigned gpshead Apr 1, 2018

gpshead added the needs backport to 3.7 label Apr 1, 2018

Fix the style of tests.

6839f0b

gpshead mentioned this pull request Apr 1, 2018

bpo-33191: Fix refleak in posix_spawn #6315

Closed

zware reviewed Apr 10, 2018

View reviewed changes

serhiy-storchaka added 4 commits April 10, 2018 15:13

Add more tests.

6b46b48

Merge branch 'master' into posix_spawn

0e2cb80

Update a news entry.

7474141

Improve docs formatting and describe file actions.

b5a8c7b

serhiy-storchaka commented Apr 10, 2018

View reviewed changes

serhiy-storchaka removed the DO-NOT-MERGE label Apr 11, 2018

serhiy-storchaka added 3 commits April 26, 2018 15:05

Merge branch 'master' into posix_spawn

23ce8e3

Try to fix tests for no_such_executable.

5da8953

Yet one attempt to fix a test.

a3e1fae

serhiy-storchaka changed the title ~~bpo-20104: Fix numerous issues with os.posix_spawn().~~ bpo-20104: Improve error handling and fix a reference leak in os.posix_spawn(). Apr 26, 2018

serhiy-storchaka merged commit ef34753 into python:master May 1, 2018

bedevere-bot removed the awaiting changes label May 1, 2018

serhiy-storchaka deleted the posix_spawn branch May 1, 2018 13:45

bedevere-bot removed the needs backport to 3.7 label May 1, 2018

miss-islington added a commit that referenced this pull request May 1, 2018

bpo-20104: Improve error handling and fix a reference leak in os.posi…

77fa783

…x_spawn(). (GH-6332) (cherry picked from commit ef34753) Co-authored-by: Serhiy Storchaka <[email protected]>

		@@ -0,0 +1 @@
		Fixed numerous issues with :func:`os.posix_spawn()`.

Uh oh!

Conversation

serhiy-storchaka commented Apr 1, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablogsal commented Apr 1, 2018

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!

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 Apr 1, 2018

Uh oh!

zware left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka 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!

serhiy-storchaka commented Apr 10, 2018

Uh oh!

vstinner commented Apr 26, 2018

Uh oh!

bedevere-bot commented May 1, 2018

Uh oh!

miss-islington commented May 1, 2018

Uh oh!

bedevere-bot commented May 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

serhiy-storchaka commented Apr 1, 2018 •

edited by bedevere-bot

Loading