diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index 1e619a51e216ac..ce581abdd1dced 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -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 diff --git a/Lib/multiprocessing/util.py b/Lib/multiprocessing/util.py index 6ad4632fccb9e4..6ee0d33e88a060 100644 --- a/Lib/multiprocessing/util.py +++ b/Lib/multiprocessing/util.py @@ -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) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 968cfc14ddf910..9099822d0b5b19 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -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 @@ def __init__(self, args, bufsize=-1, executable=None, 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 @@ def __init__(self, args, bufsize=-1, executable=None, 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 @@ def __init__(self, args, bufsize=-1, executable=None, 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 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, 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 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, 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 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, 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 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, 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: diff --git a/Lib/test/test_asyncio/test_subprocess.py b/Lib/test/test_asyncio/test_subprocess.py index 14fa6dd76f9ca8..09a5c390b36299 100644 --- a/Lib/test/test_asyncio/test_subprocess.py +++ b/Lib/test/test_asyncio/test_subprocess.py @@ -15,6 +15,9 @@ 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)'] diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py index ab4caefd35fbf6..6d75895589328c 100644 --- a/Lib/test/test_capi.py +++ b/Lib/test/test_capi.py @@ -140,7 +140,7 @@ class Z(object): 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 @@ def __len__(self): 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 @@ def __len__(self): # 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") diff --git a/Lib/test/test_distutils.py b/Lib/test/test_distutils.py index d82d2b6423433e..28320fb5c0bfd1 100644 --- a/Lib/test/test_distutils.py +++ b/Lib/test/test_distutils.py @@ -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() diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 0764120952ebd9..f6854922a5b878 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1905,14 +1905,32 @@ def test_start_new_session(self): 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 @@ def test_fork_exec(self): 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 @@ def __int__(self): 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 index 00000000000000..b18d6a432427f5 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2020-12-24-19-11-53.bpo-38435.rEHTAR.rst @@ -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. diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 21c2bd13a3e27b..9132f13e8166a7 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -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\