gh-121381 Remove subprocess._USE_VFORK escape hatch by cmaloney · Pull Request #121383 · python/cpython · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 5 additions & 20 deletions Doc/library/subprocess.rst
3 changes: 1 addition & 2 deletions Lib/multiprocessing/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 1 addition & 2 deletions Lib/subprocess.py
Original file line number Diff line number Diff line change
Expand Up @@ -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')


Expand Down Expand Up @@ -1902,7 +1901,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
Expand Down
6 changes: 3 additions & 3 deletions Lib/test/test_capi/test_misc.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,15 +120,15 @@ 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):
return sys.maxsize
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):
Expand All @@ -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")
Expand Down
22 changes: 1 addition & 21 deletions Lib/test/test_subprocess.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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.")
Expand Down Expand Up @@ -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),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
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.
7 changes: 3 additions & 4 deletions Modules/_posixsubprocess.c
15 changes: 5 additions & 10 deletions Modules/clinic/_posixsubprocess.c.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.