Skip to content
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.

Sign up for GitHub

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-43423 Fix IndexError in subprocess _communicate function #24777

Merged
merged 4 commits into from
Mar 11, 2021
Merged

bpo-43423 Fix IndexError in subprocess _communicate function #24777

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cabbc72
Check to make sure stdout and stderr are not empty before selecting i…
cdgriffith Mar 6, 2021
61002a0
Fix that empty lists may be returned instead of None
cdgriffith Mar 11, 2021
9b94b31
📜🤖 Added by blurb_it.
blurb-it[bot] Mar 11, 2021
20eaff8
clarify news wording.
gpshead Mar 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Lib/subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1535,9 +1535,9 @@ def _communicate(self, input, endtime, orig_timeout):
self.stderr.close()

# All data exchanged. Translate lists into strings.
if stdout is not None:
if stdout:
stdout = stdout[0]
if stderr is not None:
if stderr:
stderr = stderr[0]

return (stdout, stderr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to option A

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand Down