bpo-32270: Don't close stdin/out/err in pass_fds #6242
Merged
Conversation
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
gpshead
added
type-bug
izbyshev
reviewed
Mar 26, 2018
Modules/_posixsubprocess.c
Outdated
| /* Close pipe fds. Make sure we don't close the same fd more than */ | ||
| /* once, or standard fds. */ | ||
| if (p2cread > 2) | ||
| if (p2cread > 2 && !_is_fd_in_sorted_fd_sequence(p2cread, py_fds_to_keep)) |
Contributor
There was a problem hiding this comment.
Note that there is no need to manually close p2cread at all, and other two descriptors need to be closed only if they are the result of dup above. This is because if a descriptor is created by subprocess.py (a pipe or for /dev/null), it's non-inheritable, and if a descriptor is specified by the user, is not in pass_fds and is > 2, it will be closed by _close_open_fds. So my original idea of fixing this issue was to add an async-signal-safe version of _Py_dup and use it instead of dup above, and then just remove these three checks.
Note also that p2cwrite and its three friends are never required to be manually closed since they are always non-inheritable pipe ends created by subprocess.py.
Member
Author
There was a problem hiding this comment.
Not surprising. The original code here predates our non-inheritable file descriptor implementation. I'm examining this vs your izbyshev@b89b52f changes to see what makes the most sense.
This change as is does make it pass your unittest.
Member
When subprocess.Popen() stdin= stdout= or stderr= handles are specified and appear in pass_fds=, don't close the original fds after dup'ing them.
gpshead
force-pushed
the
subprocess-32270
branch
from
b883088 to
657fd10
Compare
Taken from izbyshev@b89b52f per the suggestion in the BPO issue comment.
Based on code review by izbyshev@ and izbyshev@b89b52f. This also removes the old manual p2cread, c2pwrite, and errwrite closing logic as inheritable flags and _close_open_fds takes care of them properly today. This code is within child_exec() where it is the only thread so there is no race condition between the dup and _Py_set_inheritable_async_safe call.
Member
Author
|
With the latest changes, this PR is essentially @izbyshev's code from their own branch. :) Thanks for the eyeballs and review! |
Contributor
|
Thanks @gpshead for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7. |
miss-islington
pushed a commit
to miss-islington/cpython
that referenced
this pull request
Sep 11, 2018
When subprocess.Popen() stdin= stdout= or stderr= handles are specified and appear in pass_fds=, don't close the original fds after dup'ing them. This implementation and unittest primarily came from @izbyshev (see the PR) See also izbyshev@b89b52f This also removes the old manual p2cread, c2pwrite, and errwrite closing logic as inheritable flags and _close_open_fds takes care of that properly today without special treatment. This code is within child_exec() where it is the only thread so there is no race condition between the dup and _Py_set_inheritable_async_safe call. (cherry picked from commit ce34410) Co-authored-by: Gregory P. Smith <[email protected]>
|
GH-9148 is a backport of this pull request to the 3.7 branch. |
|
GH-9149 is a backport of this pull request to the 3.6 branch. |
miss-islington
pushed a commit
to miss-islington/cpython
that referenced
this pull request
Sep 11, 2018
When subprocess.Popen() stdin= stdout= or stderr= handles are specified and appear in pass_fds=, don't close the original fds after dup'ing them. This implementation and unittest primarily came from @izbyshev (see the PR) See also izbyshev@b89b52f This also removes the old manual p2cread, c2pwrite, and errwrite closing logic as inheritable flags and _close_open_fds takes care of that properly today without special treatment. This code is within child_exec() where it is the only thread so there is no race condition between the dup and _Py_set_inheritable_async_safe call. (cherry picked from commit ce34410) Co-authored-by: Gregory P. Smith <[email protected]>
gpshead
pushed a commit
that referenced
this pull request
Sep 11, 2018
When subprocess.Popen() stdin= stdout= or stderr= handles are specified and appear in pass_fds=, don't close the original fds after dup'ing them. This implementation and unittest primarily came from @izbyshev (see the PR) See also izbyshev@b89b52f This also removes the old manual p2cread, c2pwrite, and errwrite closing logic as inheritable flags and _close_open_fds takes care of that properly today without special treatment. This code is within child_exec() where it is the only thread so there is no race condition between the dup and _Py_set_inheritable_async_safe call. (cherry picked from commit ce34410) Co-authored-by: Gregory P. Smith <[email protected]> [Google]
gpshead
pushed a commit
that referenced
this pull request
Sep 11, 2018
When subprocess.Popen() stdin= stdout= or stderr= handles are specified and appear in pass_fds=, don't close the original fds after dup'ing them. This implementation and unittest primarily came from @izbyshev (see the PR) See also izbyshev@b89b52f This also removes the old manual p2cread, c2pwrite, and errwrite closing logic as inheritable flags and _close_open_fds takes care of that properly today without special treatment. This code is within child_exec() where it is the only thread so there is no race condition between the dup and _Py_set_inheritable_async_safe call. (cherry picked from commit ce34410) Co-authored-by: Gregory P. Smith <[email protected]> [Google]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
When subprocess.Popen() stdin= stdout= or stderr= handles are specified and
appear in pass_fds=, don't close their original fds after dup2'ing them to 0/1/2.
I still need to create a proper unittest and NEWS entry before this can be merged.
https://bugs.python.org/issue32270