classification
Title: subprocess.Popen() leaks cwd in case of uid/gid overflow
Type: resource usage Stage: commit review
Components: Extension Modules Versions: Python 3.10, Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: izbyshev Nosy List: gregory.p.smith, izbyshev, miss-islington, patrick.mclean
Priority: normal Keywords: 3.9regression, patch

Created on 2020-10-25 12:30 by izbyshev, last changed 2020-11-01 05:36 by gregory.p.smith. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 22966 merged izbyshev, 2020-10-25 12:34
PR 22970 merged izbyshev, 2020-10-25 14:16
PR 22980 merged miss-islington, 2020-10-26 00:09
Messages (6)
msg379575 - (view) Author: Alexey Izbyshev (izbyshev) * (Python triager) Date: 2020-10-25 12:30
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 https://github.com/python/cpython/blob/492d513ccbebeec40a8ba85cbd894a027ca5b2b3/Modules/_posixsubprocess.c#L892

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.
msg379579 - (view) Author: Alexey Izbyshev (izbyshev) * (Python triager) Date: 2020-10-25 14:25
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.
msg379623 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2020-10-26 00:09
New changeset c0590c0033e86f98cdf5f2ca6898656f98ab4053 by Alexey Izbyshev in branch 'master':
bpo-42146: Fix memory leak in subprocess.Popen() in case of uid/gid overflow (GH-22966)
https://github.com/python/cpython/commit/c0590c0033e86f98cdf5f2ca6898656f98ab4053
msg379626 - (view) Author: miss-islington (miss-islington) Date: 2020-10-26 00:34
New changeset c12afa92b0db6f017e479b41f95d75bac7b59850 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)
https://github.com/python/cpython/commit/c12afa92b0db6f017e479b41f95d75bac7b59850
msg379646 - (view) Author: Alexey Izbyshev (izbyshev) * (Python triager) Date: 2020-10-26 07:28
Thanks for merging! I've rebased PR 22970.
msg380116 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2020-11-01 05:33
New changeset d3b4e068077dd26927ae7485bd0303e09d962c02 by Alexey Izbyshev in branch 'master':
bpo-42146: Unify cleanup in subprocess_fork_exec() (GH-22970)
https://github.com/python/cpython/commit/d3b4e068077dd26927ae7485bd0303e09d962c02
History
Date User Action Args
2020-11-01 05:36:27gregory.p.smithsetstatus: open -> closed
resolution: fixed
stage: patch review -> commit review
2020-11-01 05:33:16gregory.p.smithsetmessages: + msg380116
2020-10-26 07:28:19izbyshevsetmessages: + msg379646
2020-10-26 00:34:36miss-islingtonsetmessages: + msg379626
2020-10-26 00:09:47miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request21897
2020-10-26 00:09:40gregory.p.smithsetmessages: + msg379623
2020-10-25 14:33:39izbyshevsettype: behavior -> resource usage
2020-10-25 14:28:32izbyshevsetkeywords: + patch
stage: patch review
2020-10-25 14:25:42izbyshevsetkeywords: - patch

messages: + msg379579
stage: patch review -> (no value)
2020-10-25 14:16:16izbyshevsetpull_requests: + pull_request21885
2020-10-25 12:34:55izbyshevsetkeywords: + patch
stage: patch review
pull_requests: + pull_request21882
2020-10-25 12:30:55izbyshevcreate