Repository navigation
Subprocess IndexError possible in _communicate #87589
Description
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2021-03-11.19:51:50.937>
created_at = <Date 2021-03-06.18:52:42.452>
labels = ['3.8', 'type-crash', '3.10', 'library', 'OS-windows', '3.9']
title = 'Subprocess IndexError possible in _communicate'
updated_at = <Date 2021-03-12.01:56:49.605>
user = 'https://github.com/cdgriffith'bugs.python.org fields:
activity = <Date 2021-03-12.01:56:49.605>
actor = 'miss-islington'
assignee = 'none'
closed = True
closed_date = <Date 2021-03-11.19:51:50.937>
closer = 'gregory.p.smith'
components = ['Library (Lib)', 'Windows']
creation = <Date 2021-03-06.18:52:42.452>
creator = 'cdgriffith'
dependencies = []
files = []
hgrepos = []
issue_num = 43423
keywords = ['patch']
message_count = 6.0
messages = ['388211', '388213', '388520', '388521', '388531', '388533']
nosy_count = 8.0
nosy_names = ['gregory.p.smith', 'paul.moore', 'tim.golden', 'zach.ware', 'eryksun', 'steve.dower', 'cdgriffith', 'miss-islington']
pr_nums = ['24777', '24824', '24823', '24830', '24831']
priority = 'normal'
resolution = 'fixed'
stage = 'commit review'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue43423'
versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']
Activity
cdgriffith commented on Mar 6, 2021
It is possible to run into an IndexError in the subprocess module's _communicate function.
The lines in question are:
if stdout is not None: stdout = stdout[0] if stderr is not None: stderr = stderr[0]I believe this is due to the fact there is no safety checking to make sure that self._stdout_buff and self._stderr_buff have any content in them after being set to empty lists.
The fix I suggest is to change the checks from if stdout is not None to simply if stdout to make sure it is a populated list.
Example fixed code:
if stdout: stdout = stdout[0] if stderr: stderr = stderr[0]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.
eryksun commented on Mar 6, 2021
The presumption I suppose is that these statements only execute if self.stdout_thread and/or self.stderr_thread completes successfully. I suppose that the read could fail or get canceled via CancelSynchronousIo(). Of course in that case you have a bigger problem than an IndexError.
On a related note, _communicate() needs significant changes in Windows. See bpo-43346 if you're interested.
gpshead commented on Mar 11, 2021
New changeset b4fc44b by Chris Griffith in branch 'master':
bpo-43423 Fix IndexError in subprocess _communicate function (GH-24777)
b4fc44b
gpshead commented on Mar 11, 2021
The bug is fixed, Thanks Chris! There was a refactoring noted as being nice in my comments on the primary main branch PR that would be nice to have. But isn't critical. If you want to make a PR for that, just reuse this bpo-43423 issue number on the PR and assign it to me.
The backports to 3.9 an 3.8 are approved and should automerge after CI finishes.
miss-islington commented on Mar 12, 2021
New changeset 1a5001c by Miss Islington (bot) in branch '3.8':
bpo-43423 Fix IndexError in subprocess _communicate function (GH-24777)
1a5001c
miss-islington commented on Mar 12, 2021
New changeset ad83fde by Miss Islington (bot) in branch '3.9':
bpo-43423 Fix IndexError in subprocess _communicate function (GH-24777)
ad83fde