mirrors/cpython
1
0
Fork 0
mirror of https://github.com/python/cpython.git synced 2025-09-26 18:29:57 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 10

gh-82616: Add process_group support to subprocess.Popen (#23930)

Browse source 
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>
This commit is contained in:
Gregory P. Smith 2022-05-05 16:22:32 -07:00 • committed by GitHub
parent 49fda0cc51
commit f6dd14c653
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
9 changed files with 73 additions and 28 deletions
Download patch file Download diff file Expand all files Collapse all files

23
Doc/library/subprocess.rst
View 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

2
Lib/multiprocessing/util.py
View 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)

17
Lib/subprocess.py
View 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:

3
Lib/test/test_asyncio/test_subprocess.py
View 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)']

6
Lib/test/test_capi.py
View 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")

2
Lib/test/test_distutils.py
View 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()

26
Lib/test/test_subprocess.py
View 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))

2
Misc/NEWS.d/next/Library/2020-12-24-19-11-53.bpo-38435.rEHTAR.rst Normal file
View file

@ -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.

20
Modules/_posixsubprocess.c
View 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\