
This issue trackerhas been migrated toGitHub, and is currentlyread-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.
Created on2017-12-10 17:14 byizbyshev, last changed2022-04-11 14:58 byadmin. This issue is nowclosed.
| 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.pyimport osimport subprocessimport sysfd = 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.pyHello stdoutTraceback (most recent call last): File "<string>", line 1, in <module>OSError: [Errno 9] Bad file descriptorI 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)*![]() | 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)*![]() | 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)*![]() | Date: 2018-03-25 19:01 | |
This bug stems from:https://github.com/python/cpython/blob/master/Modules/_posixsubprocess.c#L454When 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/b89b52f28490b69142d5c061604b3a3989cec66cFeel free to use the test if you don't like the approach :) | |||
| msg324968 -(view) | Author: Gregory P. Smith (gregory.p.smith)*![]() | Date: 2018-09-11 00:46 | |
New changesetce34410b8b67f49d8275c05d51b3ead50cf97f48 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)*![]() | Date: 2018-09-11 04:36 | |
New changeset9c4a63fc17efea31ad41f90d28825be37469e0e2 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)*![]() | Date: 2018-09-11 06:00 | |
New changeset2173bb818c6c726d831b106ed0d3fad7825905dc 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 |
| 2022-04-11 14:58:55 | admin | set | github: 76451 |
| 2018-09-11 22:52:35 | izbyshev | set | status: open -> closed resolution: fixed messages: +msg325087 stage: patch review -> resolved |
| 2018-09-11 06:00:54 | gregory.p.smith | set | messages: +msg324992 |
| 2018-09-11 04:36:24 | gregory.p.smith | set | messages: +msg324980 |
| 2018-09-11 00:46:47 | miss-islington | set | pull_requests: +pull_request8596 |
| 2018-09-11 00:46:37 | miss-islington | set | pull_requests: +pull_request8595 |
| 2018-09-11 00:46:26 | gregory.p.smith | set | messages: +msg324968 |
| 2018-04-11 22:06:45 | izbyshev | set | messages: +msg315202 |
| 2018-03-25 19:35:31 | gregory.p.smith | set | keywords: +patch stage: patch review pull_requests: +pull_request5977 |
| 2018-03-25 19:12:04 | izbyshev | set | messages: +msg314428 |
| 2018-03-25 19:01:27 | gregory.p.smith | set | assignee:gregory.p.smith messages: +msg314426 versions: + Python 3.8 |
| 2018-03-25 13:12:42 | martin.panter | set | nosy: +martin.panter |
| 2017-12-12 23:38:30 | pitrou | set | messages: +msg308173 |
| 2017-12-10 22:03:36 | izbyshev | set | messages: +msg307995 |
| 2017-12-10 21:35:05 | nitishch | set | nosy: +nitishch |
| 2017-12-10 18:38:47 | pitrou | set | messages: +msg307967 |
| 2017-12-10 17:14:06 | izbyshev | create | |