[Python-checkins] [3.10] gh-91401: Conservative backport of `subprocess._USE_VFORK` (#91932)

gpshead webhook-mailer at python.org
Sun May 1 19:09:55 EDT 2022


https://github.com/python/cpython/commit/ea1eba03e7e8401d5acf8b30b56b41faa209e8c6
commit: ea1eba03e7e8401d5acf8b30b56b41faa209e8c6
branch: 3.10
author: Gregory P. Smith <greg at krypto.org>
committer: gpshead <greg at krypto.org>
date: 2022-05-01T16:09:50-07:00
summary:

[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.

files:
A Misc/NEWS.d/next/Library/2022-04-26-00-10-06.gh-issue-91401.mddRC8.rst
M Lib/subprocess.py
M Lib/test/test_subprocess.py
M Modules/_posixsubprocess.c

diff --git a/Lib/subprocess.py b/Lib/subprocess.py
index ccb46a6337b3d..a414321b9d197 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 7bb049296912b..b91791a02a2e5 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 0000000000000..964a54f8935c2
--- /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 18a81c6ed8b35..b852ad71d7225 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



More information about the Python-checkins mailing list