From 1454088b258b3b2078cedfef10b394d4ce307f61 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 24 Dec 2020 19:12:06 -0800 Subject: [PATCH 01/15] Add setpgid support to subprocess.POpen Adds a setpgid parameter to subprocess APIs to help us deprecate uses of preexec_fn. --- Doc/library/subprocess.rst | 21 ++++++++++++------ Lib/subprocess.py | 14 +++++++----- Lib/test/test_subprocess.py | 22 +++++++++++++++++-- .../2020-12-24-19-11-53.bpo-38435.rEHTAR.rst | 2 ++ Modules/_posixsubprocess.c | 16 +++++++++----- 5 files changed, 56 insertions(+), 19 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2020-12-24-19-11-53.bpo-38435.rEHTAR.rst diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index 4ac3f80a319f80..196642129ac7a0 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -341,7 +341,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, + setpgid=-1) 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, @@ -496,18 +497,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 *setpgid* parameters should take the place of + code using *preexec_fn* to call os.setsid() or os.setpgid() in the child. .. versionchanged:: 3.8 @@ -565,11 +564,19 @@ functions. *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) + child process prior to the execution of the subprocess. + .. availability:: POSIX .. versionchanged:: 3.2 *start_new_session* was added. + If *setpgid* is a non-negative value, the setpgid(0, value) system call will + be made in the child process prior to the execution of the subprocess. + + .. availability:: POSIX + .. versionchanged:: 3.10 + *setpgid* 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/subprocess.py b/Lib/subprocess.py index e259dc3a8e538a..7e18b2eae36e33 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -736,6 +736,8 @@ class Popen(object): start_new_session (POSIX only) + setpgid (POSIX only) + group (POSIX only) extra_groups (POSIX only) @@ -761,7 +763,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, + setpgid=-1): """Create new Popen instance.""" _cleanup() # Held while anything is calling waitpid before returncode has been @@ -963,7 +966,7 @@ def __init__(self, args, bufsize=-1, executable=None, errread, errwrite, restore_signals, gid, gids, uid, umask, - start_new_session) + start_new_session, setpgid) except: # Cleanup if the child failed starting. for f in filter(None, (self.stdin, self.stdout, self.stderr)): @@ -1347,7 +1350,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_setpgid): """Execute program (MS Windows version)""" assert not pass_fds, "pass_fds not supported on Windows." @@ -1681,7 +1684,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, setpgid): """Execute program (POSIX version)""" if isinstance(args, (str, bytes)): @@ -1717,6 +1720,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 setpgid == -1 and gid is None and gids is None and uid is None @@ -1775,7 +1779,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, errpipe_read, errpipe_write, restore_signals, start_new_session, gid, gids, uid, umask, - preexec_fn) + preexec_fn, setpgid) self._child_created = True finally: # be sure the FD is closed no matter what diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 2a4c47530e6a1b..0b91aa25a57c71 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1843,14 +1843,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_setpgid_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))"], + setpgid=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 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..b7fb423c9c6471 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2020-12-24-19-11-53.bpo-38435.rEHTAR.rst @@ -0,0 +1,2 @@ +Add a ``setpgid`` parameter to *subprocess.POpen* to help ease the +deprecation of the unsafe ``preexec_fn`` parameter. diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 46c41d3c20a146..a8ae3ecc44e929 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -500,7 +500,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, @@ -594,6 +594,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)); @@ -768,6 +773,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; int call_setgid = 0, call_setgroups = 0, call_setuid = 0; uid_t uid; gid_t gid, *groups = NULL; @@ -783,13 +789,13 @@ subprocess_fork_exec(PyObject *module, PyObject *args) _posixsubprocessstate *state = get_posixsubprocess_state(module); if (!PyArg_ParseTuple( - args, "OOpO!OOiiiiiiiiiiOOOiO:fork_exec", + args, "OOpO!OOiiiiiiiiii" _Py_PARSE_PID "OOOiO: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)) return NULL; @@ -1020,7 +1026,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); @@ -1083,7 +1089,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\ From 6aad6e350e01a797a7989c81e5fd21b0c10c608b Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 24 Dec 2020 20:40:50 -0800 Subject: [PATCH 02/15] Fix compilation error. --- Modules/_posixsubprocess.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index a8ae3ecc44e929..f85426e0931f70 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -704,7 +704,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, @@ -751,7 +751,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); From 5f5d8001ef1be456e7701815f36e5fed1613de57 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 24 Dec 2020 20:50:19 -0800 Subject: [PATCH 03/15] Fix the fork_exec() calls. tests pass. --- Lib/subprocess.py | 4 ++-- Lib/test/test_subprocess.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 7e18b2eae36e33..a1b7e056d585ad 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -1778,8 +1778,8 @@ 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, - preexec_fn, setpgid) + setpgid, gid, gids, uid, umask, + preexec_fn) self._child_created = True finally: # be sure the FD is closed no matter what diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 0b91aa25a57c71..8f6908cf743ca4 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -3105,7 +3105,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) # Attempt to prevent @@ -3154,7 +3154,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) self.assertIn('fds_to_keep', str(c.exception)) From 010ae9a1e187fdc3ad87eb4c54e083798f3b54d1 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 24 Dec 2020 21:03:18 -0800 Subject: [PATCH 04/15] Fix multiprocessing's _posixsubprocess.fork_exec() call. --- Lib/multiprocessing/util.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/multiprocessing/util.py b/Lib/multiprocessing/util.py index 21f2a7ebe25002..49de43d97e9f57 100644 --- a/Lib/multiprocessing/util.py +++ b/Lib/multiprocessing/util.py @@ -452,7 +452,7 @@ def spawnv_passfds(path, args, passfds): return _posixsubprocess.fork_exec( args, [os.fsencode(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) finally: os.close(errpipe_read) os.close(errpipe_write) From 8a770ab6ff35977dc0f92be8fa5e166aaf91891e Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 24 Dec 2020 21:12:41 -0800 Subject: [PATCH 05/15] Fix test_capi. --- Lib/test/test_capi.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py index a4ebe4a0a1b5cb..3b3c5b9c528915 100644 --- a/Lib/test/test_capi.py +++ b/Lib/test/test_capi.py @@ -108,7 +108,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) + 1,Z(),3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22) # Issue #15736: overflow in _PySequence_BytesToCharpArray() class Z(object): def __len__(self): @@ -116,7 +116,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) + 1,Z(),3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22) @unittest.skipUnless(_posixsubprocess, '_posixsubprocess required for this test.') def test_subprocess_fork_exec(self): @@ -126,7 +126,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) + Z(),[b'1'],3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22) @unittest.skipIf(MISSING_C_DOCSTRINGS, "Signature information for builtins requires docstrings") From b58758bdb14624c087c81b51d90bfc5f6c658f81 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 5 May 2022 11:00:38 -0700 Subject: [PATCH 06/15] Add missing \ in Doc/library/subprocess.rst Co-authored-by: Zackery Spytz --- Doc/library/subprocess.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index 915114cde6139a..11c6dfca698fbe 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -344,7 +344,7 @@ 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, \ setpgid=-1) Execute a child program in a new process. On POSIX, the class uses From acba3947511fa875320f4f92e7307e4e04141583 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 5 May 2022 11:01:14 -0700 Subject: [PATCH 07/15] better ReST in Doc/library/subprocess.rst Co-authored-by: Serhiy Storchaka --- Doc/library/subprocess.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index 11c6dfca698fbe..ada0d6ed5f8055 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -510,7 +510,7 @@ functions. 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* and *setpgid* parameters should take the place of - code using *preexec_fn* to call os.setsid() or os.setpgid() in the child. + code using *preexec_fn* to call :func:`os.setsid` or :func:`os.setpgid` in the child. .. versionchanged:: 3.8 From cef3bbe997a8aaa110ddb6edc5ec459e68b46ad3 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 5 May 2022 11:01:33 -0700 Subject: [PATCH 08/15] improve formatting in Doc/library/subprocess.rst Co-authored-by: Serhiy Storchaka --- Doc/library/subprocess.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index ada0d6ed5f8055..022e9a4cef7bb5 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -567,7 +567,7 @@ functions. .. versionchanged:: 3.2 *restore_signals* was added. - If *start_new_session* is true the setsid() system call will be made in the + 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 From e594a01f8a2bedc5091956fc8561d3d0f420b40c Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 5 May 2022 11:01:47 -0700 Subject: [PATCH 09/15] improve formatting in Doc/library/subprocess.rst Co-authored-by: Serhiy Storchaka --- Doc/library/subprocess.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index 022e9a4cef7bb5..f6483f66137b86 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -574,7 +574,7 @@ functions. .. versionchanged:: 3.2 *start_new_session* was added. - If *setpgid* is a non-negative value, the setpgid(0, value) system call will + If *setpgid* is a non-negative value, the ``setpgid(0, value)`` system call will be made in the child process prior to the execution of the subprocess. .. availability:: POSIX From ab9f0a5bc313d56edacb8c9ceaae8a0d2cd00597 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 5 May 2022 11:02:37 -0700 Subject: [PATCH 10/15] better formatting --- .../next/Library/2020-12-24-19-11-53.bpo-38435.rEHTAR.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index b7fb423c9c6471..7ad67480f9de51 100644 --- 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 @@ -1,2 +1,2 @@ -Add a ``setpgid`` parameter to *subprocess.POpen* to help ease the +Add a ``setpgid`` parameter to :class:`subprocess.Popen` to help ease the deprecation of the unsafe ``preexec_fn`` parameter. From 3b3a5d0da4c3f4d71154870453d7d60dc54e4f7f Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Thu, 5 May 2022 18:12:49 +0000 Subject: [PATCH 11/15] versionadded 3.11 --- Doc/library/subprocess.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index f6483f66137b86..5a0484bd84e98d 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -578,7 +578,7 @@ functions. be made in the child process prior to the execution of the subprocess. .. availability:: POSIX - .. versionchanged:: 3.10 + .. versionchanged:: 3.11 *setpgid* was added. If *group* is not ``None``, the setregid() system call will be made in the From 20a1363b5e0f5394d0423cdb1d0eb40a1afe1f5e Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Thu, 5 May 2022 18:32:21 +0000 Subject: [PATCH 12/15] Rename from setpgid to process_group as suggested by Serhiy on the issue. --- Doc/library/subprocess.rst | 8 ++++---- Lib/subprocess.py | 17 ++++++++++------- Lib/test/test_subprocess.py | 4 ++-- .../2020-12-24-19-11-53.bpo-38435.rEHTAR.rst | 4 ++-- 4 files changed, 18 insertions(+), 15 deletions(-) diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index 5a0484bd84e98d..ce581abdd1dced 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -345,7 +345,7 @@ functions. start_new_session=False, pass_fds=(), *, group=None, \ extra_groups=None, user=None, umask=-1, \ encoding=None, errors=None, text=None, pipesize=-1, \ - setpgid=-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, @@ -509,7 +509,7 @@ functions. 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* and *setpgid* parameters should take the place of + 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 @@ -574,12 +574,12 @@ functions. .. versionchanged:: 3.2 *start_new_session* was added. - If *setpgid* is a non-negative value, the ``setpgid(0, value)`` system call will + 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 - *setpgid* was added. + *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 diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 6e78eddc065820..9099822d0b5b19 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -769,7 +769,7 @@ class Popen: start_new_session (POSIX only) - setpgid (POSIX only) + process_group (POSIX only) group (POSIX only) @@ -797,7 +797,7 @@ def __init__(self, args, bufsize=-1, executable=None, 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, - setpgid=-1): + process_group=None): """Create new Popen instance.""" _cleanup() # Held while anything is calling waitpid before returncode has been @@ -903,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'): @@ -1006,7 +1009,7 @@ def __init__(self, args, bufsize=-1, executable=None, errread, errwrite, restore_signals, gid, gids, uid, umask, - start_new_session, setpgid) + start_new_session, process_group) except: # Cleanup if the child failed starting. for f in filter(None, (self.stdin, self.stdout, self.stderr)): @@ -1390,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_setpgid): + unused_start_new_session, unused_process_group): """Execute program (MS Windows version)""" assert not pass_fds, "pass_fds not supported on Windows." @@ -1722,7 +1725,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, errread, errwrite, restore_signals, gid, gids, uid, umask, - start_new_session, setpgid): + start_new_session, process_group): """Execute program (POSIX version)""" if isinstance(args, (str, bytes)): @@ -1758,7 +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 setpgid == -1 + and process_group == -1 and gid is None and gids is None and uid is None @@ -1816,7 +1819,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, errread, errwrite, errpipe_read, errpipe_write, restore_signals, start_new_session, - setpgid, 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_subprocess.py b/Lib/test/test_subprocess.py index 6918ad3bbc4de3..f6854922a5b878 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1915,14 +1915,14 @@ def test_start_new_session(self): @unittest.skipUnless(hasattr(os, 'setpgid') and hasattr(os, 'getpgid'), 'no setpgid or getpgid on platform') - def test_setpgid_0(self): + 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))"], - setpgid=0) + process_group=0) except PermissionError as e: if e.errno != errno.EPERM: raise # EACCES? 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 index 7ad67480f9de51..b18d6a432427f5 100644 --- 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 @@ -1,2 +1,2 @@ -Add a ``setpgid`` parameter to :class:`subprocess.Popen` to help ease the -deprecation of the unsafe ``preexec_fn`` parameter. +Add a ``process_group`` parameter to :class:`subprocess.Popen` to help move +more things off of the unsafe ``preexec_fn`` parameter. From 3df5b302b521d636a5b2fde769dfa98274450986 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Thu, 5 May 2022 18:58:02 +0000 Subject: [PATCH 13/15] test_capi: update its internal call --- Lib/test/test_capi.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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") From 44cc2937765436df5bab672d3698376fffdfec23 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Thu, 5 May 2022 21:51:12 +0000 Subject: [PATCH 14/15] blindly attempting to appease an ASAN bug. --- Modules/_posixsubprocess.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index a6d1901c0ddceb..9132f13e8166a7 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -796,7 +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; + pid_t pgid_to_set = -1; int call_setgid = 0, call_setgroups = 0, call_setuid = 0; uid_t uid; gid_t gid, *groups = NULL; From bb9461ba67d00e71310b551f22d063a8a472a33f Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Thu, 5 May 2022 22:14:25 +0000 Subject: [PATCH 15/15] Disable the tests exposing ASAN flakiness. --- Lib/test/test_asyncio/test_subprocess.py | 3 +++ Lib/test/test_distutils.py | 2 ++ 2 files changed, 5 insertions(+) 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_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()