[issue43308] subprocess.Popen leaks file descriptors opened for DEVNULL or PIPE stdin/stdout/stderr arguments

[Alexey Izbyshev]

Change by Alexey Izbyshev :


--
nosy: +gregory.p.smith

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue43308] subprocess.Popen leaks file descriptors opened for DEVNULL or PIPE stdin/stdout/stderr arguments

[cptpcrd]

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: subprocess.Popen leaks file descriptors opened for DEVNULL or PIPE 
stdin/stdout/stderr arguments
type: resource usage
versions: Python 3.10, Python 3.8, Python 3.9
Added file: 
https://bugs.python.org/file49830/subprocess-validation-fd-leak.patch