]> git.ipfire.org Git - thirdparty/Python/cpython.git/commitdiff
gh-82616: Add process_group support to subprocess.Popen (#23930)
authorGregory P. Smith <greg@krypto.org>
Thu, 5 May 2022 23:22:32 +0000 (16:22 -0700)
committerGitHub <noreply@github.com>
Thu, 5 May 2022 23:22:32 +0000 (16:22 -0700)
One more thing that can help prevent people from using `preexec_fn`.

Also adds conditional skips to two tests exposing ASAN flakiness on the Ubuntu 20.04 Address Sanitizer Github CI system. When that build is run on more modern systems the "problem" does not show up. It seems ASAN implementation related.

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Doc/library/subprocess.rst
Lib/multiprocessing/util.py
Lib/subprocess.py
Lib/test/test_asyncio/test_subprocess.py
Lib/test/test_capi.py
Lib/test/test_distutils.py
Lib/test/test_subprocess.py
Misc/NEWS.d/next/Library/2020-12-24-19-11-53.bpo-38435.rEHTAR.rst [new file with mode: 0644]
Modules/_posixsubprocess.c

index 1e619a51e216ac812cca48bdf9520d06a6cef129..ce581abdd1dcedcbcb6f7dd0c4cba0c8584f5047 100644 (file)
@@ -344,7 +344,8 @@ functions.
                  startupinfo=None, creationflags=0, restore_signals=True, \
                  start_new_session=False, pass_fds=(), *, group=None, \
                  extra_groups=None, user=None, umask=-1, \
-                 encoding=None, errors=None, text=None, pipesize=-1)
+                 encoding=None, errors=None, text=None, pipesize=-1, \
+                 process_group=None)
 
    Execute a child program in a new process.  On POSIX, the class uses
    :meth:`os.execvpe`-like behavior to execute the child program.  On Windows,
@@ -500,18 +501,16 @@ functions.
 
    .. warning::
 
-      The *preexec_fn* parameter is not safe to use in the presence of threads
+      The *preexec_fn* parameter is NOT SAFE to use in the presence of threads
       in your application.  The child process could deadlock before exec is
       called.
-      If you must use it, keep it trivial!  Minimize the number of libraries
-      you call into.
 
    .. note::
 
       If you need to modify the environment for the child use the *env*
       parameter rather than doing it in a *preexec_fn*.
-      The *start_new_session* parameter can take the place of a previously
-      common use of *preexec_fn* to call os.setsid() in the child.
+      The *start_new_session* and *process_group* parameters should take the place of
+      code using *preexec_fn* to call :func:`os.setsid` or :func:`os.setpgid` in the child.
 
    .. versionchanged:: 3.8
 
@@ -568,12 +567,20 @@ functions.
    .. versionchanged:: 3.2
       *restore_signals* was added.
 
-   If *start_new_session* is true the setsid() system call will be made in the
-   child process prior to the execution of the subprocess.  (POSIX only)
+   If *start_new_session* is true the ``setsid()`` system call will be made in the
+   child process prior to the execution of the subprocess.
 
+   .. availability:: POSIX
    .. versionchanged:: 3.2
       *start_new_session* was added.
 
+   If *process_group* is a non-negative integer, the ``setpgid(0, value)`` system call will
+   be made in the child process prior to the execution of the subprocess.
+
+   .. availability:: POSIX
+   .. versionchanged:: 3.11
+      *process_group* was added.
+
    If *group* is not ``None``, the setregid() system call will be made in the
    child process prior to the execution of the subprocess. If the provided
    value is a string, it will be looked up via :func:`grp.getgrnam()` and
index 6ad4632fccb9e49802732eeed1eb86c50804e7d5..6ee0d33e88a060a68c97717aee4ed2b6792b8e3f 100644 (file)
@@ -453,7 +453,7 @@ def spawnv_passfds(path, args, passfds):
         return _posixsubprocess.fork_exec(
             args, [path], True, passfds, None, None,
             -1, -1, -1, -1, -1, -1, errpipe_read, errpipe_write,
-            False, False, None, None, None, -1, None,
+            False, False, -1, None, None, None, -1, None,
             subprocess._USE_VFORK)
     finally:
         os.close(errpipe_read)
index 968cfc14ddf910c39a10a4170a49a9c67ad2fd66..9099822d0b5b192fbb3326786b3dc049c52e376c 100644 (file)
@@ -769,6 +769,8 @@ class Popen:
 
       start_new_session (POSIX only)
 
+      process_group (POSIX only)
+
       group (POSIX only)
 
       extra_groups (POSIX only)
@@ -794,7 +796,8 @@ class Popen:
                  startupinfo=None, creationflags=0,
                  restore_signals=True, start_new_session=False,
                  pass_fds=(), *, user=None, group=None, extra_groups=None,
-                 encoding=None, errors=None, text=None, umask=-1, pipesize=-1):
+                 encoding=None, errors=None, text=None, umask=-1, pipesize=-1,
+                 process_group=None):
         """Create new Popen instance."""
         _cleanup()
         # Held while anything is calling waitpid before returncode has been
@@ -900,6 +903,9 @@ class Popen:
             else:
                 line_buffering = False
 
+        if process_group is None:
+            process_group = -1  # The internal APIs are int-only
+
         gid = None
         if group is not None:
             if not hasattr(os, 'setregid'):
@@ -1003,7 +1009,7 @@ class Popen:
                                 errread, errwrite,
                                 restore_signals,
                                 gid, gids, uid, umask,
-                                start_new_session)
+                                start_new_session, process_group)
         except:
             # Cleanup if the child failed starting.
             for f in filter(None, (self.stdin, self.stdout, self.stderr)):
@@ -1387,7 +1393,7 @@ class Popen:
                            unused_restore_signals,
                            unused_gid, unused_gids, unused_uid,
                            unused_umask,
-                           unused_start_new_session):
+                           unused_start_new_session, unused_process_group):
             """Execute program (MS Windows version)"""
 
             assert not pass_fds, "pass_fds not supported on Windows."
@@ -1719,7 +1725,7 @@ class Popen:
                            errread, errwrite,
                            restore_signals,
                            gid, gids, uid, umask,
-                           start_new_session):
+                           start_new_session, process_group):
             """Execute program (POSIX version)"""
 
             if isinstance(args, (str, bytes)):
@@ -1755,6 +1761,7 @@ class Popen:
                     and (c2pwrite == -1 or c2pwrite > 2)
                     and (errwrite == -1 or errwrite > 2)
                     and not start_new_session
+                    and process_group == -1
                     and gid is None
                     and gids is None
                     and uid is None
@@ -1812,7 +1819,7 @@ class Popen:
                             errread, errwrite,
                             errpipe_read, errpipe_write,
                             restore_signals, start_new_session,
-                            gid, gids, uid, umask,
+                            process_group, gid, gids, uid, umask,
                             preexec_fn, _USE_VFORK)
                     self._child_created = True
                 finally:
index 14fa6dd76f9ca867d7ae2149d8da81923215f4e3..09a5c390b36299ddb939b01673f497ab4fdd83f8 100644 (file)
@@ -15,6 +15,9 @@ from test.support import os_helper
 if sys.platform != 'win32':
     from asyncio import unix_events
 
+if support.check_sanitizer(address=True):
+    raise unittest.SkipTest("Exposes ASAN flakiness in GitHub CI")
+
 # Program blocking
 PROGRAM_BLOCKED = [sys.executable, '-c', 'import time; time.sleep(3600)']
 
index ab4caefd35fbf637579cecc9e104835ba1d98cb3..6d75895589328c4210873aceace138f96c72e6da 100644 (file)
@@ -140,7 +140,7 @@ class CAPITest(unittest.TestCase):
             def __len__(self):
                 return 1
         self.assertRaises(TypeError, _posixsubprocess.fork_exec,
-                          1,Z(),3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22)
+                          1,Z(),3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23)
         # Issue #15736: overflow in _PySequence_BytesToCharpArray()
         class Z(object):
             def __len__(self):
@@ -148,7 +148,7 @@ class CAPITest(unittest.TestCase):
             def __getitem__(self, i):
                 return b'x'
         self.assertRaises(MemoryError, _posixsubprocess.fork_exec,
-                          1,Z(),3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22)
+                          1,Z(),3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23)
 
     @unittest.skipUnless(_posixsubprocess, '_posixsubprocess required for this test.')
     def test_subprocess_fork_exec(self):
@@ -158,7 +158,7 @@ class CAPITest(unittest.TestCase):
 
         # Issue #15738: crash in subprocess_fork_exec()
         self.assertRaises(TypeError, _posixsubprocess.fork_exec,
-                          Z(),[b'1'],3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22)
+                          Z(),[b'1'],3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23)
 
     @unittest.skipIf(MISSING_C_DOCSTRINGS,
                      "Signature information for builtins requires docstrings")
index d82d2b6423433eeb69e05469b0d4b18fbe423e92..28320fb5c0bfd129c8d6e8e27fb841e2e11a0a74 100644 (file)
@@ -23,6 +23,8 @@ def load_tests(*_):
 def tearDownModule():
     support.reap_children()
 
+if support.check_sanitizer(address=True):
+    raise unittest.SkipTest("Exposes ASAN flakiness in GitHub CI")
 
 if __name__ == "__main__":
     unittest.main()
index 0764120952ebd9c7a4a7ffbf069cf6dd543e949b..f6854922a5b87825469a8dc00eb6f0855c7baf04 100644 (file)
@@ -1905,14 +1905,32 @@ class POSIXProcessTestCase(BaseTestCase):
             output = subprocess.check_output(
                     [sys.executable, "-c", "import os; print(os.getsid(0))"],
                     start_new_session=True)
-        except OSError as e:
+        except PermissionError as e:
             if e.errno != errno.EPERM:
-                raise
+                raise  # EACCES?
         else:
             parent_sid = os.getsid(0)
             child_sid = int(output)
             self.assertNotEqual(parent_sid, child_sid)
 
+    @unittest.skipUnless(hasattr(os, 'setpgid') and hasattr(os, 'getpgid'),
+                         'no setpgid or getpgid on platform')
+    def test_process_group_0(self):
+        # For code coverage of calling setpgid().  We don't care if we get an
+        # EPERM error from it depending on the test execution environment, that
+        # still indicates that it was called.
+        try:
+            output = subprocess.check_output(
+                    [sys.executable, "-c", "import os; print(os.getpgid(0))"],
+                    process_group=0)
+        except PermissionError as e:
+            if e.errno != errno.EPERM:
+                raise  # EACCES?
+        else:
+            parent_pgid = os.getpgid(0)
+            child_pgid = int(output)
+            self.assertNotEqual(parent_pgid, child_pgid)
+
     @unittest.skipUnless(hasattr(os, 'setreuid'), 'no setreuid on platform')
     def test_user(self):
         # For code coverage of the user parameter.  We don't care if we get an
@@ -3134,7 +3152,7 @@ class POSIXProcessTestCase(BaseTestCase):
                         True, (), cwd, env_list,
                         -1, -1, -1, -1,
                         1, 2, 3, 4,
-                        True, True,
+                        True, True, 0,
                         False, [], 0, -1,
                         func, False)
                 # Attempt to prevent
@@ -3183,7 +3201,7 @@ class POSIXProcessTestCase(BaseTestCase):
                         True, fds_to_keep, None, [b"env"],
                         -1, -1, -1, -1,
                         1, 2, 3, 4,
-                        True, True,
+                        True, True, 0,
                         None, None, None, -1,
                         None, "no vfork")
                 self.assertIn('fds_to_keep', str(c.exception))
diff --git a/Misc/NEWS.d/next/Library/2020-12-24-19-11-53.bpo-38435.rEHTAR.rst b/Misc/NEWS.d/next/Library/2020-12-24-19-11-53.bpo-38435.rEHTAR.rst
new file mode 100644 (file)
index 0000000..b18d6a4
--- /dev/null
@@ -0,0 +1,2 @@
+Add a ``process_group`` parameter to :class:`subprocess.Popen` to help move
+more things off of the unsafe ``preexec_fn`` parameter.
index 21c2bd13a3e27b1541090e56e31839bda27f938c..9132f13e8166a72fc725743db866d6f0e57ff6b7 100644 (file)
@@ -517,7 +517,7 @@ child_exec(char *const exec_array[],
            int errread, int errwrite,
            int errpipe_read, int errpipe_write,
            int close_fds, int restore_signals,
-           int call_setsid,
+           int call_setsid, pid_t pgid_to_set,
            int call_setgid, gid_t gid,
            int call_setgroups, size_t groups_size, const gid_t *groups,
            int call_setuid, uid_t uid, int child_umask,
@@ -611,6 +611,11 @@ child_exec(char *const exec_array[],
         POSIX_CALL(setsid());
 #endif
 
+#ifdef HAVE_SETPGID
+    if (pgid_to_set >= 0)
+        POSIX_CALL(setpgid(0, pgid_to_set));
+#endif
+
 #ifdef HAVE_SETGROUPS
     if (call_setgroups)
         POSIX_CALL(setgroups(groups_size, groups));
@@ -716,7 +721,7 @@ do_fork_exec(char *const exec_array[],
              int errread, int errwrite,
              int errpipe_read, int errpipe_write,
              int close_fds, int restore_signals,
-             int call_setsid,
+             int call_setsid, pid_t pgid_to_set,
              int call_setgid, gid_t gid,
              int call_setgroups, size_t groups_size, const gid_t *groups,
              int call_setuid, uid_t uid, int child_umask,
@@ -769,7 +774,7 @@ do_fork_exec(char *const exec_array[],
     child_exec(exec_array, argv, envp, cwd,
                p2cread, p2cwrite, c2pread, c2pwrite,
                errread, errwrite, errpipe_read, errpipe_write,
-               close_fds, restore_signals, call_setsid,
+               close_fds, restore_signals, call_setsid, pgid_to_set,
                call_setgid, gid, call_setgroups, groups_size, groups,
                call_setuid, uid, child_umask, child_sigmask,
                py_fds_to_keep, preexec_fn, preexec_fn_args_tuple);
@@ -791,6 +796,7 @@ subprocess_fork_exec(PyObject *module, PyObject *args)
     int p2cread, p2cwrite, c2pread, c2pwrite, errread, errwrite;
     int errpipe_read, errpipe_write, close_fds, restore_signals;
     int call_setsid;
+    pid_t pgid_to_set = -1;
     int call_setgid = 0, call_setgroups = 0, call_setuid = 0;
     uid_t uid;
     gid_t gid, *groups = NULL;
@@ -806,13 +812,13 @@ subprocess_fork_exec(PyObject *module, PyObject *args)
     int allow_vfork;
 
     if (!PyArg_ParseTuple(
-            args, "OOpO!OOiiiiiiiiiiOOOiOp:fork_exec",
+            args, "OOpO!OOiiiiiiiiii" _Py_PARSE_PID "OOOiOp:fork_exec",
             &process_args, &executable_list,
             &close_fds, &PyTuple_Type, &py_fds_to_keep,
             &cwd_obj, &env_list,
             &p2cread, &p2cwrite, &c2pread, &c2pwrite,
             &errread, &errwrite, &errpipe_read, &errpipe_write,
-            &restore_signals, &call_setsid,
+            &restore_signals, &call_setsid, &pgid_to_set,
             &gid_object, &groups_list, &uid_object, &child_umask,
             &preexec_fn, &allow_vfork))
         return NULL;
@@ -1016,7 +1022,7 @@ subprocess_fork_exec(PyObject *module, PyObject *args)
     pid = do_fork_exec(exec_array, argv, envp, cwd,
                        p2cread, p2cwrite, c2pread, c2pwrite,
                        errread, errwrite, errpipe_read, errpipe_write,
-                       close_fds, restore_signals, call_setsid,
+                       close_fds, restore_signals, call_setsid, pgid_to_set,
                        call_setgid, gid, call_setgroups, num_groups, groups,
                        call_setuid, uid, child_umask, old_sigmask,
                        py_fds_to_keep, preexec_fn, preexec_fn_args_tuple);
@@ -1081,7 +1087,7 @@ PyDoc_STRVAR(subprocess_fork_exec_doc,
 "fork_exec(args, executable_list, close_fds, pass_fds, cwd, env,\n\
           p2cread, p2cwrite, c2pread, c2pwrite,\n\
           errread, errwrite, errpipe_read, errpipe_write,\n\
-          restore_signals, call_setsid,\n\
+          restore_signals, call_setsid, pgid_to_set,\n\
           gid, groups_list, uid,\n\
           preexec_fn)\n\
 \n\