Skip to content

Commit

Permalink
pythongh-91401: Add a failsafe way to disable vfork.
Browse files Browse the repository at this point in the history
Just in case there is ever an issue with _posixsubprocess's use of
vfork() due to the complexity of using it properly and potential
directions that Linux platforms where it defaults to on could take, this
adds a failsafe so that users can disable its use entirely by setting
a global flag.

No known reason to disable it exists. But it'd be a shame to encounter
one and not be able to use CPython without patching and rebuilding it.

See the linked issue for some discussion on reasoning.
  • Loading branch information
gpshead committed Apr 13, 2022
1 parent 612e422 commit aba06a0
Show file tree
Hide file tree
Showing 6 changed files with 49 additions and 11 deletions.
26 changes: 26 additions & 0 deletions Doc/library/subprocess.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1529,3 +1529,29 @@ runtime):

:mod:`shlex`
Module which provides function to parse and escape command lines.


.. _disable_vfork:

Disabling potential use of ``vfork()``
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

On Linux, :mod:`subprocess` defaults to using the ``vfork()`` system call
internally when it is safe to do so rather than ``fork()``. This greatly
improves performance.

If you ever encounter a presumed highly-unusual situation where you need to
prevent ``vfork()`` from being used by Python, you can set the
:attr:`subprocess.disable_vfork_reason` attribute to a non-empty string.
Ideally one describing why and linking to a bug report explaining how to setup
an environment with code to reproduce the issue preventing it from working
properly. Without recording that information, nobody will understand when they
can unset it in your code in the future.

Setting this has no impact on use of ``posix_spawn()`` which could use
``vfork()`` within its libc implementation.

It is safe to set this attribute on older Python versions. Do not assume it
exists to be read until 3.11.

.. versionadded:: 3.11 ``disable_vfork_reason``
4 changes: 3 additions & 1 deletion Lib/multiprocessing/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -446,13 +446,15 @@ def _flush_std_streams():

def spawnv_passfds(path, args, passfds):
import _posixsubprocess
import subprocess
passfds = tuple(sorted(map(int, passfds)))
errpipe_read, errpipe_write = os.pipe()
try:
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, None, None, None, -1, None,
subprocess.disable_vfork_reason)
finally:
os.close(errpipe_read)
os.close(errpipe_write)
Expand Down
10 changes: 9 additions & 1 deletion Lib/subprocess.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,14 @@ def _fork_exec(*args, **kwargs):
import selectors


# This is purely a failsafe. If non-empty the _posixsubprocess code will never
# consider using vfork(). While any true value will trigger this, set it to a
# description of why it is being disabled. With a link to a bug report
# explaining how to setup an environment with code to reproduce the issue this
# is working around. Otherwise nobody will understand when they can unset it.
disable_vfork_reason = ""


# Exception classes used by this module.
class SubprocessError(Exception): pass

Expand Down Expand Up @@ -1792,7 +1800,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, disable_vfork_reason)
self._child_created = True
finally:
# be sure the FD is closed no matter what
Expand Down
6 changes: 3 additions & 3 deletions Lib/test/test_capi.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,15 +118,15 @@ 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):
return sys.maxsize
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):
Expand All @@ -136,7 +136,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")
Expand Down
7 changes: 4 additions & 3 deletions Lib/test/test_subprocess.py
Original file line number Diff line number Diff line change
Expand Up @@ -3107,7 +3107,7 @@ def test_fork_exec(self):
1, 2, 3, 4,
True, True,
False, [], 0, -1,
func)
func, "")
# Attempt to prevent
# "TypeError: fork_exec() takes exactly N arguments (M given)"
# from passing the test. More refactoring to have us start
Expand Down Expand Up @@ -3156,7 +3156,7 @@ def __int__(self):
1, 2, 3, 4,
True, True,
None, None, None, -1,
None)
None, "no vfork")
self.assertIn('fds_to_keep', str(c.exception))
finally:
if not gc_enabled:
Expand Down Expand Up @@ -3614,7 +3614,8 @@ def test_getoutput(self):

def test__all__(self):
"""Ensure that __all__ is populated properly."""
intentionally_excluded = {"list2cmdline", "Handle", "pwd", "grp", "fcntl"}
intentionally_excluded = {"list2cmdline", "Handle", "pwd", "grp",
"fcntl", "disable_vfork_reason"}
exported = set(subprocess.__all__)
possible_exports = set()
import types
Expand Down
7 changes: 4 additions & 3 deletions Modules/_posixsubprocess.c
Original file line number Diff line number Diff line change
Expand Up @@ -751,17 +751,18 @@ subprocess_fork_exec(PyObject *module, PyObject *args)
Py_ssize_t arg_num, num_groups = 0;
int need_after_fork = 0;
int saved_errno = 0;
int disable_vfork;

if (!PyArg_ParseTuple(
args, "OOpO!OOiiiiiiiiiiOOOiO:fork_exec",
args, "OOpO!OOiiiiiiiiiiOOOiOp: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,
&gid_object, &groups_list, &uid_object, &child_umask,
&preexec_fn))
&preexec_fn, &disable_vfork))
return NULL;

if ((preexec_fn != Py_None) &&
Expand Down Expand Up @@ -940,7 +941,7 @@ subprocess_fork_exec(PyObject *module, PyObject *args)
#ifdef VFORK_USABLE
/* Use vfork() only if it's safe. See the comment above child_exec(). */
sigset_t old_sigs;
if (preexec_fn == Py_None &&
if (preexec_fn == Py_None && !disable_vfork &&
!call_setuid && !call_setgid && !call_setgroups) {
/* Block all signals to ensure that no signal handlers are run in the
* child process while it shares memory with us. Note that signals
Expand Down

0 comments on commit aba06a0

Please sign in to comment.