[Python-checkins] bpo-42146: Fix memory leak in subprocess.Popen() in case of uid/gid overflow (GH-22966)

Sun Oct 25 20:09:41 EDT 2020

https://github.com/python/cpython/commit/c0590c0033e86f98cdf5f2ca6898656f98ab4053
commit: c0590c0033e86f98cdf5f2ca6898656f98ab4053
branch: master
author: Alexey Izbyshev <izbyshev at ispras.ru>
committer: gpshead <greg at krypto.org>
date: 2020年10月25日T17:09:32-07:00
summary:
bpo-42146: Fix memory leak in subprocess.Popen() in case of uid/gid overflow (GH-22966)
Fix memory leak in subprocess.Popen() in case of uid/gid overflow
Also add a test that would catch this leak with `--huntrleaks`.
Alas, the test for `extra_groups` also exposes an inconsistency
in our error reporting: we use a custom ValueError for `extra_groups`,
but propagate OverflowError for `user` and `group`.
files:
A Misc/NEWS.d/next/Library/2020-10-25-19-25-02.bpo-42146.6A8uvS.rst
M Lib/test/test_subprocess.py
M Modules/_posixsubprocess.c

diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py
index 9fc4434649dbc..e25474abed4b7 100644
--- a/Lib/test/test_subprocess.py
+++ b/Lib/test/test_subprocess.py
@@ -1895,6 +1895,10 @@ def test_user(self):
 with self.assertRaises(ValueError):
 subprocess.check_call(ZERO_RETURN_CMD, user=-1)
 
+ with self.assertRaises(OverflowError):
+ subprocess.check_call(ZERO_RETURN_CMD,
+ cwd=os.curdir, env=os.environ, user=2**64)
+
 if pwd is None and name_uid is not None:
 with self.assertRaises(ValueError):
 subprocess.check_call(ZERO_RETURN_CMD, user=name_uid)
@@ -1938,6 +1942,10 @@ def test_group(self):
 with self.assertRaises(ValueError):
 subprocess.check_call(ZERO_RETURN_CMD, group=-1)
 
+ with self.assertRaises(OverflowError):
+ subprocess.check_call(ZERO_RETURN_CMD,
+ cwd=os.curdir, env=os.environ, group=2**64)
+
 if grp is None:
 with self.assertRaises(ValueError):
 subprocess.check_call(ZERO_RETURN_CMD, group=name_group)
@@ -1986,6 +1994,11 @@ def test_extra_groups(self):
 with self.assertRaises(ValueError):
 subprocess.check_call(ZERO_RETURN_CMD, extra_groups=[-1])
 
+ with self.assertRaises(ValueError):
+ subprocess.check_call(ZERO_RETURN_CMD,
+ cwd=os.curdir, env=os.environ,
+ extra_groups=[2**64])
+
 if grp is None:
 with self.assertRaises(ValueError):
 subprocess.check_call(ZERO_RETURN_CMD,
diff --git a/Misc/NEWS.d/next/Library/2020-10-25-19-25-02.bpo-42146.6A8uvS.rst b/Misc/NEWS.d/next/Library/2020-10-25-19-25-02.bpo-42146.6A8uvS.rst
new file mode 100644
index 0000000000000..041809803db6a
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2020-10-25-19-25-02.bpo-42146.6A8uvS.rst
@@ -0,0 +1,2 @@
+Fix memory leak in :func:`subprocess.Popen` in case an uid (gid) specified in
+`user` (`group`, `extra_groups`) overflows `uid_t` (`gid_t`).
diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c
index 8baea314f4e40..5e5fbb2e79a7f 100644
--- a/Modules/_posixsubprocess.c
+++ b/Modules/_posixsubprocess.c
@@ -772,7 +772,7 @@ subprocess_fork_exec(PyObject* self, PyObject *args)
 uid_t uid;
 gid_t gid, *groups = NULL;
 int child_umask;
- PyObject *cwd_obj, *cwd_obj2;
+ PyObject *cwd_obj, *cwd_obj2 = NULL;
 const char *cwd;
 pid_t pid;
 int need_to_reenable_gc = 0;
@@ -894,7 +894,6 @@ subprocess_fork_exec(PyObject* self, PyObject *args)
 cwd = PyBytes_AsString(cwd_obj2);
 } else {
 cwd = NULL;
- cwd_obj2 = NULL;
 }
 
 if (groups_list != Py_None) {
@@ -1080,6 +1079,7 @@ subprocess_fork_exec(PyObject* self, PyObject *args)
 return PyLong_FromPid(pid);
 
 cleanup:
+ Py_XDECREF(cwd_obj2);
 if (envp)
 _Py_FreeCharPArray(envp);
 if (argv)
