From aba06a0fb2b9b1c054addc66d3dd2acf5c413e8e Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Tue, 12 Apr 2022 23:54:37 +0000 Subject: [PATCH 1/5] gh-91401: Add a failsafe way to disable vfork. 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. --- Doc/library/subprocess.rst | 26 ++++++++++++++++++++++++++ Lib/multiprocessing/util.py | 4 +++- Lib/subprocess.py | 10 +++++++++- Lib/test/test_capi.py | 6 +++--- Lib/test/test_subprocess.py | 7 ++++--- Modules/_posixsubprocess.c | 7 ++++--- 6 files changed, 49 insertions(+), 11 deletions(-) diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index ab9f1d88a0fc26..9046fc88148c3f 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -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`` diff --git a/Lib/multiprocessing/util.py b/Lib/multiprocessing/util.py index a4683339820f5f..946fe6d5808171 100644 --- a/Lib/multiprocessing/util.py +++ b/Lib/multiprocessing/util.py @@ -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) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index b58c5784283902..bf48c8365edae6 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -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 @@ -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 diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py index 714a2d98e9fb5e..9d9ff0a490b126 100644 --- a/Lib/test/test_capi.py +++ b/Lib/test/test_capi.py @@ -118,7 +118,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): @@ -126,7 +126,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): @@ -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") diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 8603b9888bb984..8497c6912e2b3c 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -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 @@ -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: @@ -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 diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 440c7c5b335994..7521244c6d16f3 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -751,9 +751,10 @@ 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, @@ -761,7 +762,7 @@ subprocess_fork_exec(PyObject *module, PyObject *args) &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) && @@ -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 From c99fb75786c41ef36bf4e5606474730b0c92e036 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Mon, 25 Apr 2022 21:17:34 +0000 Subject: [PATCH 2/5] Switch the attr name to _USE_VFORK for consistency No double negatives. Also documents _USE_POSIX_SPAWN. --- Doc/library/subprocess.rst | 32 +++++++++++++++++++++----------- Lib/multiprocessing/util.py | 2 +- Lib/subprocess.py | 11 ++--------- Lib/test/test_subprocess.py | 5 ++--- Modules/_posixsubprocess.c | 6 +++--- 5 files changed, 29 insertions(+), 27 deletions(-) diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index 9046fc88148c3f..fca55496492685 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -1532,9 +1532,10 @@ runtime): .. _disable_vfork: +.. _disable_posix_spawn: -Disabling potential use of ``vfork()`` -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +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 @@ -1542,16 +1543,25 @@ 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. +: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()`` within its libc implementation. +``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 this attribute on older Python versions. Do not assume it -exists to be read until 3.11. +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.11 ``disable_vfork_reason`` +.. versionadded:: 3.8 ``_USE_POSIX_SPAWN`` +.. versionadded:: 3.11 ``_USE_VFORK`` diff --git a/Lib/multiprocessing/util.py b/Lib/multiprocessing/util.py index 946fe6d5808171..c05ea634fe5572 100644 --- a/Lib/multiprocessing/util.py +++ b/Lib/multiprocessing/util.py @@ -454,7 +454,7 @@ def spawnv_passfds(path, args, passfds): 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, - subprocess.disable_vfork_reason) + subprocess._USE_VFORK) finally: os.close(errpipe_read) os.close(errpipe_write) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index bf48c8365edae6..212773a43f595a 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -107,14 +107,6 @@ 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 @@ -711,6 +703,7 @@ def _use_posix_spawn(): _USE_POSIX_SPAWN = _use_posix_spawn() +_USE_VFORK = sys.platform == 'linux' class Popen: @@ -1800,7 +1793,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, disable_vfork_reason) + preexec_fn, _USE_VFORK) 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 8497c6912e2b3c..1eb56ff96c4ed3 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -3107,7 +3107,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 @@ -3614,8 +3614,7 @@ def test_getoutput(self): def test__all__(self): """Ensure that __all__ is populated properly.""" - intentionally_excluded = {"list2cmdline", "Handle", "pwd", "grp", - "fcntl", "disable_vfork_reason"} + intentionally_excluded = {"list2cmdline", "Handle", "pwd", "grp", "fcntl"} exported = set(subprocess.__all__) possible_exports = set() import types diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 7521244c6d16f3..2440609e31bcc2 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -751,7 +751,7 @@ 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; + int allow_vfork; if (!PyArg_ParseTuple( args, "OOpO!OOiiiiiiiiiiOOOiOp:fork_exec", @@ -762,7 +762,7 @@ subprocess_fork_exec(PyObject *module, PyObject *args) &errread, &errwrite, &errpipe_read, &errpipe_write, &restore_signals, &call_setsid, &gid_object, &groups_list, &uid_object, &child_umask, - &preexec_fn, &disable_vfork)) + &preexec_fn, &allow_vfork)) return NULL; if ((preexec_fn != Py_None) && @@ -941,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 && !disable_vfork && + 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 From b794394eea984f0cb5ecff9cf83198dc5f43c123 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Mon, 25 Apr 2022 21:27:24 +0000 Subject: [PATCH 3/5] Add a comment, default to True. Rather than re-encoding our platform selection here (configure.ac and runtime logic in _posixsubprocess already does that). --- Lib/subprocess.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 212773a43f595a..a5fa152715c145 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -702,8 +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 = sys.platform == 'linux' +_USE_VFORK = True class Popen: From 9154ad8f7119292ca6e07fdecfff20efa0a377be Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Mon, 25 Apr 2022 21:34:31 +0000 Subject: [PATCH 4/5] NEWS entry. --- .../next/Library/2022-04-25-21-33-48.gh-issue-91401._Jo4Bu.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2022-04-25-21-33-48.gh-issue-91401._Jo4Bu.rst diff --git a/Misc/NEWS.d/next/Library/2022-04-25-21-33-48.gh-issue-91401._Jo4Bu.rst b/Misc/NEWS.d/next/Library/2022-04-25-21-33-48.gh-issue-91401._Jo4Bu.rst new file mode 100644 index 00000000000000..7584710af30ff8 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-04-25-21-33-48.gh-issue-91401._Jo4Bu.rst @@ -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()``. From 9272669fd31c4bfaa56fc2d1c80599b463ab3820 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Mon, 25 Apr 2022 22:11:41 +0000 Subject: [PATCH 5/5] Add a unittest for _USE_VFORK. --- Lib/test/test_subprocess.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 1eb56ff96c4ed3..99b5947e54be68 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -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"""