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

gpshead · serhiy-storchaka · web-flow · commit 9fe7655c6ce0 · 2023-12-04T15:08:19.000-08:00
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>
diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py
@@ -2066,8 +2066,14 @@ def test_group_error(self):
     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 @@ def test_extra_groups(self):
                     [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 @@ def test_extra_groups(self):
             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))
+            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 @@ def test_extra_groups(self):
                                   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):
diff --git a/Misc/NEWS.d/next/Library/2023-12-01-21-05-46.gh-issue-112334.DmNXKh.rst b/Misc/NEWS.d/next/Library/2023-12-01-21-05-46.gh-issue-112334.DmNXKh.rst
@@ -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.
diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c
@@ -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)) {
