From d19dd0c641351d3106f17ac443fd87de2a6e8cf3 Mon Sep 17 00:00:00 2001 From: Cody Maloney Date: Fri, 28 Jun 2024 23:17:57 -0700 Subject: [PATCH 1/7] Remove subprocess._USE_VFORK escape hatch This flag was added as an escape hatch in gh-91401 and backported to Python 3.10. The flag broke at some point between its addition and now. As there is currently no publicly known environments that require this, remove it rather than work on fixing it. This leaves the flag in the subprocess module to not break code which may have used / checked the flag itself. discussion: https://discuss.python.org/t/subprocess-use-vfork-escape-hatch-broken-fix-or-remove/56915/2 --- Doc/library/subprocess.rst | 17 +++----------- Lib/multiprocessing/util.py | 3 +-- Lib/subprocess.py | 2 +- Lib/test/test_capi/test_misc.py | 6 ++--- Lib/test/test_subprocess.py | 22 +------------------ ...-06-28-23-17-22.gh-issue-120754.i2xL7P.rst | 2 ++ Modules/_posixsubprocess.c | 7 +++--- Modules/clinic/_posixsubprocess.c.h | 15 +++++-------- 8 files changed, 19 insertions(+), 55 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2024-06-28-23-17-22.gh-issue-120754.i2xL7P.rst diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index f520d989e0c70d..bd3daf4010ce62 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -1564,26 +1564,13 @@ runtime): .. _disable_vfork: .. _disable_posix_spawn: -Disabling use of ``vfork()`` or ``posix_spawn()`` +Disabling use of ``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 -:const:`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 -:const:`subprocess._USE_POSIX_SPAWN` attribute if you need to prevent use of -that. - :: subprocess._USE_POSIX_SPAWN = False # See CPython issue gh-NNNNNN. @@ -1599,3 +1586,5 @@ code. .. versionadded:: 3.8 ``_USE_POSIX_SPAWN`` .. versionadded:: 3.11 ``_USE_VFORK`` +.. versionchanged:: 3.14 ``_USE_VFORK`` + :const:`subprocess._USE_VFORK` has no effect diff --git a/Lib/multiprocessing/util.py b/Lib/multiprocessing/util.py index 4f471fbde71ace..d48ef8a86b34e1 100644 --- a/Lib/multiprocessing/util.py +++ b/Lib/multiprocessing/util.py @@ -445,8 +445,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, -1, None, None, None, -1, None, - subprocess._USE_VFORK) + False, False, -1, None, None, None, -1, None) finally: os.close(errpipe_read) os.close(errpipe_write) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index bc08878db313df..9dc38951998c3c 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -1902,7 +1902,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, errpipe_read, errpipe_write, restore_signals, start_new_session, process_group, gid, gids, uid, umask, - preexec_fn, _USE_VFORK) + preexec_fn) self._child_created = True finally: # be sure the FD is closed no matter what diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index 9de97c0c2c776a..5c4547da1bdc53 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -120,7 +120,7 @@ def __len__(self): return 1 with self.assertRaisesRegex(TypeError, 'indexing'): _posixsubprocess.fork_exec( - 1,Z(),True,(1, 2),5,6,7,8,9,10,11,12,13,14,True,True,17,False,19,20,21,22,False) + 1,Z(),True,(1, 2),5,6,7,8,9,10,11,12,13,14,True,True,17,False,19,20,21,22) # Issue #15736: overflow in _PySequence_BytesToCharpArray() class Z(object): def __len__(self): @@ -128,7 +128,7 @@ def __len__(self): def __getitem__(self, i): return b'x' self.assertRaises(MemoryError, _posixsubprocess.fork_exec, - 1,Z(),True,(1, 2),5,6,7,8,9,10,11,12,13,14,True,True,17,False,19,20,21,22,False) + 1,Z(),True,(1, 2),5,6,7,8,9,10,11,12,13,14,True,True,17,False,19,20,21,22) @unittest.skipUnless(_posixsubprocess, '_posixsubprocess required for this test.') def test_subprocess_fork_exec(self): @@ -138,7 +138,7 @@ def __len__(self): # Issue #15738: crash in subprocess_fork_exec() self.assertRaises(TypeError, _posixsubprocess.fork_exec, - Z(),[b'1'],True,(1, 2),5,6,7,8,9,10,11,12,13,14,True,True,17,False,19,20,21,22,False) + Z(),[b'1'],True,(1, 2),5,6,7,8,9,10,11,12,13,14,True,True,17,False,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 8b69cd03ba7f24..972da8107f300b 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -3278,7 +3278,7 @@ def __int__(self): 1, 2, 3, 4, True, True, 0, None, None, None, -1, - None, True) + None) self.assertIn('fds_to_keep', str(c.exception)) finally: if not gc_enabled: @@ -3413,25 +3413,6 @@ def __del__(self): self.assertEqual(out.strip(), b"OK") self.assertIn(b"preexec_fn not supported at interpreter shutdown", err) - @unittest.skipIf(not sysconfig.get_config_var("HAVE_VFORK"), - "vfork() not enabled by configure.") - @mock.patch("subprocess._fork_exec") - @mock.patch("subprocess._USE_POSIX_SPAWN", new=False) - 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() - # NOTE: These assertions are *ugly* as they require the last arg - # to remain the have_vfork boolean. We really need to refactor away - # from the giant "wall of args" internal C extension API. - 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]) - @unittest.skipIf(not sysconfig.get_config_var("HAVE_VFORK"), "vfork() not enabled by configure.") @unittest.skipIf(sys.platform != "linux", "Linux only, requires strace.") @@ -3478,7 +3459,6 @@ def test_vfork_used_when_expected(self): # Test that each individual thing that would disable the use of vfork # actually disables it. for sub_name, preamble, sp_kwarg, expect_permission_error in ( - ("!use_vfork", "subprocess._USE_VFORK = False", "", False), ("preexec", "", "preexec_fn=lambda: None", False), ("setgid", "", f"group={os.getgid()}", True), ("setuid", "", f"user={os.getuid()}", True), diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-06-28-23-17-22.gh-issue-120754.i2xL7P.rst b/Misc/NEWS.d/next/Core and Builtins/2024-06-28-23-17-22.gh-issue-120754.i2xL7P.rst new file mode 100644 index 00000000000000..2022cb8b8788a2 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2024-06-28-23-17-22.gh-issue-120754.i2xL7P.rst @@ -0,0 +1,2 @@ +Remove :const:`subprocess._USE_VFORK` escape hatch code and documentation. +It was added just in case, and doesn't have any known cases that require it. diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index daec4ad708dea4..ad6d7ceda84e37 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -977,7 +977,6 @@ _posixsubprocess.fork_exec as subprocess_fork_exec uid as uid_object: object child_umask: int preexec_fn: object - allow_vfork: bool / Spawn a fresh new child process. @@ -1014,8 +1013,8 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_args, pid_t pgid_to_set, PyObject *gid_object, PyObject *extra_groups_packed, PyObject *uid_object, int child_umask, - PyObject *preexec_fn, int allow_vfork) -/*[clinic end generated code: output=7ee4f6ee5cf22b5b input=51757287ef266ffa]*/ + PyObject *preexec_fn) +/*[clinic end generated code: output=288464dc56e373c7 input=f311c3bcb5dd55c8]*/ { PyObject *converted_args = NULL, *fast_args = NULL; PyObject *preexec_fn_args_tuple = NULL; @@ -1218,7 +1217,7 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_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 && allow_vfork && + if (preexec_fn == Py_None && uid == (uid_t)-1 && gid == (gid_t)-1 && extra_group_size < 0) { /* Block all signals to ensure that no signal handlers are run in the * child process while it shares memory with us. Note that signals diff --git a/Modules/clinic/_posixsubprocess.c.h b/Modules/clinic/_posixsubprocess.c.h index dd7644de6b7534..d52629cf6eaa5b 100644 --- a/Modules/clinic/_posixsubprocess.c.h +++ b/Modules/clinic/_posixsubprocess.c.h @@ -9,7 +9,7 @@ PyDoc_STRVAR(subprocess_fork_exec__doc__, " env, p2cread, p2cwrite, c2pread, c2pwrite, errread, errwrite,\n" " errpipe_read, errpipe_write, restore_signals, call_setsid,\n" " pgid_to_set, gid, extra_groups, uid, child_umask, preexec_fn,\n" -" allow_vfork, /)\n" +" /)\n" "--\n" "\n" "Spawn a fresh new child process.\n" @@ -48,7 +48,7 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_args, pid_t pgid_to_set, PyObject *gid_object, PyObject *extra_groups_packed, PyObject *uid_object, int child_umask, - PyObject *preexec_fn, int allow_vfork); + PyObject *preexec_fn); static PyObject * subprocess_fork_exec(PyObject *module, PyObject *const *args, Py_ssize_t nargs) @@ -76,9 +76,8 @@ subprocess_fork_exec(PyObject *module, PyObject *const *args, Py_ssize_t nargs) PyObject *uid_object; int child_umask; PyObject *preexec_fn; - int allow_vfork; - if (!_PyArg_CheckPositional("fork_exec", nargs, 23, 23)) { + if (!_PyArg_CheckPositional("fork_exec", nargs, 22, 22)) { goto exit; } process_args = args[0]; @@ -146,13 +145,9 @@ subprocess_fork_exec(PyObject *module, PyObject *const *args, Py_ssize_t nargs) goto exit; } preexec_fn = args[21]; - allow_vfork = PyObject_IsTrue(args[22]); - if (allow_vfork < 0) { - goto exit; - } - return_value = subprocess_fork_exec_impl(module, process_args, executable_list, close_fds, py_fds_to_keep, cwd_obj, env_list, p2cread, p2cwrite, c2pread, c2pwrite, errread, errwrite, errpipe_read, errpipe_write, restore_signals, call_setsid, pgid_to_set, gid_object, extra_groups_packed, uid_object, child_umask, preexec_fn, allow_vfork); + return_value = subprocess_fork_exec_impl(module, process_args, executable_list, close_fds, py_fds_to_keep, cwd_obj, env_list, p2cread, p2cwrite, c2pread, c2pwrite, errread, errwrite, errpipe_read, errpipe_write, restore_signals, call_setsid, pgid_to_set, gid_object, extra_groups_packed, uid_object, child_umask, preexec_fn); exit: return return_value; } -/*[clinic end generated code: output=48555f5965a871be input=a9049054013a1b77]*/ +/*[clinic end generated code: output=942bc2748a9c2785 input=a9049054013a1b77]*/ From 3226642b68a40131dfdbb7a7c63cc6b8bb6b44b9 Mon Sep 17 00:00:00 2001 From: Cody Maloney Date: Thu, 4 Jul 2024 14:57:11 -0700 Subject: [PATCH 2/7] Fix News docs issues --- Doc/library/subprocess.rst | 2 +- ...2xL7P.rst => 2024-06-28-23-17-22.gh-issue-121381.i2xL7P.rst} | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename Misc/NEWS.d/next/Core and Builtins/{2024-06-28-23-17-22.gh-issue-120754.i2xL7P.rst => 2024-06-28-23-17-22.gh-issue-121381.i2xL7P.rst} (50%) diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index bd3daf4010ce62..a800a1c07adc9c 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -1587,4 +1587,4 @@ code. .. versionadded:: 3.8 ``_USE_POSIX_SPAWN`` .. versionadded:: 3.11 ``_USE_VFORK`` .. versionchanged:: 3.14 ``_USE_VFORK`` - :const:`subprocess._USE_VFORK` has no effect + ``_USE_VFORK`` has no effect diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-06-28-23-17-22.gh-issue-120754.i2xL7P.rst b/Misc/NEWS.d/next/Core and Builtins/2024-06-28-23-17-22.gh-issue-121381.i2xL7P.rst similarity index 50% rename from Misc/NEWS.d/next/Core and Builtins/2024-06-28-23-17-22.gh-issue-120754.i2xL7P.rst rename to Misc/NEWS.d/next/Core and Builtins/2024-06-28-23-17-22.gh-issue-121381.i2xL7P.rst index 2022cb8b8788a2..3a02145378e2cd 100644 --- a/Misc/NEWS.d/next/Core and Builtins/2024-06-28-23-17-22.gh-issue-120754.i2xL7P.rst +++ b/Misc/NEWS.d/next/Core and Builtins/2024-06-28-23-17-22.gh-issue-121381.i2xL7P.rst @@ -1,2 +1,2 @@ -Remove :const:`subprocess._USE_VFORK` escape hatch code and documentation. +Remove ``subprocess._USE_VFORK`` escape hatch code and documentation. It was added just in case, and doesn't have any known cases that require it. From 904c87a7717b5c9266b71a27f60f2ac1540f4513 Mon Sep 17 00:00:00 2001 From: Cody Maloney Date: Thu, 4 Jul 2024 15:13:09 -0700 Subject: [PATCH 3/7] docs: Remove repeated _USE_VFORK in rendering --- 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 a800a1c07adc9c..7c80a5846faeb3 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -1587,4 +1587,4 @@ code. .. versionadded:: 3.8 ``_USE_POSIX_SPAWN`` .. versionadded:: 3.11 ``_USE_VFORK`` .. versionchanged:: 3.14 ``_USE_VFORK`` - ``_USE_VFORK`` has no effect + has no effect From cd086a3adfbd735968dfda2c1087466b4fae736a Mon Sep 17 00:00:00 2001 From: Cody Maloney Date: Thu, 4 Jul 2024 20:23:03 -0700 Subject: [PATCH 4/7] Tweak docs to make case match --- Doc/library/subprocess.rst | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index 7c80a5846faeb3..b8fd2cbf31c795 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -1574,10 +1574,11 @@ improves performance. :: subprocess._USE_POSIX_SPAWN = False # See CPython issue gh-NNNNNN. + subprocess._USE_VFORK = 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 +effect on older or newer versions when unsupported. Do not assume the attributes +are available to read. Despite their names, a true value does not indicate the corresponding function will be used, only that it may be. Please file issues any time you have to use these private knobs with a way to From 9a804f9fa1212a1637ecc39937ae86fb8415269a Mon Sep 17 00:00:00 2001 From: Cody Maloney Date: Fri, 5 Jul 2024 12:23:08 -0700 Subject: [PATCH 5/7] Update to remove _USE_VFORK entirely --- Doc/library/subprocess.rst | 11 +++-------- Lib/subprocess.py | 1 - 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index b8fd2cbf31c795..bb8583f3593066 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -1561,7 +1561,6 @@ runtime): Module which provides function to parse and escape command lines. -.. _disable_vfork: .. _disable_posix_spawn: Disabling use of ``posix_spawn()`` @@ -1574,11 +1573,10 @@ improves performance. :: subprocess._USE_POSIX_SPAWN = False # See CPython issue gh-NNNNNN. - subprocess._USE_VFORK = 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 or newer versions when unsupported. Do not assume the attributes -are available to read. Despite their names, a true value does not indicate the +It is safe to set this to false on any Python version. It will have no +effect on older or newer versions where unsupported. Do not assume the attributes +are available to read. Despite the name, a true value does not indicate the corresponding function will be used, only that it may be. Please file issues any time you have to use these private knobs with a way to @@ -1586,6 +1584,3 @@ 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`` -.. versionchanged:: 3.14 ``_USE_VFORK`` - has no effect diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 9dc38951998c3c..88f0230b05fbc7 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -749,7 +749,6 @@ def _use_posix_spawn(): # 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 _HAVE_POSIX_SPAWN_CLOSEFROM = hasattr(os, 'POSIX_SPAWN_CLOSEFROM') From 8c4cf55e2aba644f53879f7b44a06781b5f4c870 Mon Sep 17 00:00:00 2001 From: Cody Maloney Date: Fri, 5 Jul 2024 12:41:31 -0700 Subject: [PATCH 6/7] Simplify further per review --- Doc/library/subprocess.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index bb8583f3593066..145f516902fdc8 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -1563,7 +1563,7 @@ runtime): .. _disable_posix_spawn: -Disabling use of ``posix_spawn()`` +Disable use of ``posix_spawn()`` ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ On Linux, :mod:`subprocess` defaults to using the ``vfork()`` system call @@ -1575,8 +1575,8 @@ improves performance. subprocess._USE_POSIX_SPAWN = False # See CPython issue gh-NNNNNN. It is safe to set this to false on any Python version. It will have no -effect on older or newer versions where unsupported. Do not assume the attributes -are available to read. Despite the name, a true value does not indicate the +effect on older or newer versions where unsupported. Do not assume the attribute +is available to read. Despite the name, a true value does not indicate the corresponding function will be used, only that it may be. Please file issues any time you have to use these private knobs with a way to From f8540c97508b4f2e4913952068ebb9e39b48dc6e Mon Sep 17 00:00:00 2001 From: Cody Maloney Date: Fri, 5 Jul 2024 17:09:21 -0700 Subject: [PATCH 7/7] tweak title underline length --- 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 145f516902fdc8..d8aa45ea33ea08 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -1564,7 +1564,7 @@ runtime): .. _disable_posix_spawn: Disable use of ``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