subprocess.Popen() leaks cwd in case of uid/gid overflow

[izbyshev]

BPO	42146
Nosy	@gpshead, @izbyshev, @miss-islington, @patrick-mclean
PRs	bpo-42146: Fix memory leak in subprocess.Popen() in case of uid/gid overflow #22966 bpo-42146: Unify cleanup in subprocess_fork_exec() #22970 [3.9] bpo-42146: Fix memory leak in subprocess.Popen() in case of uid/gid overflow (GH-22966) #22980

^{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/izbyshev'
closed_at = <Date 2020-11-01.05:36:27.395>
created_at = <Date 2020-10-25.12:30:55.592>
labels = ['extension-modules', '3.9', '3.10', 'performance']
title = 'subprocess.Popen() leaks cwd in case of uid/gid overflow'
updated_at = <Date 2020-11-01.05:36:27.394>
user = 'https://github.com/izbyshev'

bugs.python.org fields:

activity = <Date 2020-11-01.05:36:27.394>
actor = 'gregory.p.smith'
assignee = 'izbyshev'
closed = True
closed_date = <Date 2020-11-01.05:36:27.395>
closer = 'gregory.p.smith'
components = ['Extension Modules']
creation = <Date 2020-10-25.12:30:55.592>
creator = 'izbyshev'
dependencies = []
files = []
hgrepos = []
issue_num = 42146
keywords = ['patch', '3.9regression']
message_count = 6.0
messages = ['379575', '379579', '379623', '379626', '379646', '380116']
nosy_count = 4.0
nosy_names = ['gregory.p.smith', 'izbyshev', 'miss-islington', 'patrick.mclean']
pr_nums = ['22966', '22970', '22980']
priority = 'normal'
resolution = 'fixed'
stage = 'commit review'
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue42146'
versions = ['Python 3.9', 'Python 3.10']

[izbyshev]

The following test demonstrates the leak:

import subprocess

cwd = 'x' * 10**6
for __ in range(100):
    try:
        subprocess.call(['/xxx'], cwd=cwd, user=2**64)
    except OverflowError:
        pass

from resource import *
print(getrusage(RUSAGE_SELF).ru_maxrss)

The leak was introduced by bpo-36046. Previously, `cleanup:` label was not reachable after `cwd_obj2` was initialized at

cpython/Modules/_posixsubprocess.c

Line 892 in 492d513

if (PyUnicode_FSConverter(cwd_obj, &cwd_obj2) == 0)

I'll submit a PR with a simple fix suitable for backporting to 3.9.

Also, I think it might make sense to unify the two almost-identical cleanup paths we have now. I'll follow up with another PR.

[izbyshev]

I've submitted both PRs.

Regarding PR 22970:

• I made it a draft since we'd probably want to fix the leak first, but then it will have to be rebased.

• It fixes a bug with _enable_gc(): if it failed after fork(), we'd raise OSError instead. Additionally, if fork() succeeded(), the errno inside OSError would be zero, and we'd leak the child process.

[gpshead]

New changeset c0590c0 by Alexey Izbyshev in branch 'master':
bpo-42146: Fix memory leak in subprocess.Popen() in case of uid/gid overflow (GH-22966)
c0590c0

[miss-islington]

New changeset c12afa9 by Miss Skeleton (bot) in branch '3.9':
[3.9] bpo-42146: Fix memory leak in subprocess.Popen() in case of uid/gid overflow (GH-22966) (GH-22980)
c12afa9

[izbyshev]

Thanks for merging! I've rebased PR 22970.

[gpshead]

New changeset d3b4e06 by Alexey Izbyshev in branch 'master':
bpo-42146: Unify cleanup in subprocess_fork_exec() (GH-22970)
d3b4e06