gh-112334: Restore subprocess's use of `vfork()` & fix `extra_groups=[]` behavior (#112617)

Restore `subprocess`'s intended use of `vfork()` by default for performance on Linux;
also fixes the behavior of `extra_groups=[]` which was unintentionally broken in 3.12.0:

Fixed a performance regression in 3.12's :mod:`subprocess` on Linux where it
would no longer use the fast-path ``vfork()`` system call when it could have
due to a logic bug, instead falling back to the safe but slower ``fork()``.

Also fixed a security bug introduced in 3.12.0.  If a value of ``extra_groups=[]``
was passed to :mod:`subprocess.Popen` or related APIs, the underlying
``setgroups(0, NULL)`` system call to clear the groups list would not be made
in the child process prior to ``exec()``.

The security issue was identified via code inspection in the process of
fixing the first bug.  Thanks to @vain for the detailed report and
analysis in the initial bug on Github.

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
This commit is contained in:
Gregory P. Smith 2023-12-04 15:08:19 -08:00 committed by GitHub
parent c5fa8a54db
commit 9fe7655c6c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 38 additions and 23 deletions

View File

@ -2066,8 +2066,14 @@ class POSIXProcessTestCase(BaseTestCase):
def test_extra_groups(self):
gid = os.getegid()
group_list = [65534 if gid != 65534 else 65533]
self._test_extra_groups_impl(gid=gid, group_list=group_list)
@unittest.skipUnless(hasattr(os, 'setgroups'), 'no setgroups() on platform')
def test_extra_groups_empty_list(self):
self._test_extra_groups_impl(gid=os.getegid(), group_list=[])
def _test_extra_groups_impl(self, *, gid, group_list):
name_group = _get_test_grp_name()
perm_error = False
if grp is not None:
group_list.append(name_group)
@ -2077,11 +2083,8 @@ class POSIXProcessTestCase(BaseTestCase):
[sys.executable, "-c",
"import os, sys, json; json.dump(os.getgroups(), sys.stdout)"],
extra_groups=group_list)
except OSError as ex:
if ex.errno != errno.EPERM:
raise
perm_error = True
except PermissionError:
self.skipTest("setgroup() EPERM; this test may require root.")
else:
parent_groups = os.getgroups()
child_groups = json.loads(output)
@ -2092,12 +2095,15 @@ class POSIXProcessTestCase(BaseTestCase):
else:
desired_gids = group_list
if perm_error:
self.assertEqual(set(child_groups), set(parent_groups))
else:
self.assertEqual(set(desired_gids), set(child_groups))
# make sure we bomb on negative values
if grp is None:
with self.assertRaises(ValueError):
subprocess.check_call(ZERO_RETURN_CMD,
extra_groups=[name_group])
# No skip necessary, this test won't make it to a setgroup() call.
def test_extra_groups_invalid_gid_t_values(self):
with self.assertRaises(ValueError):
subprocess.check_call(ZERO_RETURN_CMD, extra_groups=[-1])
@ -2106,16 +2112,6 @@ class POSIXProcessTestCase(BaseTestCase):
cwd=os.curdir, env=os.environ,
extra_groups=[2**64])
if grp is None:
with self.assertRaises(ValueError):
subprocess.check_call(ZERO_RETURN_CMD,
extra_groups=[name_group])
@unittest.skipIf(hasattr(os, 'setgroups'), 'setgroups() available on platform')
def test_extra_groups_error(self):
with self.assertRaises(ValueError):
subprocess.check_call(ZERO_RETURN_CMD, extra_groups=[])
@unittest.skipIf(mswindows or not hasattr(os, 'umask'),
'POSIX umask() is not available.')
def test_umask(self):

View File

@ -0,0 +1,11 @@
Fixed a performance regression in 3.12's :mod:`subprocess` on Linux where it
would no longer use the fast-path ``vfork()`` system call when it could have
due to a logic bug, instead falling back to the safe but slower ``fork()``.
Also fixed a second 3.12.0 potential security bug. If a value of
``extra_groups=[]`` was passed to :mod:`subprocess.Popen` or related APIs,
the underlying ``setgroups(0, NULL)`` system call to clear the groups list
would not be made in the child process prior to ``exec()``.
This was identified via code inspection in the process of fixing the first
bug.

View File

@ -767,8 +767,10 @@ child_exec(char *const exec_array[],
#endif
#ifdef HAVE_SETGROUPS
if (extra_group_size > 0)
if (extra_group_size >= 0) {
assert((extra_group_size == 0) == (extra_groups == NULL));
POSIX_CALL(setgroups(extra_group_size, extra_groups));
}
#endif /* HAVE_SETGROUPS */
#ifdef HAVE_SETREGID
@ -1022,7 +1024,6 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_args,
pid_t pid = -1;
int need_to_reenable_gc = 0;
char *const *argv = NULL, *const *envp = NULL;
Py_ssize_t extra_group_size = 0;
int need_after_fork = 0;
int saved_errno = 0;
int *c_fds_to_keep = NULL;
@ -1103,6 +1104,13 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_args,
cwd = PyBytes_AsString(cwd_obj2);
}
// Special initial value meaning that subprocess API was called with
// extra_groups=None leading to _posixsubprocess.fork_exec(gids=None).
// We use this to differentiate between code desiring a setgroups(0, NULL)
// call vs no call at all. The fast vfork() code path could be used when
// there is no setgroups call.
Py_ssize_t extra_group_size = -2;
if (extra_groups_packed != Py_None) {
#ifdef HAVE_SETGROUPS
if (!PyList_Check(extra_groups_packed)) {