[issue43308] subprocess.Popen leaks file descriptors opened for DEVNULL or PIPE stdin/stdout/stderr arguments
New submission from cptpcrd :
TL;DR: subprocess.Popen's handling of file descriptors opened for DEVNULL or
PIPE inputs/outputs has serious problems, and it can be coerced into leaking
file descriptors in several ways. This can cause issues related to resource
exhaustion.
# The basic problem
As part of its setup, Popen.__init__() calls Popen._get_handles(), which looks
at the given stdin/stdout/stderr arguments and returns a tuple of 6 file
descriptors (on Windows, file handles) indicating how stdin/stdout/stderr
should be redirected. However, these file descriptors aren't properly closed if
exceptions occur in certain cases.
# Variant 1: Bad argument errors (introduced in 3.9)
The first variant of this bug is shockingly easy to reproduce (note that this
only works on platforms with /proc/self/fd, like Linux):
```
import os, subprocess
def show_fds():
for entry in os.scandir("/proc/self/fd"):
print(entry.name, "->", os.readlink(entry.path))
print("Before:")
show_fds()
try:
subprocess.Popen(["ls"], stdin=subprocess.PIPE, user=1.0)
except TypeError as e: # "User must be a string or an integer"
print(e)
print("After:")
show_fds()
```
This produces something like:
```
Before:
0 -> /dev/pts/1
1 -> /dev/pts/1
2 -> /dev/pts/1
3 -> /proc/12345/fd
User must be a string or an integer
After:
0 -> /dev/pts/1
1 -> /dev/pts/1
2 -> /dev/pts/1
3 -> pipe:[1234567]
3 -> pipe:[1234567]
5 -> /proc/12345/fd
```
The process never got launched (because of the invalid `user` argument), but
the (unused) pipe created for piping to stdin is left open! Substituting
DEVNULL for PIPE instead leaves a single file descriptor open to `/dev/null`.
This happens because the code that validates the `user`, `group`, and
`extra_groups` arguments [1] was added to Popen.__init__() *after* the call to
Popen._get_handles() [2], and there isn't a try/except that closes the file
descriptors if an exception gets raised during validation (which can easily
happen).
# Variant 2: Error opening file descriptors (seems to have been around in
`subprocess` forever)
Within Popen._get_handles() (on Windows [3] or POSIX [4]), previously opened
file descriptors are not closed if an error occurs while opening later file
descriptors.
For example, take the case where only one more file descriptor can be opened
without hitting the limit on the number of file descriptors, and
`subprocess.Popen(["ls"], stdin=subprocess.DEVNULL, stdout=supbrocess.PIPE)` is
called. subprocess will be able to open `/dev/null` for stdin, but trying to
creating a `pipe()` for stdout will fail with EMFILE or ENFILE. Since
Popen._get_handles() doesn't handle exceptions from `pipe()` (or when opening
`/dev/null`), the `/dev/null` file descriptor opened for stdin will be be left
open.
This variant is most easily triggered by file descriptor exhaustion, and it
makes that problem worse by leaking even *more* file descriptors.
Here's an example that reproduces this by monkey-patching `os` to force an
error:
```
import os, subprocess
def show_fds():
for entry in os.scandir("/proc/self/fd"):
print(entry.name, "->", os.readlink(entry.path))
print("Before:")
show_fds()
# Trigger an error when trying to open /dev/null
os.devnull = "/NOEXIST"
try:
subprocess.Popen(["ls"], stdin=subprocess.PIPE, stdout=subprocess.DEVNULL)
except FileNotFoundError as e: # "User must be a string or an integer"
print(e)
print("After:")
show_fds()
```
Output:
```
Before:
0 -> /dev/pts/1
1 -> /dev/pts/1
2 -> /dev/pts/1
3 -> /proc/12345/fd
[Errno 2] No such file or directory: '/dev/null'
After:
0 -> /dev/pts/1
1 -> /dev/pts/1
2 -> /dev/pts/1
3 -> pipe:[1234567]
4 -> pipe:[1234567]
5 -> /proc/12345/fd
```
Again, the pipe is left open.
# Paths to fix.
Variant 1 can be fixed by simply reordering code in Popen.__init__() (and
leaving comments warning about the importance of maintaining the order!). I've
attached a basic patch that does this.
Variant 2 might take some more work -- especially given the shared
Popen._devnull file descriptor that needs to be accounted for separately -- and
may require significant changes to both Popen.__init__() and
Popen._get_handles() to fix.
[1]: https://github.com/python/cpython/blob/master/Lib/subprocess.py#L872
[2]: https://github.com/python/cpython/blob/master/Lib/subprocess.py#L840
[3]: https://github.com/python/cpython/blob/master/Lib/subprocess.py#L1251
[4]: https://github.com/python/cpython/blob/master/Lib/subprocess.py#L1581
--
components: Library (Lib)
files: subprocess-validation-fd-leak.patch
keywords: patch
messages: 387589
nosy: cptpcrd
priority: normal
severity: normal
status: open
title: s
[issue42780] os.set_inheritable() fails for O_PATH file descriptors on Linux
cptpcrd added the comment: No problem! I've noticed at least one other (relatively minor) issue in `os`, so I may be submitting further bug reports. I haven't been keeping close track of 3.6/3.7's status, so I added them in without thinking it. Thanks for the reminder. -- ___ Python tracker <https://bugs.python.org/issue42780> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue42780] os.set_inheritable() fails for O_PATH file descriptors on Linux
Change by cptpcrd : -- pull_requests: +23101 pull_request: https://github.com/python/cpython/pull/24278 ___ Python tracker <https://bugs.python.org/issue42780> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue42780] os.set_inheritable() fails for O_PATH file descriptors on Linux
Change by cptpcrd : -- pull_requests: +23100 pull_request: https://github.com/python/cpython/pull/24277 ___ Python tracker <https://bugs.python.org/issue42780> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue42780] os.set_inheritable() fails for O_PATH file descriptors on Linux
Change by cptpcrd : -- pull_requests: +22999 stage: -> patch review pull_request: https://github.com/python/cpython/pull/24172 ___ Python tracker <https://bugs.python.org/issue42780> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue42780] os.set_inheritable() fails for O_PATH file descriptors on Linux
cptpcrd added the comment: I've put together some tests (patch attached). Should I PR this to python/cpython? -- Added file: https://bugs.python.org/file49721/set-inheritable-test.patch ___ Python tracker <https://bugs.python.org/issue42780> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue42780] os.set_inheritable() fails for O_PATH file descriptors on Linux
cptpcrd added the comment: > I like this approach! Should I put together unit tests to go with the patch? Maybe `test_os.FDInheritanceTests.test_set_inheritable_o_path()`? -- ___ Python tracker <https://bugs.python.org/issue42780> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue42780] os.set_inheritable() fails for O_PATH file descriptors on Linux
New submission from cptpcrd :
Note: I filed this bug report after seeing
https://github.com/rust-lang/rust/pull/62425 and verifying that it was also
reproducible on Python. Credit for discovering the underlying issue should go
to Aleksa Sarai, and further discussion can be found there.
# Background
Linux has O_PATH file descriptors. These are file descriptors that refer to a
specific path, without allowing any other kind of access to the file. They
can't be used to read or write data; instead, they're intended to be used for
use cases like the *at() functions. In that respect, they have similar
semantics to O_SEARCH on other platforms (except that they also work on other
file types, not just directories).
More information on O_PATH file descriptors can be found in open(2)
(https://www.man7.org/linux/man-pages/man2/open.2.html), or in the Rust PR
linked above.
# The problem
As documented in the Rust PR linked above, *no* ioctl() calls will succeed on
O_PATH file descriptors (they always fail with EBADF). Since
os.set_inheritable() uses ioctl(FIOCLEX)/ioctl(FIONCLEX), it will fail on
O_PATH file descriptors.
This is easy to reproduce:
>>> import os
>>> a = os.open("/", os.O_RDONLY)
>>> b = os.open("/", os.O_PATH)
>>> os.set_inheritable(a, True)
>>> os.set_inheritable(b, True) # Should succeed!
Traceback (most recent call last):
File "", line 1, in
OSError: [Errno 9] Bad file descriptor
>>>
I believe this affects all versions of Python going back to version 3.4 (where
os.set_inheritable()/os.get_inheritable() were introduced).
# Possible fixes
I see two potential paths for fixing this:
1. Don't use ioctl(FIOCLEX) at all on Linux.
This is what Rust did. However, based on bpo-22258 I'm guessing there would be
opposition to implementing this strategy in Python, on the grounds that the
fcntl() route takes an extra syscall (which is fair).
2. On Linux, fall back on fcntl() if ioctl(FIOCLEX) fails with EBADF.
This could be a very simple patch to Python/fileutils.c. I've attached a basic
version of said patch (not sure if it matches standard coding conventions).
Downsides: This would add 2 extra syscalls for O_PATH file descriptors, and 1
extra syscall for actual cases of invalid file descriptors (i.e. EBADF).
However, I believe these are edge cases that shouldn't come up frequently.
--
files: set-inheritable-o-path.patch
keywords: patch
messages: 384016
nosy: cptpcrd
priority: normal
severity: normal
status: open
title: os.set_inheritable() fails for O_PATH file descriptors on Linux
type: behavior
versions: Python 3.10, Python 3.6, Python 3.7, Python 3.8, Python 3.9
Added file: https://bugs.python.org/file49706/set-inheritable-o-path.patch
___
Python tracker
<https://bugs.python.org/issue42780>
___
___
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
