From 4702552885811d0af8f0e4545f494336801ad4dd Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 31 Oct 2022 13:35:54 -0600 Subject: gh-98610: Adjust the Optional Restrictions on Subinterpreters (GH-98618) Previously, the optional restrictions on subinterpreters were: disallow fork, subprocess, and threads. By default, we were disallowing all three for "isolated" interpreters. We always allowed all three for the main interpreter and those created through the legacy `Py_NewInterpreter()` API. Those settings were a bit conservative, so here we've adjusted the optional restrictions to: fork, exec, threads, and daemon threads. The default for "isolated" interpreters disables fork, exec, and daemon threads. Regular threads are allowed by default. We continue always allowing everything For the main interpreter and the legacy API. In the code, we add `_PyInterpreterConfig.allow_exec` and `_PyInterpreterConfig.allow_daemon_threads`. We also add `Py_RTFLAGS_DAEMON_THREADS` and `Py_RTFLAGS_EXEC`. --- Include/cpython/initconfig.h | 14 +++++- Include/cpython/pystate.h | 13 ++--- Lib/test/test__xxsubinterpreters.py | 57 +++++++++++++++++++++- Lib/test/test_capi.py | 11 +++-- Lib/test/test_embed.py | 5 +- Lib/test/test_threading.py | 56 +++++++++++++++++++++ Lib/threading.py | 8 ++- .../2022-10-24-12-09-17.gh-issue-98610.PLX2Np.rst | 9 ++++ Modules/_posixsubprocess.c | 11 +---- Modules/_testcapimodule.c | 24 ++++++--- Modules/_threadmodule.c | 20 ++++++++ Modules/_winapi.c | 7 --- Modules/_xxsubinterpretersmodule.c | 8 ++- Modules/posixmodule.c | 14 ++++++ Python/pylifecycle.c | 10 +++- 15 files changed, 220 insertions(+), 47 deletions(-) create mode 100644 Misc/NEWS.d/next/C API/2022-10-24-12-09-17.gh-issue-98610.PLX2Np.rst diff --git a/Include/cpython/initconfig.h b/Include/cpython/initconfig.h index 64748cf..6ce42b4 100644 --- a/Include/cpython/initconfig.h +++ b/Include/cpython/initconfig.h @@ -245,15 +245,25 @@ PyAPI_FUNC(PyStatus) PyConfig_SetWideStringList(PyConfig *config, typedef struct { int allow_fork; - int allow_subprocess; + int allow_exec; int allow_threads; + int allow_daemon_threads; } _PyInterpreterConfig; +#define _PyInterpreterConfig_INIT \ + { \ + .allow_fork = 0, \ + .allow_exec = 0, \ + .allow_threads = 1, \ + .allow_daemon_threads = 0, \ + } + #define _PyInterpreterConfig_LEGACY_INIT \ { \ .allow_fork = 1, \ - .allow_subprocess = 1, \ + .allow_exec = 1, \ .allow_threads = 1, \ + .allow_daemon_threads = 1, \ } /* --- Helper functions --------------------------------------- */ diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h index 7996bd3..70c2342 100644 --- a/Include/cpython/pystate.h +++ b/Include/cpython/pystate.h @@ -11,16 +11,17 @@ is available in a given context. For example, forking the process might not be allowed in the current interpreter (i.e. os.fork() would fail). */ -// We leave the first 10 for less-specific features. - /* Set if threads are allowed. */ -#define Py_RTFLAGS_THREADS (1UL << 10) +#define Py_RTFLAGS_THREADS (1UL << 10) + +/* Set if daemon threads are allowed. */ +#define Py_RTFLAGS_DAEMON_THREADS (1UL << 11) /* Set if os.fork() is allowed. */ -#define Py_RTFLAGS_FORK (1UL << 15) +#define Py_RTFLAGS_FORK (1UL << 15) -/* Set if subprocesses are allowed. */ -#define Py_RTFLAGS_SUBPROCESS (1UL << 16) +/* Set if os.exec*() is allowed. */ +#define Py_RTFLAGS_EXEC (1UL << 16) PyAPI_FUNC(int) _PyInterpreterState_HasFeature(PyInterpreterState *interp, diff --git a/Lib/test/test__xxsubinterpreters.py b/Lib/test/test__xxsubinterpreters.py index f20aae8..66f29b9 100644 --- a/Lib/test/test__xxsubinterpreters.py +++ b/Lib/test/test__xxsubinterpreters.py @@ -801,7 +801,7 @@ class RunStringTests(TestBase): self.assertEqual(out, 'it worked!') def test_create_thread(self): - subinterp = interpreters.create(isolated=False) + subinterp = interpreters.create() script, file = _captured_script(""" import threading def f(): @@ -817,6 +817,61 @@ class RunStringTests(TestBase): self.assertEqual(out, 'it worked!') + def test_create_daemon_thread(self): + with self.subTest('isolated'): + expected = 'spam spam spam spam spam' + subinterp = interpreters.create(isolated=True) + script, file = _captured_script(f""" + import threading + def f(): + print('it worked!', end='') + + try: + t = threading.Thread(target=f, daemon=True) + t.start() + t.join() + except RuntimeError: + print('{expected}', end='') + """) + with file: + interpreters.run_string(subinterp, script) + out = file.read() + + self.assertEqual(out, expected) + + with self.subTest('not isolated'): + subinterp = interpreters.create(isolated=False) + script, file = _captured_script(""" + import threading + def f(): + print('it worked!', end='') + + t = threading.Thread(target=f, daemon=True) + t.start() + t.join() + """) + with file: + interpreters.run_string(subinterp, script) + out = file.read() + + self.assertEqual(out, 'it worked!') + + def test_os_exec(self): + expected = 'spam spam spam spam spam' + subinterp = interpreters.create() + script, file = _captured_script(f""" + import os, sys + try: + os.execl(sys.executable) + except RuntimeError: + print('{expected}', end='') + """) + with file: + interpreters.run_string(subinterp, script) + out = file.read() + + self.assertEqual(out, expected) + @support.requires_fork() def test_fork(self): import tempfile diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py index 2a35576..49f207e 100644 --- a/Lib/test/test_capi.py +++ b/Lib/test/test_capi.py @@ -1148,15 +1148,16 @@ class SubinterpreterTest(unittest.TestCase): import json THREADS = 1<<10 + DAEMON_THREADS = 1<<11 FORK = 1<<15 - SUBPROCESS = 1<<16 + EXEC = 1<<16 - features = ['fork', 'subprocess', 'threads'] + features = ['fork', 'exec', 'threads', 'daemon_threads'] kwlist = [f'allow_{n}' for n in features] for config, expected in { - (True, True, True): FORK | SUBPROCESS | THREADS, - (False, False, False): 0, - (False, True, True): SUBPROCESS | THREADS, + (True, True, True, True): FORK | EXEC | THREADS | DAEMON_THREADS, + (False, False, False, False): 0, + (False, False, True, False): THREADS, }.items(): kwargs = dict(zip(kwlist, config)) expected = { diff --git a/Lib/test/test_embed.py b/Lib/test/test_embed.py index 930f763..fa9815c 100644 --- a/Lib/test/test_embed.py +++ b/Lib/test/test_embed.py @@ -1655,11 +1655,12 @@ class InitConfigTests(EmbeddingTestsMixin, unittest.TestCase): def test_init_main_interpreter_settings(self): THREADS = 1<<10 + DAEMON_THREADS = 1<<11 FORK = 1<<15 - SUBPROCESS = 1<<16 + EXEC = 1<<16 expected = { # All optional features should be enabled. - 'feature_flags': THREADS | FORK | SUBPROCESS, + 'feature_flags': FORK | EXEC | THREADS | DAEMON_THREADS, } out, err = self.run_embedded_interpreter( 'test_init_main_interpreter_settings', diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index c664996..13ba506 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1305,6 +1305,62 @@ class SubinterpThreadingTests(BaseTestCase): self.assertIn("Fatal Python error: Py_EndInterpreter: " "not the last thread", err.decode()) + def _check_allowed(self, before_start='', *, + allowed=True, + daemon_allowed=True, + daemon=False, + ): + subinterp_code = textwrap.dedent(f""" + import test.support + import threading + def func(): + print('this should not have run!') + t = threading.Thread(target=func, daemon={daemon}) + {before_start} + t.start() + """) + script = textwrap.dedent(f""" + import test.support + test.support.run_in_subinterp_with_config( + {subinterp_code!r}, + allow_fork=True, + allow_exec=True, + allow_threads={allowed}, + allow_daemon_threads={daemon_allowed}, + ) + """) + with test.support.SuppressCrashReport(): + _, _, err = assert_python_ok("-c", script) + return err.decode() + + @cpython_only + def test_threads_not_allowed(self): + err = self._check_allowed( + allowed=False, + daemon_allowed=False, + daemon=False, + ) + self.assertIn('RuntimeError', err) + + @cpython_only + def test_daemon_threads_not_allowed(self): + with self.subTest('via Thread()'): + err = self._check_allowed( + allowed=True, + daemon_allowed=False, + daemon=True, + ) + self.assertIn('RuntimeError', err) + + with self.subTest('via Thread.daemon setter'): + err = self._check_allowed( + 't.daemon = True', + allowed=True, + daemon_allowed=False, + daemon=False, + ) + self.assertIn('RuntimeError', err) + class ThreadingExceptionTests(BaseTestCase): # A RuntimeError should be raised if Thread.start() is called diff --git a/Lib/threading.py b/Lib/threading.py index d030e12..723bd58 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -33,6 +33,7 @@ __all__ = ['get_ident', 'active_count', 'Condition', 'current_thread', # Rename some stuff so "from threading import *" is safe _start_new_thread = _thread.start_new_thread +_daemon_threads_allowed = _thread.daemon_threads_allowed _allocate_lock = _thread.allocate_lock _set_sentinel = _thread._set_sentinel get_ident = _thread.get_ident @@ -899,6 +900,8 @@ class Thread: self._args = args self._kwargs = kwargs if daemon is not None: + if daemon and not _daemon_threads_allowed(): + raise RuntimeError('daemon threads are disabled in this (sub)interpreter') self._daemonic = daemon else: self._daemonic = current_thread().daemon @@ -1226,6 +1229,8 @@ class Thread: def daemon(self, daemonic): if not self._initialized: raise RuntimeError("Thread.__init__() not called") + if daemonic and not _daemon_threads_allowed(): + raise RuntimeError('daemon threads are disabled in this interpreter') if self._started.is_set(): raise RuntimeError("cannot set daemon status of active thread") self._daemonic = daemonic @@ -1432,7 +1437,8 @@ class _MainThread(Thread): class _DummyThread(Thread): def __init__(self): - Thread.__init__(self, name=_newname("Dummy-%d"), daemon=True) + Thread.__init__(self, name=_newname("Dummy-%d"), + daemon=_daemon_threads_allowed()) self._started.set() self._set_ident() diff --git a/Misc/NEWS.d/next/C API/2022-10-24-12-09-17.gh-issue-98610.PLX2Np.rst b/Misc/NEWS.d/next/C API/2022-10-24-12-09-17.gh-issue-98610.PLX2Np.rst new file mode 100644 index 0000000..05bcfae --- /dev/null +++ b/Misc/NEWS.d/next/C API/2022-10-24-12-09-17.gh-issue-98610.PLX2Np.rst @@ -0,0 +1,9 @@ +Some configurable capabilities of sub-interpreters have changed. +They always allow subprocesses (:mod:`subprocess`) now, whereas before +subprocesses could be optionally disaallowed for a sub-interpreter. +Instead :func:`os.exec` can now be disallowed. +Disallowing daemon threads is now supported. Disallowing all threads +is still allowed, but is never done by default. +Note that the optional restrictions are only available through +``_Py_NewInterpreterFromConfig()``, which isn't a public API. +They do not affect the main interpreter, nor :c:func:`Py_NewInterpreter`. diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 8275b11..717b1cf 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -825,8 +825,8 @@ subprocess_fork_exec(PyObject *module, PyObject *args) &preexec_fn, &allow_vfork)) return NULL; - if ((preexec_fn != Py_None) && - (PyInterpreterState_Get() != PyInterpreterState_Main())) { + PyInterpreterState *interp = PyInterpreterState_Get(); + if ((preexec_fn != Py_None) && (interp != PyInterpreterState_Main())) { PyErr_SetString(PyExc_RuntimeError, "preexec_fn not supported within subinterpreters"); return NULL; @@ -841,13 +841,6 @@ subprocess_fork_exec(PyObject *module, PyObject *args) return NULL; } - PyInterpreterState *interp = PyInterpreterState_Get(); - if (!_PyInterpreterState_HasFeature(interp, Py_RTFLAGS_SUBPROCESS)) { - PyErr_SetString(PyExc_RuntimeError, - "subprocess not supported for isolated subinterpreters"); - return NULL; - } - /* We need to call gc.disable() when we'll be calling preexec_fn */ if (preexec_fn != Py_None) { need_to_reenable_gc = PyGC_Disable(); diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index bade0db..19ceb10 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -3231,33 +3231,42 @@ run_in_subinterp_with_config(PyObject *self, PyObject *args, PyObject *kwargs) { const char *code; int allow_fork = -1; - int allow_subprocess = -1; + int allow_exec = -1; int allow_threads = -1; + int allow_daemon_threads = -1; int r; PyThreadState *substate, *mainstate; /* only initialise 'cflags.cf_flags' to test backwards compatibility */ PyCompilerFlags cflags = {0}; static char *kwlist[] = {"code", - "allow_fork", "allow_subprocess", "allow_threads", + "allow_fork", + "allow_exec", + "allow_threads", + "allow_daemon_threads", NULL}; if (!PyArg_ParseTupleAndKeywords(args, kwargs, - "s$ppp:run_in_subinterp_with_config", kwlist, - &code, &allow_fork, &allow_subprocess, &allow_threads)) { + "s$pppp:run_in_subinterp_with_config", kwlist, + &code, &allow_fork, &allow_exec, + &allow_threads, &allow_daemon_threads)) { return NULL; } if (allow_fork < 0) { PyErr_SetString(PyExc_ValueError, "missing allow_fork"); return NULL; } - if (allow_subprocess < 0) { - PyErr_SetString(PyExc_ValueError, "missing allow_subprocess"); + if (allow_exec < 0) { + PyErr_SetString(PyExc_ValueError, "missing allow_exec"); return NULL; } if (allow_threads < 0) { PyErr_SetString(PyExc_ValueError, "missing allow_threads"); return NULL; } + if (allow_daemon_threads < 0) { + PyErr_SetString(PyExc_ValueError, "missing allow_daemon_threads"); + return NULL; + } mainstate = PyThreadState_Get(); @@ -3265,8 +3274,9 @@ run_in_subinterp_with_config(PyObject *self, PyObject *args, PyObject *kwargs) const _PyInterpreterConfig config = { .allow_fork = allow_fork, - .allow_subprocess = allow_subprocess, + .allow_exec = allow_exec, .allow_threads = allow_threads, + .allow_daemon_threads = allow_daemon_threads, }; substate = _Py_NewInterpreterFromConfig(&config); if (substate == NULL) { diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 93b3b8d..5968d4e 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1103,6 +1103,24 @@ thread_run(void *boot_raw) } static PyObject * +thread_daemon_threads_allowed(PyObject *module, PyObject *Py_UNUSED(ignored)) +{ + PyInterpreterState *interp = _PyInterpreterState_Get(); + if (interp->feature_flags & Py_RTFLAGS_DAEMON_THREADS) { + Py_RETURN_TRUE; + } + else { + Py_RETURN_FALSE; + } +} + +PyDoc_STRVAR(daemon_threads_allowed_doc, +"daemon_threads_allowed()\n\ +\n\ +Return True if daemon threads are allowed in the current interpreter,\n\ +and False otherwise.\n"); + +static PyObject * thread_PyThread_start_new_thread(PyObject *self, PyObject *fargs) { _PyRuntimeState *runtime = &_PyRuntime; @@ -1543,6 +1561,8 @@ static PyMethodDef thread_methods[] = { METH_VARARGS, start_new_doc}, {"start_new", (PyCFunction)thread_PyThread_start_new_thread, METH_VARARGS, start_new_doc}, + {"daemon_threads_allowed", (PyCFunction)thread_daemon_threads_allowed, + METH_NOARGS, daemon_threads_allowed_doc}, {"allocate_lock", thread_PyThread_allocate_lock, METH_NOARGS, allocate_doc}, {"allocate", thread_PyThread_allocate_lock, diff --git a/Modules/_winapi.c b/Modules/_winapi.c index 2a916cc..71e74d1 100644 --- a/Modules/_winapi.c +++ b/Modules/_winapi.c @@ -1089,13 +1089,6 @@ _winapi_CreateProcess_impl(PyObject *module, return NULL; } - PyInterpreterState *interp = PyInterpreterState_Get(); - if (!_PyInterpreterState_HasFeature(interp, Py_RTFLAGS_SUBPROCESS)) { - PyErr_SetString(PyExc_RuntimeError, - "subprocess not supported for isolated subinterpreters"); - return NULL; - } - ZeroMemory(&si, sizeof(si)); si.StartupInfo.cb = sizeof(si); diff --git a/Modules/_xxsubinterpretersmodule.c b/Modules/_xxsubinterpretersmodule.c index f38de57..9d979ef 100644 --- a/Modules/_xxsubinterpretersmodule.c +++ b/Modules/_xxsubinterpretersmodule.c @@ -2003,11 +2003,9 @@ interp_create(PyObject *self, PyObject *args, PyObject *kwds) // Create and initialize the new interpreter. PyThreadState *save_tstate = _PyThreadState_GET(); - const _PyInterpreterConfig config = { - .allow_fork = !isolated, - .allow_subprocess = !isolated, - .allow_threads = !isolated, - }; + const _PyInterpreterConfig config = isolated + ? (_PyInterpreterConfig)_PyInterpreterConfig_INIT + : (_PyInterpreterConfig)_PyInterpreterConfig_LEGACY_INIT; // XXX Possible GILState issues? PyThreadState *tstate = _Py_NewInterpreterFromConfig(&config); PyThreadState_Swap(save_tstate); diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index a5eb866..d863f9f 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5773,6 +5773,13 @@ os_execv_impl(PyObject *module, path_t *path, PyObject *argv) EXECV_CHAR **argvlist; Py_ssize_t argc; + PyInterpreterState *interp = _PyInterpreterState_GET(); + if (!_PyInterpreterState_HasFeature(interp, Py_RTFLAGS_EXEC)) { + PyErr_SetString(PyExc_RuntimeError, + "exec not supported for isolated subinterpreters"); + return NULL; + } + /* execv has two arguments: (path, argv), where argv is a list or tuple of strings. */ @@ -5839,6 +5846,13 @@ os_execve_impl(PyObject *module, path_t *path, PyObject *argv, PyObject *env) EXECV_CHAR **envlist; Py_ssize_t argc, envc; + PyInterpreterState *interp = _PyInterpreterState_GET(); + if (!_PyInterpreterState_HasFeature(interp, Py_RTFLAGS_EXEC)) { + PyErr_SetString(PyExc_RuntimeError, + "exec not supported for isolated subinterpreters"); + return NULL; + } + /* execve has three arguments: (path, argv, env), where argv is a list or tuple of strings and env is a dictionary like posix.environ. */ diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index d26ae74..e648492 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -615,15 +615,21 @@ static void init_interp_settings(PyInterpreterState *interp, const _PyInterpreterConfig *config) { assert(interp->feature_flags == 0); + if (config->allow_fork) { interp->feature_flags |= Py_RTFLAGS_FORK; } - if (config->allow_subprocess) { - interp->feature_flags |= Py_RTFLAGS_SUBPROCESS; + if (config->allow_exec) { + interp->feature_flags |= Py_RTFLAGS_EXEC; } + // Note that fork+exec is always allowed. + if (config->allow_threads) { interp->feature_flags |= Py_RTFLAGS_THREADS; } + if (config->allow_daemon_threads) { + interp->feature_flags |= Py_RTFLAGS_DAEMON_THREADS; + } } -- cgit v0.12