From ea1eba03e7e8401d5acf8b30b56b41faa209e8c6 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 1 May 2022 16:09:50 -0700 Subject: [PATCH] [3.10] gh-91401: Conservative backport of `subprocess._USE_VFORK` (#91932) This does not alter the `_posixsubprocess.fork_exec()` private API to avoid issues for anyone relying on that (bad idea) or for anyone who's `subprocess.py` and `_posixsubprocess.so` upgrades may not become visible to existing Python 3.10 processes at the same time. Backports the concept of cd5726fe674eaff442510eeb6c75628858be9e9f. Provides a fail-safe way to disable vfork for #91401. I didn't backport the documentation as I don't actually expect this to be used and `.. versionadded: 3.10.5` always looks weird in docs. It's being done more to have a fail-safe in place for people just in case. --- Lib/subprocess.py | 3 +++ Lib/test/test_subprocess.py | 22 +++++++++++++++ ...2-04-26-00-10-06.gh-issue-91401.mddRC8.rst | 5 ++++ Modules/_posixsubprocess.c | 27 +++++++++++++++++-- 4 files changed, 55 insertions(+), 2 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2022-04-26-00-10-06.gh-issue-91401.mddRC8.rst diff --git a/Lib/subprocess.py b/Lib/subprocess.py index ccb46a6337b3d9..a414321b9d197e 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -691,7 +691,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: diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 7bb049296912b9..b91791a02a2e52 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1702,6 +1702,28 @@ def test_run_with_shell_timeout_and_capture_output(self): msg="TimeoutExpired was delayed! Bad traceback:\n```\n" f"{stacks}```") + @unittest.skipIf(not sysconfig.get_config_var("HAVE_VFORK"), + "vfork() not enabled by configure.") + def test__use_vfork(self): + # Attempts code coverage within _posixsubprocess.c on the code that + # probes the subprocess module for the existence and value of this + # attribute in 3.10.5. + self.assertTrue(subprocess._USE_VFORK) # The default value regardless. + with mock.patch.object(subprocess, "_USE_VFORK", False): + self.assertEqual(self.run_python("pass").returncode, 0, + msg="False _USE_VFORK failed") + + class RaisingBool: + def __bool__(self): + raise RuntimeError("force PyObject_IsTrue to return -1") + + with mock.patch.object(subprocess, "_USE_VFORK", RaisingBool()): + self.assertEqual(self.run_python("pass").returncode, 0, + msg="odd bool()-error _USE_VFORK failed") + del subprocess._USE_VFORK + self.assertEqual(self.run_python("pass").returncode, 0, + msg="lack of a _USE_VFORK attribute failed") + def _get_test_grp_name(): for name_group in ('staff', 'nogroup', 'grp', 'nobody', 'nfsnobody'): diff --git a/Misc/NEWS.d/next/Library/2022-04-26-00-10-06.gh-issue-91401.mddRC8.rst b/Misc/NEWS.d/next/Library/2022-04-26-00-10-06.gh-issue-91401.mddRC8.rst new file mode 100644 index 00000000000000..964a54f8935c2e --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-04-26-00-10-06.gh-issue-91401.mddRC8.rst @@ -0,0 +1,5 @@ +Provide a fail-safe way to disable :mod:`subprocess` use of ``vfork()`` via +a private ``subprocess._USE_VFORK`` attribute. While there is currently no +known need for this, if you find a need please only set it to ``False``. +File a CPython issue as to why you needed it and link to that from a +comment in your code. This attribute is documented as a footnote in 3.11. diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 18a81c6ed8b353..b852ad71d72251 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -936,8 +936,31 @@ 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 && - !call_setuid && !call_setgid && !call_setgroups) { + int allow_vfork; + if (preexec_fn == Py_None) { + allow_vfork = 1; /* 3.10.0 behavior */ + PyObject *subprocess_module = PyImport_ImportModule("subprocess"); + if (subprocess_module != NULL) { + PyObject *allow_vfork_obj = PyObject_GetAttrString( + subprocess_module, "_USE_VFORK"); + Py_DECREF(subprocess_module); + if (allow_vfork_obj != NULL) { + allow_vfork = PyObject_IsTrue(allow_vfork_obj); + Py_DECREF(allow_vfork_obj); + if (allow_vfork < 0) { + PyErr_Clear(); /* Bad _USE_VFORK attribute. */ + allow_vfork = 1; /* 3.10.0 behavior */ + } + } else { + PyErr_Clear(); /* No _USE_VFORK attribute. */ + } + } else { + PyErr_Clear(); /* no subprocess module? suspicious; don't care. */ + } + } else { + allow_vfork = 0; + } + if (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 * used internally by C libraries won't be blocked by