classification
Title: subprocess closes redirected fds even if they are in pass_fds
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: gregory.p.smith, izbyshev, martin.panter, nitishch, pitrou, vstinner
Priority: normal Keywords: patch

Created on 2017-12-10 17:14 by izbyshev, last changed 2018-09-11 22:52 by izbyshev. This issue is now closed.

Files
File name Uploaded Description Edit
test.py izbyshev, 2017-12-10 17:14
Pull Requests
URL Status Linked Edit
PR 6242 merged gregory.p.smith, 2018-03-25 19:35
PR 9148 merged miss-islington, 2018-09-11 00:46
PR 9149 merged miss-islington, 2018-09-11 00:46
Messages (11)
msg307962 - (view) Author: Alexey Izbyshev (izbyshev) * Date: 2017-12-10 17:14
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.
msg307967 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-12-10 18:38
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?
msg307995 - (view) Author: Alexey Izbyshev (izbyshev) * Date: 2017-12-10 22:03
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.
msg308173 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-12-12 23:38
> 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.
msg314426 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-03-25 19:01
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
msg314428 - (view) Author: Alexey Izbyshev (izbyshev) * Date: 2018-03-25 19:12
Actually I've started to work on this with #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.
msg315202 - (view) Author: Alexey Izbyshev (izbyshev) * Date: 2018-04-11 22:06
An alternative fix is here: https://github.com/izbyshev/cpython/commit/b89b52f28490b69142d5c061604b3a3989cec66c

Feel free to use the test if you don't like the approach :)
msg324968 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-09-11 00:46
New changeset ce34410b8b67f49d8275c05d51b3ead50cf97f48 by Gregory P. Smith in branch 'master':
bpo-32270: Don't close stdin/out/err in pass_fds (GH-6242)
https://github.com/python/cpython/commit/ce34410b8b67f49d8275c05d51b3ead50cf97f48
msg324980 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-09-11 04:36
New changeset 9c4a63fc17efea31ad41f90d28825be37469e0e2 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)
https://github.com/python/cpython/commit/9c4a63fc17efea31ad41f90d28825be37469e0e2
msg324992 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-09-11 06:00
New changeset 2173bb818c6c726d831b106ed0d3fad7825905dc 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)
https://github.com/python/cpython/commit/2173bb818c6c726d831b106ed0d3fad7825905dc
msg325087 - (view) Author: Alexey Izbyshev (izbyshev) * Date: 2018-09-11 22:52
Thank you, Gregory!
History
Date User Action Args
2018-09-11 22:52:35izbyshevsetstatus: open -> closed
resolution: fixed
messages: + msg325087

stage: patch review -> resolved
2018-09-11 06:00:54gregory.p.smithsetmessages: + msg324992
2018-09-11 04:36:24gregory.p.smithsetmessages: + msg324980
2018-09-11 00:46:47miss-islingtonsetpull_requests: + pull_request8596
2018-09-11 00:46:37miss-islingtonsetpull_requests: + pull_request8595
2018-09-11 00:46:26gregory.p.smithsetmessages: + msg324968
2018-04-11 22:06:45izbyshevsetmessages: + msg315202
2018-03-25 19:35:31gregory.p.smithsetkeywords: + patch
stage: patch review
pull_requests: + pull_request5977
2018-03-25 19:12:04izbyshevsetmessages: + msg314428
2018-03-25 19:01:27gregory.p.smithsetassignee: gregory.p.smith
messages: + msg314426
versions: + Python 3.8
2018-03-25 13:12:42martin.pantersetnosy: + martin.panter
2017-12-12 23:38:30pitrousetmessages: + msg308173
2017-12-10 22:03:36izbyshevsetmessages: + msg307995
2017-12-10 21:35:05nitishchsetnosy: + nitishch
2017-12-10 18:38:47pitrousetmessages: + msg307967
2017-12-10 17:14:06izbyshevcreate