From 97933df9e980819d8c7ae247adc0d5b2b8673f20 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Tue, 26 Apr 2022 00:10:14 +0000 Subject: [PATCH 1/7] [3.10] Conservative backport of `subprocess._USE_VFORK` 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. --- Lib/subprocess.py | 3 + Lib/test/test_subprocess.py | 24 ++++++++ ...2-04-26-00-10-06.gh-issue-91401.mddRC8.rst | 5 ++ Modules/_posixsubprocess.c | 55 +++++++++++++------ 4 files changed, 70 insertions(+), 17 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 ccb46a6337b3d96..a414321b9d197e2 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 7bb049296912b9b..9a4f258873ee7c7 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1528,6 +1528,7 @@ def test_class_getitems(self): self.assertIsInstance(subprocess.Popen[bytes], types.GenericAlias) self.assertIsInstance(subprocess.CompletedProcess[str], types.GenericAlias) + class RunFuncTestCase(BaseTestCase): def run_python(self, code, **kwargs): """Run Python code in a subprocess using subprocess.run""" @@ -1702,6 +1703,29 @@ 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. + self.assertEqual(self.run_python("pass").returncode, 0) + 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 000000000000000..2254d2be4dd0b63 --- /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. There is currently no known +need for this. If you find a need, please only set it to ``False`` and file +an issue as to why and leaving a comment in your code pointing to the issue. +This attribute is documented in 3.11. diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 18a81c6ed8b353e..cdd9ccddd61876b 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -936,24 +936,45 @@ 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) { - /* 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 - * pthread_sigmask(), but signal handlers installed by C libraries - * normally service only signals originating from *within the process*, - * so it should be sufficient to consider any library function that - * might send such a signal to be vfork-unsafe and do not call it in - * the child. - */ - sigset_t all_sigs; - sigfillset(&all_sigs); - if ((saved_errno = pthread_sigmask(SIG_BLOCK, &all_sigs, &old_sigs))) { - goto cleanup; + if (preexec_fn == Py_None) { + PyObject *subprocess_module = PyImport_ImportModule("subprocess"); + if (subprocess_module != NULL) { + int allow_vfork; + PyObject *allow_vfork_obj = PyObject_GetAttrString( + subprocess_module, "_USE_VFORK"); + Py_DECREF(subprocess_module); + if (allow_vfork_obj) { + 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. */ + allow_vfork = 1; /* 3.10.0 behavior */ + } + 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 + * pthread_sigmask(), but signal handlers installed by C libraries + * normally service only signals originating from *within the process*, + * so it should be sufficient to consider any library function that + * might send such a signal to be vfork-unsafe and do not call it in + * the child. + */ + sigset_t all_sigs; + sigfillset(&all_sigs); + if ((saved_errno = pthread_sigmask(SIG_BLOCK, &all_sigs, &old_sigs))) { + goto cleanup; + } + old_sigmask = &old_sigs; + } + } else { + PyErr_Clear(); /* no subprocess module? suspicious; don't care. */ } - old_sigmask = &old_sigs; - } + } /* no preexec_fn */ #endif pid = do_fork_exec(exec_array, argv, envp, cwd, From 7e086a677570a4b6fc71a011f12d1f4316a1ae9d Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Tue, 26 Apr 2022 00:17:41 +0000 Subject: [PATCH 2/7] undo addition of a blank line (clean diff) --- Lib/test/test_subprocess.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 9a4f258873ee7c7..485fc5f2b556851 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1528,7 +1528,6 @@ def test_class_getitems(self): self.assertIsInstance(subprocess.Popen[bytes], types.GenericAlias) self.assertIsInstance(subprocess.CompletedProcess[str], types.GenericAlias) - class RunFuncTestCase(BaseTestCase): def run_python(self, code, **kwargs): """Run Python code in a subprocess using subprocess.run""" From 6eae10d10b54a7e3e00d033c6681826737e45e4f Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Tue, 26 Apr 2022 00:18:52 +0000 Subject: [PATCH 3/7] remove an unnecessary test assert. Other tests obviously have this case covered. --- Lib/test/test_subprocess.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 485fc5f2b556851..b91791a02a2e52f 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1709,7 +1709,6 @@ def test__use_vfork(self): # 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. - self.assertEqual(self.run_python("pass").returncode, 0) with mock.patch.object(subprocess, "_USE_VFORK", False): self.assertEqual(self.run_python("pass").returncode, 0, msg="False _USE_VFORK failed") From 88a01e1142d1e17e92e32472cf88d8645846f0c8 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Tue, 26 Apr 2022 00:26:36 +0000 Subject: [PATCH 4/7] Reduce the diff size further. Moves the allow_vfork checks into a stanza before the existing setsig stuff. Avoids reindenting existing code. --- Modules/_posixsubprocess.c | 41 +++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index cdd9ccddd61876b..f8f55e65f6c2571 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -936,10 +936,10 @@ 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; + int allow_vfork = 1; /* 3.10.0 behavior */ if (preexec_fn == Py_None) { PyObject *subprocess_module = PyImport_ImportModule("subprocess"); if (subprocess_module != NULL) { - int allow_vfork; PyObject *allow_vfork_obj = PyObject_GetAttrString( subprocess_module, "_USE_VFORK"); Py_DECREF(subprocess_module); @@ -952,29 +952,30 @@ subprocess_fork_exec(PyObject *module, PyObject *args) } } else { PyErr_Clear(); /* No _USE_VFORK attribute. */ - allow_vfork = 1; /* 3.10.0 behavior */ - } - 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 - * pthread_sigmask(), but signal handlers installed by C libraries - * normally service only signals originating from *within the process*, - * so it should be sufficient to consider any library function that - * might send such a signal to be vfork-unsafe and do not call it in - * the child. - */ - sigset_t all_sigs; - sigfillset(&all_sigs); - if ((saved_errno = pthread_sigmask(SIG_BLOCK, &all_sigs, &old_sigs))) { - goto cleanup; - } - old_sigmask = &old_sigs; } } else { PyErr_Clear(); /* no subprocess module? suspicious; don't care. */ } - } /* no preexec_fn */ + } 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 + * pthread_sigmask(), but signal handlers installed by C libraries + * normally service only signals originating from *within the process*, + * so it should be sufficient to consider any library function that + * might send such a signal to be vfork-unsafe and do not call it in + * the child. + */ + sigset_t all_sigs; + sigfillset(&all_sigs); + if ((saved_errno = pthread_sigmask(SIG_BLOCK, &all_sigs, &old_sigs))) { + goto cleanup; + } + old_sigmask = &old_sigs; + } #endif pid = do_fork_exec(exec_array, argv, envp, cwd, From ddb8c2334f76601d9152cb928185a04aec20f41c Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Tue, 26 Apr 2022 00:28:38 +0000 Subject: [PATCH 5/7] better idiom, don't initialize to a default. --- Modules/_posixsubprocess.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index f8f55e65f6c2571..66f89b92f5b65b4 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -936,8 +936,9 @@ 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; - int allow_vfork = 1; /* 3.10.0 behavior */ + 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( From 37192dc38dc6aff10bff135bf68b1dc271d3b238 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Tue, 26 Apr 2022 00:30:02 +0000 Subject: [PATCH 6/7] consistency in NULL check idioms. --- Modules/_posixsubprocess.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 66f89b92f5b65b4..b852ad71d722512 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -944,7 +944,7 @@ subprocess_fork_exec(PyObject *module, PyObject *args) PyObject *allow_vfork_obj = PyObject_GetAttrString( subprocess_module, "_USE_VFORK"); Py_DECREF(subprocess_module); - if (allow_vfork_obj) { + if (allow_vfork_obj != NULL) { allow_vfork = PyObject_IsTrue(allow_vfork_obj); Py_DECREF(allow_vfork_obj); if (allow_vfork < 0) { From 35601ba014366d2fae98b4d6638c5cb25ffab34a Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 1 May 2022 15:09:21 -0700 Subject: [PATCH 7/7] reword the news entry. --- .../Library/2022-04-26-00-10-06.gh-issue-91401.mddRC8.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 index 2254d2be4dd0b63..964a54f8935c2e5 100644 --- 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 @@ -1,5 +1,5 @@ Provide a fail-safe way to disable :mod:`subprocess` use of ``vfork()`` via -a private ``subprocess._USE_VFORK`` attribute. There is currently no known -need for this. If you find a need, please only set it to ``False`` and file -an issue as to why and leaving a comment in your code pointing to the issue. -This attribute is documented in 3.11. +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.