Skip to content

/ cpython

New issue

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

bpo-35182: fix communicate() crash after child closes its pipes #17020

Closed
wants to merge 1 commit into from
Closed

bpo-35182: fix communicate() crash after child closes its pipes #17020

wants to merge 1 commit into from
+15 −2

Conversation

@and800
Copy link
Contributor

and800 commented Oct 31, 2019 •
edited by bedevere-bot

When communicate() is called in a loop, it crashes when the child process has already closed any piped standard stream, but still continues to be running. The fix is trivial.

https://bugs.python.org/issue35182
@and800 and800 requested a review from gpshead as a code owner Oct 31, 2019
and800 added a commit to and800/cpython that referenced this pull request Oct 31, 2019
@and800
bpo-35182: fix communicate() crash after child closes its pipes (pyth…
abcec29 
…onGH-17020)

When communicate() is called in a loop, it crashes when the child process
has already closed any piped standard stream, but still continues to be running
@and800 and800 force-pushed the and800:subprocess branch from 6587fec to abcec29 Oct 31, 2019
@and800
bpo-35182: fix communicate() crash after child closes its pipes (GH-1…
Loading status checks…
b644be1 
…7020)

When communicate() is called in a loop, it crashes when the child process
has already closed any piped standard stream, but still continues to be running
@and800 and800 force-pushed the and800:subprocess branch from abcec29 to b644be1 Oct 31, 2019
@codecov

This comment has been minimized.

Sign in to view
Copy link

codecov bot commented Oct 31, 2019 •
edited

Codecov Report

❗️ No coverage uploaded for pull request base (master@112f2b8). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #17020   +/-   ##
=========================================
  Coverage          ?   82.13%           
=========================================
  Files             ?     1951           
  Lines             ?   580578           
  Branches          ?    44252           
=========================================
  Hits              ?   476847           
  Misses            ?    94109           
  Partials          ?     9622
Impacted Files Coverage Δ
Lib/subprocess.py 58.53% <100%> (ø)
Lib/test/test_subprocess.py 85.36% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 112f2b8...b644be1. Read the comment docs.
@and800

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

and800 commented Oct 31, 2019

please port it to 3.7 & 3.8
and800 added a commit to and800/cpython that referenced this pull request Oct 31, 2019
@and800
[2.7] bpo-35182: fix communicate() crash after child closes its pipes (…
66c1e1a 
…pythonGH-17020)

When communicate() is called in a loop, it crashes when the child process
has already closed any piped standard stream, but still continues to be running
@and800 and800 mentioned this pull request Oct 31, 2019
Closed
@alpire

This comment has been minimized.

Sign in to view
Copy link
Contributor

alpire commented Jan 21, 2020

Hi, I'm hitting this issue in production. Is there anything I could do to help get this merged?
@gpshead

This comment has been minimized.

Sign in to view
Copy link
Member

gpshead commented Jan 21, 2020

can you resolve the test_subprocess.py merge conflict?
alpire added a commit to alpire/cpython that referenced this pull request Jan 22, 2020
@and800 @alpire
bpo-35182: fix communicate() crash after child closes its pipes (pyth…
35fd4c9 
…onGH-17020)

When communicate() is called in a loop, it crashes when the child process
has already closed any piped standard stream, but still continues to be running
@alpire alpire mentioned this pull request Jan 22, 2020
Merged
@alpire

This comment has been minimized.

Sign in to view
Copy link
Contributor

alpire commented Jan 22, 2020

@gpshead: Since I didn't have write access here, I created another PR with the merge conflict resolved (#18117). Waiting on the CLA records to update after I signed.

@and800: Hope you don't mind. I'd be happy to close my PR if you wanna take over.
@and800

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

and800 commented Jan 22, 2020

that's ok, let's proceed with your PR
gpshead added a commit that referenced this pull request Jan 22, 2020
@alpire @and800 @gpshead
bpo-35182: fix communicate() crash after child closes its pipes (GH-1…
Loading status checks…
d3ae95e 
…7020) (GH-18117)

When communicate() is called in a loop, it crashes when the child process
has already closed any piped standard stream, but still continues to be running

Co-authored-by: Andriy Maletsky <andriy.maletsky@gmail.com>
@gpshead

This comment has been minimized.

Sign in to view
Copy link
Member

gpshead commented Jan 22, 2020

thanks! other PR merged. :)
@gpshead gpshead closed this Jan 22, 2020
alpire added a commit to alpire/cpython that referenced this pull request Jan 23, 2020
@alpire
[3.8] bpo-35182: fix communicate() crash after child closes its pipes (…
d2f16b5 
…pythonGH-17020) (pythonGH-18117)

When communicate() is called in a loop, it crashes when the child process
has already closed any piped standard stream, but still continues to be running

Co-authored-by: Andriy Maletsky <andriy.maletsky@gmail.com>.
(cherry picked from commit d3ae95e)

Co-authored-by: Alex Rebert <alex@forallsecure.com>
@alpire alpire mentioned this pull request Jan 23, 2020
Merged
alpire added a commit to alpire/cpython that referenced this pull request Jan 23, 2020
@alpire
[3.7] bpo-35182: fix communicate() crash after child closes its pipes (…
1520d4d 
…pythonGH-17020) (pythonGH-18117)

When communicate() is called in a loop, it crashes when the child process
has already closed any piped standard stream, but still continues to be running

Co-authored-by: Andriy Maletsky <andriy.maletsky@gmail.com>.
(cherry picked from commit d3ae95e)

Co-authored-by: Alex Rebert <alex@forallsecure.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gpshead gpshead

Assignees
No one assigned
Labels
CLA signed awaiting review
Projects
None yet
Milestone
No milestone
5 participants
@and800 @alpire @gpshead @the-knights-who-say-ni @bedevere-bot
You can’t perform that action at this time.