classification
Title: Subprocess IndexError possible in _communicate
Type: crash Stage: commit review
Components: Library (Lib), Windows Versions: Python 3.10, Python 3.9, Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: cdgriffith, eryksun, gregory.p.smith, miss-islington, paul.moore, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: patch

Created on 2021-03-06 18:52 by cdgriffith, last changed 2021-03-12 01:56 by miss-islington. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 24777 merged cdgriffith, 2021-03-06 18:57
PR 24824 closed miss-islington, 2021-03-11 19:44
PR 24823 closed miss-islington, 2021-03-12 01:30
PR 24830 merged miss-islington, 2021-03-12 01:32
PR 24831 merged miss-islington, 2021-03-12 01:32
Messages (6)
msg388211 - (view) Author: Chris Griffith (cdgriffith) * Date: 2021-03-06 18:52
It is possible to run into an IndexError in the subprocess module's _communicate function.

```
    return run(
  File "subprocess.py", line 491, in run
  File "subprocess.py", line 1024, in communicate
  File "subprocess.py", line 1418, in _communicate
IndexError: list index out of range
```

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.
msg388213 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2021-03-06 20:36
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.
msg388520 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2021-03-11 19:43
New changeset b4fc44bb2d209182390b4f9fdf074a46b0165a2f by Chris Griffith in branch 'master':
bpo-43423 Fix IndexError in subprocess _communicate function (GH-24777)
https://github.com/python/cpython/commit/b4fc44bb2d209182390b4f9fdf074a46b0165a2f
msg388521 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2021-03-11 19:51
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.
msg388531 - (view) Author: miss-islington (miss-islington) Date: 2021-03-12 01:52
New changeset 1a5001c606b55226c03fa1046aa8f5e1db2fa67d by Miss Islington (bot) in branch '3.8':
bpo-43423 Fix IndexError in subprocess _communicate function (GH-24777)
https://github.com/python/cpython/commit/1a5001c606b55226c03fa1046aa8f5e1db2fa67d
msg388533 - (view) Author: miss-islington (miss-islington) Date: 2021-03-12 01:56
New changeset ad83fde75463dad2df878ff264f52436eb48bc6b by Miss Islington (bot) in branch '3.9':
bpo-43423 Fix IndexError in subprocess _communicate function (GH-24777)
https://github.com/python/cpython/commit/ad83fde75463dad2df878ff264f52436eb48bc6b
History
Date User Action Args
2021-03-12 01:56:49miss-islingtonsetmessages: + msg388533
2021-03-12 01:53:00miss-islingtonsetmessages: + msg388531
2021-03-12 01:32:11miss-islingtonsetpull_requests: + pull_request23596
2021-03-12 01:32:03miss-islingtonsetpull_requests: + pull_request23595
2021-03-12 01:30:02miss-islingtonsetpull_requests: + pull_request23594
2021-03-11 19:51:50gregory.p.smithsetstatus: open -> closed
resolution: fixed
messages: + msg388521

stage: patch review -> commit review
2021-03-11 19:44:21miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request23589
2021-03-11 19:43:49gregory.p.smithsetnosy: + gregory.p.smith
messages: + msg388520
2021-03-06 20:36:26eryksunsetnosy: + paul.moore, tim.golden, eryksun, zach.ware, steve.dower
messages: + msg388213
components: + Windows
2021-03-06 18:57:39cdgriffithsetkeywords: + patch
stage: patch review
pull_requests: + pull_request23542
2021-03-06 18:52:42cdgriffithcreate