subprocess closes redirected fds even if they are in pass_fds

[izbyshev]

BPO	32270
Nosy	@gpshead, @pitrou, @vstinner, @vadmium, @izbyshev, @nitishch
PRs	bpo-32270: Don't close stdin/out/err in pass_fds #6242 [3.7] bpo-32270: Don't close stdin/out/err in pass_fds (GH-6242) #9148 [3.6] bpo-32270: Don't close stdin/out/err in pass_fds (GH-6242) #9149
Files	test.py

^{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 = 'https://github.com/gpshead'
closed_at = <Date 2018-09-11.22:52:35.695>
created_at = <Date 2017-12-10.17:14:06.586>
labels = ['3.7', '3.8', 'type-bug', 'library']
title = 'subprocess closes redirected fds even if they are in pass_fds'
updated_at = <Date 2018-09-11.22:52:35.694>
user = 'https://github.com/izbyshev'

bugs.python.org fields:

activity = <Date 2018-09-11.22:52:35.694>
actor = 'izbyshev'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2018-09-11.22:52:35.695>
closer = 'izbyshev'
components = ['Library (Lib)']
creation = <Date 2017-12-10.17:14:06.586>
creator = 'izbyshev'
dependencies = []
files = ['47329']
hgrepos = []
issue_num = 32270
keywords = ['patch']
message_count = 11.0
messages = ['307962', '307967', '307995', '308173', '314426', '314428', '315202', '324968', '324980', '324992', '325087']
nosy_count = 6.0
nosy_names = ['gregory.p.smith', 'pitrou', 'vstinner', 'martin.panter', 'izbyshev', 'nitishch']
pr_nums = ['6242', '9148', '9149']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue32270'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

[izbyshev]

Demonstration:

$ cat test.py
import os
import subprocess
import sys

fd = os.dup(sys.stdout.fileno())
subprocess.call([sys.executable, '-c',
                 'import sys;'
                 'print("Hello stdout");'
                 'print("Hello fd", file=open(int(sys.argv[1]), "w"))',
                 str(fd)],
                stdout=fd,
                pass_fds=[fd])

$ python3 test.py
Hello stdout
Traceback (most recent call last):
  File "<string>", line 1, in <module>
OSError: [Errno 9] Bad file descriptor

I see two issues here:

1. The fact that file descriptors specified for redirection are closed (even with close_fds=False) unless in range [0, 2] is not documented and not tested. If I change the corresponding code in _posixsubprocess.c to close them only if they are ends of pipes created by subprocess itself, all tests still pass. Also, shells behave differently: e.g., in command 'echo 3>&1' fd 3 is duplicated but not closed in the child, so subprocess behavior may be not what people expect.

2. pass_fds interaction with (1) should be fixed and documented. We may either raise ValueError if pass_fds contains a redirected fd or don't close such a redirected fd.

Please provide your thoughts.

[pitrou]

Thanks for the report and the investigation!

1. The fact that file descriptors specified for redirection are closed (even with close_fds=False) unless in range [0, 2] is not documented and not tested.

That sounds like a bug to me, so we should fix it if decently possible.

2. pass_fds interaction with (1) should be fixed and documented. We may either raise ValueError if pass_fds contains a redirected fd or don't close such a redirected fd.

Same here, it seems like a bug which deserves fixing.

Do you want to submit a PR for this?

[izbyshev]

Regarding fixing (1), I'm worrying about backward compatibility a bit. Some people who discovered that behavior might rely on such "move" semantics and expect that the redirected descriptor is not leaked into the child. OTOH, since descriptors are non-inheritable by default since 3.4, it might be less of an issue now. What do you think?

Otherwise, yes, I'd like to fix this, but fixing is a bit trickier than it may seem because sometimes descriptors are duplicated in _posixsubprocess.c (in case of low fds), so we need to track the ownership properly. I've already performed some work and discovered another issue: in a corner case involving closed standard fds, pipes created by subprocess may leak into the child and/or an incorrect redirection may occur. Since I can't completely fix this issue without affecting a discovered one, I think I'll do the opposite: file a new issue, fix it, and then go back to this one.

[pitrou]

Regarding fixing (1), I'm worrying about backward compatibility a bit.

Well, people shouldn't rely on bugs. Otherwise we would never be able to fix bugs, lest someone relies on it.

[gpshead]

This bug stems from:

https://github.com/python/cpython/blob/master/Modules/_posixsubprocess.c#L454

When stdout=fd is passed, that results in c2pwrite=fd. The stdin/stdout/stderr pipe fds are closed when > 2 without checking to see if they are listed in pass_fds.

https://github.com/python/cpython/blob/master/Lib/subprocess.py#L1306 maps the following:
stdin -> p2cread
stdout -> c2pwrite
stderr -> errwrite

[izbyshev]

Actually I've started to work on this with bpo-32844, but got no feedback. This issue may of course be fixed independently, but the problems with descriptor ownership for fds <= 2 will remain unless all ownership problems are fixed at once.

[izbyshev]

An alternative fix is here: izbyshev@b89b52f

Feel free to use the test if you don't like the approach :)

[gpshead]

New changeset ce34410 by Gregory P. Smith in branch 'master':
bpo-32270: Don't close stdin/out/err in pass_fds (GH-6242)
ce34410

[gpshead]

New changeset 9c4a63f by Gregory P. Smith (Miss Islington (bot)) in branch '3.7':
bpo-32270: Don't close stdin/out/err in pass_fds (GH-6242) (GH-9148)
9c4a63f

[gpshead]

New changeset 2173bb8 by Gregory P. Smith (Miss Islington (bot)) in branch '3.6':
bpo-32270: Don't close stdin/out/err in pass_fds (GH-6242) (GH-9149)
2173bb8