Skip to content

Commit

Permalink
gh-91401: Add a failsafe way to disable vfork. (#91490)
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.

Also documents the existing way to disable posix_spawn.
  • Loading branch information
gpshead committed Apr 25, 2022
1 parent eddd07f commit cd5726f
Show file tree
Hide file tree
Showing 7 changed files with 70 additions and 10 deletions.
36 changes: 36 additions & 0 deletions Doc/library/subprocess.rst
Expand Up @@ -1529,3 +1529,39 @@ runtime):

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


.. _disable_vfork:
.. _disable_posix_spawn:

Disabling use of ``vfork()`` or ``posix_spawn()``
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

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._USE_VFORK` attribute to a false value.

subprocess._USE_VFORK = False # See CPython issue gh-NNNNNN.

Setting this has no impact on use of ``posix_spawn()`` which could use
``vfork()`` internally within its libc implementation. There is a similar
:attr:`subprocess._USE_POSIX_SPAWN` attribute if you need to prevent use of
that.

subprocess._USE_POSIX_SPAWN = False # See CPython issue gh-NNNNNN.

It is safe to set these to false on any Python version. They will have no
effect on older versions when unsupported. Do not assume the attributes are
available to read. Despite their names, a true value does not indicate that the
corresponding function will be used, only that that it may be.

Please file issues any time you have to use these private knobs with a way to
reproduce the issue you were seeing. Link to that issue from a comment in your
code.

.. versionadded:: 3.8 ``_USE_POSIX_SPAWN``
.. versionadded:: 3.11 ``_USE_VFORK``
4 changes: 3 additions & 1 deletion Lib/multiprocessing/util.py
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, [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._USE_VFORK)
finally:
os.close(errpipe_read)
os.close(errpipe_write)
Expand Down
5 changes: 4 additions & 1 deletion Lib/subprocess.py
Expand Up @@ -702,7 +702,10 @@ def _use_posix_spawn():
return False


# These are primarily fail-safe knobs for negatives. A True value does not
# guarantee the given libc/syscall API will be used.
_USE_POSIX_SPAWN = _use_posix_spawn()
_USE_VFORK = True


class Popen:
Expand Down Expand Up @@ -1792,7 +1795,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, _USE_VFORK)
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
Expand Up @@ -140,15 +140,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 @@ -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)
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
20 changes: 18 additions & 2 deletions Lib/test/test_subprocess.py
Expand Up @@ -1543,6 +1543,22 @@ def test_class_getitems(self):
self.assertIsInstance(subprocess.Popen[bytes], types.GenericAlias)
self.assertIsInstance(subprocess.CompletedProcess[str], types.GenericAlias)

@unittest.skipIf(not sysconfig.get_config_var("HAVE_VFORK"),
"vfork() not enabled by configure.")
@mock.patch("subprocess._fork_exec")
def test__use_vfork(self, mock_fork_exec):
self.assertTrue(subprocess._USE_VFORK) # The default value regardless.
mock_fork_exec.side_effect = RuntimeError("just testing args")
with self.assertRaises(RuntimeError):
subprocess.run([sys.executable, "-c", "pass"])
mock_fork_exec.assert_called_once()
self.assertTrue(mock_fork_exec.call_args.args[-1])
with mock.patch.object(subprocess, '_USE_VFORK', False):
with self.assertRaises(RuntimeError):
subprocess.run([sys.executable, "-c", "pass"])
self.assertFalse(mock_fork_exec.call_args_list[-1].args[-1])


class RunFuncTestCase(BaseTestCase):
def run_python(self, code, **kwargs):
"""Run Python code in a subprocess using subprocess.run"""
Expand Down Expand Up @@ -3107,7 +3123,7 @@ def test_fork_exec(self):
1, 2, 3, 4,
True, True,
False, [], 0, -1,
func)
func, False)
# 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 +3172,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
@@ -0,0 +1,2 @@
Provide a way to disable :mod:`subprocess` use of ``vfork()`` just in case
it is ever needed and document the existing mechanism for ``posix_spawn()``.
7 changes: 4 additions & 3 deletions Modules/_posixsubprocess.c
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 allow_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, &allow_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 && allow_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 cd5726f

Please sign in to comment.