Description

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'

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

1. cdgriffith commented on Mar 6, 2021

   

   cdgriffithmannequin

   on Mar 6, 2021

   MannequinAuthor

   More actions

   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.
2. cdgriffith

   added

   3.8 (EOL)end of lifeend of life

   3.9 (EOL)end of lifeend of life

   3.10 (EOL)end of lifeend of life

   stdlibStandard Library Python modules in the Lib/ directoryStandard Library Python modules in the Lib/ directory

   type-crashA hard crash of the interpreter, possibly with a core dumpA hard crash of the interpreter, possibly with a core dump

   on Mar 6, 2021
3. eryksun commented on Mar 6, 2021

   

   eryksun

   on Mar 6, 2021

   Contributor

   More actions

   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.
4. eryksun

   added

   OS-windows

   on Mar 6, 2021
5. gpshead commented on Mar 11, 2021

   

   gpshead

   on Mar 11, 2021

   Member

   More actions

   New changeset b4fc44b by Chris Griffith in branch 'master':
   bpo-43423 Fix IndexError in subprocess _communicate function (GH-24777)
   b4fc44b
6. gpshead commented on Mar 11, 2021

   

   gpshead

   on Mar 11, 2021

   Member

   More actions

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

   closed this as completedon Mar 11, 2021
8. gpshead

   closed this as completedon Mar 11, 2021
9. miss-islington commented on Mar 12, 2021

   

   miss-islington

   on Mar 12, 2021

   Contributor

   More actions

   New changeset 1a5001c by Miss Islington (bot) in branch '3.8':
   bpo-43423 Fix IndexError in subprocess _communicate function (GH-24777)
   1a5001c
10. miss-islington commented on Mar 12, 2021

    

    miss-islington

    on Mar 12, 2021

    Contributor

    More actions

    New changeset ad83fde by Miss Islington (bot) in branch '3.9':
    bpo-43423 Fix IndexError in subprocess _communicate function (GH-24777)
    ad83fde
11. ezio-melotti

    transferred this issue fromon Apr 10, 2022

Issue actions