bpo-43423 Fix IndexError in subprocess _communicate function #24777
Conversation
This file contains 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
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). CLA MissingOur records indicate the following people have not signed the CLA: For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. If you have recently signed the CLA, please wait at least one business day You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
gpshead
reviewed
Mar 10, 2021
| stdout = stdout[0] | ||
| if stderr is not None: | ||
| if stderr: | ||
| stderr = stderr[0] | ||
|
|
||
| return (stdout, stderr) |
There was a problem hiding this comment.
There's an additional bug exposed by the above fix. This line could now return ([], []) instead of (None, None).
Changing the above translation logic to
stdout = stdout[0] if stdout else None
stderr = stderr[0] if stdout else None
would work. call this option [A[.
though nicer code all around would be to not use _stdXXX_buff = [] at all but instead refactor _readerthread to take an attribute name to assign to instead of passing it a list to shove a single element in.
something similar to:
def _readerthread(self, fh, attr_name):
try:
setattr(self, attr_name, fh.read())
finally:
fh.close()
that larger refactoring makes sense in the context of 3.10. call this option [B].
as a bugfix for 3.9 and earlier i'd stick with the modification of the stdout and stderr assignments. The time when this case even happens looks to be only when the timeout has expired on the joins() above. So what's broken is timeout logic on Windows during a timeout when collecting stdout and stderr.
lets go ahead with [A] in this PR and a followup PR can do [B].
There was a problem hiding this comment.
Updated to option A
There was a problem hiding this comment.
The time when this case even happens looks to be only when the timeout has expired on the joins() above. So what's broken is timeout logic on Windows during a timeout when collecting stdout and stderr.
If either thread is still alive, then a TimeoutExpired will be raised. The only time there is a problem is if one or more reader threads has failed, either by being forcibly terminated via TerminateThread (not exposed to Python code, except via ctypes) or if the fh.read() call fails, which is most likely due to another thread calling CancelSynchronousIo (also only exposed to Python code via ctypes).
|
Looks good. Can you add a NEWS entry? https://blurb-it.herokuapp.com/ should be able to do it if you don't have blurb installed. |
gpshead
approved these changes
Mar 11, 2021
|
Thanks @cdgriffith for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9. |
|
GH-24823 is a backport of this pull request to the 3.9 branch. |
miss-islington
pushed a commit
to miss-islington/cpython
that referenced
this pull request
Mar 11, 2021
|
GH-24824 is a backport of this pull request to the 3.8 branch. |
miss-islington
pushed a commit
to miss-islington/cpython
that referenced
this pull request
Mar 11, 2021
sthagen
added a commit
to sthagen/python-cpython
that referenced
this pull request
Mar 11, 2021
bpo-43423 Fix IndexError in subprocess _communicate function (pythonGH-24777)
|
|
Thanks @cdgriffith for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9. |
|
Thanks @cdgriffith for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8. |
|
GH-24830 is a backport of this pull request to the 3.8 branch. |
miss-islington
pushed a commit
to miss-islington/cpython
that referenced
this pull request
Mar 12, 2021
|
GH-24831 is a backport of this pull request to the 3.9 branch. |
miss-islington
pushed a commit
to miss-islington/cpython
that referenced
this pull request
Mar 12, 2021
miss-islington
added a commit
that referenced
this pull request
Mar 12, 2021
Check to make sure stdout and stderr are not empty before selecting an item from them in Windows subprocess._communicate. Co-authored-by: Gregory P. Smith <greg@krypto.org> (cherry picked from commit b4fc44b) Co-authored-by: Chris Griffith <chris@cdgriffith.com>
miss-islington
added a commit
that referenced
this pull request
Mar 12, 2021
Check to make sure stdout and stderr are not empty before selecting an item from them in Windows subprocess._communicate. Co-authored-by: Gregory P. Smith <greg@krypto.org> (cherry picked from commit b4fc44b) Co-authored-by: Chris Griffith <chris@cdgriffith.com>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
It is possible to run into an IndexError in the subprocess module's _communicate function.
The lines in question are:
I believe this is due to the fact there is no safety checking to make sure that
self._stdout_buffandself._stderr_buffhave any content in them after being set to empty lists.The fix I suggest is to change the checks from
if stdout is not Noneto simplyif stdoutto make sure it is a populated list, as well as forstderr.Example fixed code:
If a more stringent check is required, we could expand that out to check type and length, such as
isinstance(stdout, list) and len(stdout) > 0:but that is more then necessary currently.https://bugs.python.org/issue43423