From eb9d2413cb27e7dc06a645be37a9f1b5b49d6c60 Mon Sep 17 00:00:00 2001 From: Eric Snow <ericsnowcurrently@gmail.com> Date: Mon, 24 Oct 2022 14:41:51 -0600 Subject: [PATCH 01/22] Add tests for extension module subinterpreter compatibility. --- Lib/test/test_import/__init__.py | 138 ++++++++++++++++++++++++++++++- 1 file changed, 137 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index 1e4429ed7efe13..6dbf043cfc56f5 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -21,7 +21,7 @@ from test.support import os_helper from test.support import ( STDLIB_DIR, swap_attr, swap_item, cpython_only, is_emscripten, - is_wasi) + is_wasi, run_in_subinterp_with_config) from test.support.import_helper import ( forget, make_legacy_pyc, unlink, unload, DirsOnSysPath, CleanImport) from test.support.os_helper import ( @@ -30,6 +30,14 @@ from test.support import threading_helper from test.test_importlib.util import uncache from types import ModuleType +try: + import _testsinglephase +except ImportError: + _testsinglephase = None +try: + import _testmultiphase +except ImportError: + _testmultiphase = None skip_if_dont_write_bytecode = unittest.skipIf( @@ -1392,6 +1400,134 @@ def test_unwritable_module(self): unwritable.x = 42 +class SubinterpImportTests(unittest.TestCase): + + RUN_KWARGS = dict( + allow_fork=False, + allow_exec=False, + allow_threads=True, + allow_daemon_threads=False, + ) + + @unittest.skipUnless(hasattr(os, "pipe"), "requires os.pipe()") + def pipe(self): + r, w = os.pipe() + self.addCleanup(os.close, r) + self.addCleanup(os.close, w) + if hasattr(os, 'set_blocking'): + os.set_blocking(r, False) + return (r, w) + + def import_script(self, name, fd): + return textwrap.dedent(f''' + import os, sys + try: + import {name} + except ImportError as exc: + text = 'ImportError: ' + str(exc) + else: + text = 'okay' + os.write({fd}, text.encode('utf-8')) + ''') + + def check_compatible_shared(self, name): + # Verify that the named module may be imported in a subinterpreter. + # + # The subinterpreter will be in the current process. + # The module will have already been imported in the main interpreter. + # Thus, for extension/builtin modules, the module definition will + # have been loaded already and cached globally. + + # This check should always pass for all modules if not strict. + + __import__(name) + + r, w = self.pipe() + ret = run_in_subinterp_with_config( + self.import_script(name, w), + **self.RUN_KWARGS, + ) + self.assertEqual(ret, 0) + out = os.read(r, 100) + self.assertEqual(out, b'okay') + + def check_compatible_isolated(self, name): + # Differences from check_compatible_shared(): + # * subinterpreter in a new process + # * module has never been imported before in that process + # * this tests importing the module for the first time + _, out, err = script_helper.assert_python_ok('-c', textwrap.dedent(f''' + import _testcapi, sys + assert ( + {name!r} in sys.builtin_module_names or + {name!r} not in sys.modules + ), repr({name!r}) + ret = _testcapi.run_in_subinterp_with_config( + {self.import_script(name, "sys.stdout.fileno()")!r}, + **{self.RUN_KWARGS}, + ) + assert ret == 0, ret + ''')) + self.assertEqual(err, b'') + self.assertEqual(out, b'okay') + + def check_incompatible_isolated(self, name): + # Differences from check_compatible_isolated(): + # * verify that import fails + _, out, err = script_helper.assert_python_ok('-c', textwrap.dedent(f''' + import _testcapi, sys + assert {name!r} not in sys.modules, {name!r} + ret = _testcapi.run_in_subinterp_with_config( + {self.import_script(name, "sys.stdout.fileno()")!r}, + **{self.RUN_KWARGS}, + ) + assert ret == 0, ret + ''')) + self.assertEqual(err, b'') + self.assertEqual( + out.decode('utf-8'), + f'ImportError: module {name} does not support loading in subinterpreters', + ) + + def test_builtin_compat(self): + module = 'sys' + with self.subTest(f'{module}: shared'): + self.check_compatible_shared(module) + + @cpython_only + def test_frozen_compat(self): + module = '_frozen_importlib' + if __import__(module).__spec__.origin != 'frozen': + raise unittest.SkipTest(f'{module} is unexpectedly not frozen') + with self.subTest(f'{module}: shared'): + self.check_compatible_shared(module) + + @unittest.skipIf(_testsinglephase is None, "test requires _testsinglephase module") + def test_single_init_extension_compat(self): + module = '_testsinglephase' + with self.subTest(f'{module}: shared'): + self.check_compatible_shared(module) + with self.subTest(f'{module}: isolated'): + self.check_compatible_isolated(module) + + @unittest.skipIf(_testmultiphase is None, "test requires _testmultiphase module") + def test_multi_init_extension_compat(self): + module = '_testmultiphase' + with self.subTest(f'{module}: shared'): + self.check_compatible_shared(module) + with self.subTest(f'{module}: isolated'): + self.check_compatible_isolated(module) + + def test_python_compat(self): + module = 'threading' + if __import__(module).__spec__.origin == 'frozen': + raise unittest.SkipTest(f'{module} is unexpectedly frozen') + with self.subTest(f'{module}: shared'): + self.check_compatible_shared(module) + with self.subTest(f'{module}: isolated'): + self.check_compatible_isolated(module) + + if __name__ == '__main__': # Test needs to be a package, so we can do relative imports. unittest.main() From 49b4895e6c18414ea0c4c7e43d0197d958928c1c Mon Sep 17 00:00:00 2001 From: Eric Snow <ericsnowcurrently@gmail.com> Date: Fri, 21 Oct 2022 19:11:49 -0600 Subject: [PATCH 02/22] Add _PyInterpreterConfig.check_multi_interp_extensions and Py_RTFLAGS_MULTI_INTERP_EXTENSIONS. --- Include/cpython/initconfig.h | 3 ++ Include/cpython/pystate.h | 3 ++ Lib/test/test_capi/test_misc.py | 11 ++++-- Lib/test/test_embed.py | 4 +- Lib/test/test_import/__init__.py | 68 ++++++++++++++++---------------- Lib/test/test_threading.py | 1 + Modules/_testcapimodule.c | 12 +++++- Python/pylifecycle.c | 4 ++ 8 files changed, 64 insertions(+), 42 deletions(-) diff --git a/Include/cpython/initconfig.h b/Include/cpython/initconfig.h index 6ce42b4c09502f..a070fa9ff3a038 100644 --- a/Include/cpython/initconfig.h +++ b/Include/cpython/initconfig.h @@ -248,6 +248,7 @@ typedef struct { int allow_exec; int allow_threads; int allow_daemon_threads; + int check_multi_interp_extensions; } _PyInterpreterConfig; #define _PyInterpreterConfig_INIT \ @@ -256,6 +257,7 @@ typedef struct { .allow_exec = 0, \ .allow_threads = 1, \ .allow_daemon_threads = 0, \ + .check_multi_interp_extensions = 1, \ } #define _PyInterpreterConfig_LEGACY_INIT \ @@ -264,6 +266,7 @@ typedef struct { .allow_exec = 1, \ .allow_threads = 1, \ .allow_daemon_threads = 1, \ + .check_multi_interp_extensions = 0, \ } /* --- Helper functions --------------------------------------- */ diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h index 0117c23f518cdb..b322dc91ede68a 100644 --- a/Include/cpython/pystate.h +++ b/Include/cpython/pystate.h @@ -11,6 +11,9 @@ 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). */ +/* Set if import should check a module for subinterpreter support. */ +#define Py_RTFLAGS_MULTI_INTERP_EXTENSIONS (1UL << 8) + /* Set if threads are allowed. */ #define Py_RTFLAGS_THREADS (1UL << 10) diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index dace37c362e569..1cd0c057ce151f 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -1297,17 +1297,20 @@ def test_configured_settings(self): """ import json + EXTENSIONS = 1<<8 THREADS = 1<<10 DAEMON_THREADS = 1<<11 FORK = 1<<15 EXEC = 1<<16 - features = ['fork', 'exec', 'threads', 'daemon_threads'] + features = ['fork', 'exec', 'threads', 'daemon_threads', 'extensions'] kwlist = [f'allow_{n}' for n in features] + kwlist[-1] = 'check_multi_interp_extensions' for config, expected in { - (True, True, True, True): FORK | EXEC | THREADS | DAEMON_THREADS, - (False, False, False, False): 0, - (False, False, True, False): THREADS, + (True, True, True, True, True): + FORK | EXEC | THREADS | DAEMON_THREADS | EXTENSIONS, + (False, False, False, False, False): 0, + (False, False, True, False, True): THREADS | EXTENSIONS, }.items(): kwargs = dict(zip(kwlist, config)) expected = { diff --git a/Lib/test/test_embed.py b/Lib/test/test_embed.py index 2dda7ccf7bf80c..cf61e8f8e6cfb6 100644 --- a/Lib/test/test_embed.py +++ b/Lib/test/test_embed.py @@ -1652,13 +1652,15 @@ def test_init_use_frozen_modules(self): api=API_PYTHON, env=env) def test_init_main_interpreter_settings(self): + EXTENSIONS = 1<<8 THREADS = 1<<10 DAEMON_THREADS = 1<<11 FORK = 1<<15 EXEC = 1<<16 expected = { # All optional features should be enabled. - 'feature_flags': FORK | EXEC | THREADS | DAEMON_THREADS, + 'feature_flags': + FORK | EXEC | THREADS | DAEMON_THREADS, } out, err = self.run_embedded_interpreter( 'test_init_main_interpreter_settings', diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index 6dbf043cfc56f5..4030d3cb23a502 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -1430,13 +1430,17 @@ def import_script(self, name, fd): os.write({fd}, text.encode('utf-8')) ''') - def check_compatible_shared(self, name): + def check_compatible_shared(self, name, *, strict=False): # Verify that the named module may be imported in a subinterpreter. # # The subinterpreter will be in the current process. # The module will have already been imported in the main interpreter. # Thus, for extension/builtin modules, the module definition will # have been loaded already and cached globally. + # + # "strict" determines whether or not the interpreter will be + # configured to check for modules that are not compatible + # with use in multiple interpreters. # This check should always pass for all modules if not strict. @@ -1446,12 +1450,13 @@ def check_compatible_shared(self, name): ret = run_in_subinterp_with_config( self.import_script(name, w), **self.RUN_KWARGS, + check_multi_interp_extensions=strict, ) self.assertEqual(ret, 0) out = os.read(r, 100) self.assertEqual(out, b'okay') - def check_compatible_isolated(self, name): + def check_compatible_isolated(self, name, *, strict=False): # Differences from check_compatible_shared(): # * subinterpreter in a new process # * module has never been imported before in that process @@ -1465,67 +1470,60 @@ def check_compatible_isolated(self, name): ret = _testcapi.run_in_subinterp_with_config( {self.import_script(name, "sys.stdout.fileno()")!r}, **{self.RUN_KWARGS}, + check_multi_interp_extensions={strict}, ) assert ret == 0, ret ''')) self.assertEqual(err, b'') self.assertEqual(out, b'okay') - def check_incompatible_isolated(self, name): - # Differences from check_compatible_isolated(): - # * verify that import fails - _, out, err = script_helper.assert_python_ok('-c', textwrap.dedent(f''' - import _testcapi, sys - assert {name!r} not in sys.modules, {name!r} - ret = _testcapi.run_in_subinterp_with_config( - {self.import_script(name, "sys.stdout.fileno()")!r}, - **{self.RUN_KWARGS}, - ) - assert ret == 0, ret - ''')) - self.assertEqual(err, b'') - self.assertEqual( - out.decode('utf-8'), - f'ImportError: module {name} does not support loading in subinterpreters', - ) - def test_builtin_compat(self): module = 'sys' - with self.subTest(f'{module}: shared'): - self.check_compatible_shared(module) + with self.subTest(f'{module}: not strict'): + self.check_compatible_shared(module, strict=False) + with self.subTest(f'{module}: strict, shared'): + self.check_compatible_shared(module, strict=True) @cpython_only def test_frozen_compat(self): module = '_frozen_importlib' if __import__(module).__spec__.origin != 'frozen': raise unittest.SkipTest(f'{module} is unexpectedly not frozen') - with self.subTest(f'{module}: shared'): - self.check_compatible_shared(module) + with self.subTest(f'{module}: not strict'): + self.check_compatible_shared(module, strict=False) + with self.subTest(f'{module}: strict, shared'): + self.check_compatible_shared(module, strict=True) - @unittest.skipIf(_testsinglephase is None, "test requires _testsinglephase module") + @unittest.skipIf(_testsinglephase is None, "test requires _testsinglphase module") def test_single_init_extension_compat(self): module = '_testsinglephase' - with self.subTest(f'{module}: shared'): + with self.subTest(f'{module}: not strict'): + self.check_compatible_shared(module, strict=False) + with self.subTest(f'{module}: strict, shared'): self.check_compatible_shared(module) - with self.subTest(f'{module}: isolated'): + with self.subTest(f'{module}: strict, isolated'): self.check_compatible_isolated(module) @unittest.skipIf(_testmultiphase is None, "test requires _testmultiphase module") def test_multi_init_extension_compat(self): module = '_testmultiphase' - with self.subTest(f'{module}: shared'): - self.check_compatible_shared(module) - with self.subTest(f'{module}: isolated'): - self.check_compatible_isolated(module) + with self.subTest(f'{module}: not strict'): + self.check_compatible_shared(module, strict=False) + with self.subTest(f'{module}: strict, shared'): + self.check_compatible_shared(module, strict=True) + with self.subTest(f'{module}: strict, isolated'): + self.check_compatible_isolated(module, strict=True) def test_python_compat(self): module = 'threading' if __import__(module).__spec__.origin == 'frozen': raise unittest.SkipTest(f'{module} is unexpectedly frozen') - with self.subTest(f'{module}: shared'): - self.check_compatible_shared(module) - with self.subTest(f'{module}: isolated'): - self.check_compatible_isolated(module) + with self.subTest(f'{module}: not strict'): + self.check_compatible_shared(module, strict=False) + with self.subTest(f'{module}: strict, shared'): + self.check_compatible_shared(module, strict=True) + with self.subTest(f'{module}: strict, isolated'): + self.check_compatible_isolated(module, strict=True) if __name__ == '__main__': diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index 31bf46311a80dc..7fea2d38673eff 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1347,6 +1347,7 @@ def func(): allow_exec=True, allow_threads={allowed}, allow_daemon_threads={daemon_allowed}, + check_multi_interp_extensions=False, ) """) with test.support.SuppressCrashReport(): diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 486988b4e34cdd..e1eba0eef9097f 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -1578,6 +1578,7 @@ run_in_subinterp_with_config(PyObject *self, PyObject *args, PyObject *kwargs) int allow_exec = -1; int allow_threads = -1; int allow_daemon_threads = -1; + int check_multi_interp_extensions = -1; int r; PyThreadState *substate, *mainstate; /* only initialise 'cflags.cf_flags' to test backwards compatibility */ @@ -1588,11 +1589,13 @@ run_in_subinterp_with_config(PyObject *self, PyObject *args, PyObject *kwargs) "allow_exec", "allow_threads", "allow_daemon_threads", + "check_multi_interp_extensions", NULL}; if (!PyArg_ParseTupleAndKeywords(args, kwargs, - "s$pppp:run_in_subinterp_with_config", kwlist, + "s$ppppp:run_in_subinterp_with_config", kwlist, &code, &allow_fork, &allow_exec, - &allow_threads, &allow_daemon_threads)) { + &allow_threads, &allow_daemon_threads, + &check_multi_interp_extensions)) { return NULL; } if (allow_fork < 0) { @@ -1611,6 +1614,10 @@ run_in_subinterp_with_config(PyObject *self, PyObject *args, PyObject *kwargs) PyErr_SetString(PyExc_ValueError, "missing allow_daemon_threads"); return NULL; } + if (check_multi_interp_extensions < 0) { + PyErr_SetString(PyExc_ValueError, "missing check_multi_interp_extensions"); + return NULL; + } mainstate = PyThreadState_Get(); @@ -1621,6 +1628,7 @@ run_in_subinterp_with_config(PyObject *self, PyObject *args, PyObject *kwargs) .allow_exec = allow_exec, .allow_threads = allow_threads, .allow_daemon_threads = allow_daemon_threads, + .check_multi_interp_extensions = check_multi_interp_extensions, }; substate = _Py_NewInterpreterFromConfig(&config); if (substate == NULL) { diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 1cb0e4d747e10a..5024c1cde711a6 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -642,6 +642,10 @@ init_interp_settings(PyInterpreterState *interp, const _PyInterpreterConfig *con if (config->allow_daemon_threads) { interp->feature_flags |= Py_RTFLAGS_DAEMON_THREADS; } + + if (config->check_multi_interp_extensions) { + interp->feature_flags |= Py_RTFLAGS_MULTI_INTERP_EXTENSIONS; + } } From ed505a67ad545c4f61e1768ac6fa309f8794faa6 Mon Sep 17 00:00:00 2001 From: Eric Snow <ericsnowcurrently@gmail.com> Date: Mon, 24 Oct 2022 14:41:51 -0600 Subject: [PATCH 03/22] Add _PyImport_CheckSubinterpIncompatibleExtensionAllowed(). --- Include/internal/pycore_import.h | 3 +++ Python/import.c | 15 +++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/Include/internal/pycore_import.h b/Include/internal/pycore_import.h index 9036dff6725330..d486bd8b03420c 100644 --- a/Include/internal/pycore_import.h +++ b/Include/internal/pycore_import.h @@ -52,6 +52,9 @@ PyAPI_DATA(const struct _frozen *) _PyImport_FrozenStdlib; PyAPI_DATA(const struct _frozen *) _PyImport_FrozenTest; extern const struct _module_alias * _PyImport_FrozenAliases; +PyAPI_FUNC(int) _PyImport_CheckSubinterpIncompatibleExtensionAllowed( + const char *name); + #ifdef __cplusplus } #endif diff --git a/Python/import.c b/Python/import.c index da6c15c5fd4144..9b4552006bc497 100644 --- a/Python/import.c +++ b/Python/import.c @@ -2395,6 +2395,21 @@ exec_builtin_or_dynamic(PyObject *mod) { return PyModule_ExecDef(mod, def); } +int +_PyImport_CheckSubinterpIncompatibleExtensionAllowed(const char *name) +{ + PyInterpreterState *interp = _PyInterpreterState_Get(); + if (_PyInterpreterState_HasFeature( + interp, Py_RTFLAGS_MULTI_INTERP_EXTENSIONS)) { + assert(!_Py_IsMainInterpreter(interp)); + PyErr_Format(PyExc_ImportError, + "module %s does not support loading in subinterpreters", + name); + return -1; + } + return 0; +} + #ifdef HAVE_DYNAMIC_LOADING /*[clinic input] From 64e1dc88e3789d70764154618a3835a2936689f9 Mon Sep 17 00:00:00 2001 From: Eric Snow <ericsnowcurrently@gmail.com> Date: Mon, 24 Oct 2022 14:41:51 -0600 Subject: [PATCH 04/22] Raise ImportError in subinterpreters for incompatible single-phase init extensions. --- Lib/test/test_import/__init__.py | 40 ++++++++++++++++++++++++++++++-- Python/import.c | 25 +++++++++++++------- Python/importdl.c | 5 ++++ 3 files changed, 59 insertions(+), 11 deletions(-) diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index 4030d3cb23a502..ac5eff7cb8a8af 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -1456,6 +1456,22 @@ def check_compatible_shared(self, name, *, strict=False): out = os.read(r, 100) self.assertEqual(out, b'okay') + def check_incompatible_shared(self, name): + # Differences from check_compatible_shared(): + # * verify that import fails + # * "strict" is always True + __import__(name) + + r, w = self.pipe() + ret = run_in_subinterp_with_config( + self.import_script(name, w), + **self.RUN_KWARGS, + check_multi_interp_extensions=True, + ) + self.assertEqual(ret, 0) + out = os.read(r, 100).decode('utf-8') + self.assertEqual(out, f'ImportError: module {name} does not support loading in subinterpreters') + def check_compatible_isolated(self, name, *, strict=False): # Differences from check_compatible_shared(): # * subinterpreter in a new process @@ -1477,6 +1493,26 @@ def check_compatible_isolated(self, name, *, strict=False): self.assertEqual(err, b'') self.assertEqual(out, b'okay') + def check_incompatible_isolated(self, name): + # Differences from check_compatible_isolated(): + # * verify that import fails + # * "strict" is always True + _, out, err = script_helper.assert_python_ok('-c', textwrap.dedent(f''' + import _testcapi, sys + assert {name!r} not in sys.modules, {name!r} + ret = _testcapi.run_in_subinterp_with_config( + {self.import_script(name, "sys.stdout.fileno()")!r}, + **{self.RUN_KWARGS}, + check_multi_interp_extensions=True, + ) + assert ret == 0, ret + ''')) + self.assertEqual(err, b'') + self.assertEqual( + out.decode('utf-8'), + f'ImportError: module {name} does not support loading in subinterpreters', + ) + def test_builtin_compat(self): module = 'sys' with self.subTest(f'{module}: not strict'): @@ -1500,9 +1536,9 @@ def test_single_init_extension_compat(self): with self.subTest(f'{module}: not strict'): self.check_compatible_shared(module, strict=False) with self.subTest(f'{module}: strict, shared'): - self.check_compatible_shared(module) + self.check_incompatible_shared(module) with self.subTest(f'{module}: strict, isolated'): - self.check_compatible_isolated(module) + self.check_incompatible_isolated(module) @unittest.skipIf(_testmultiphase is None, "test requires _testmultiphase module") def test_multi_init_extension_compat(self): diff --git a/Python/import.c b/Python/import.c index 9b4552006bc497..41014f4b5e2cf9 100644 --- a/Python/import.c +++ b/Python/import.c @@ -2442,18 +2442,23 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) PyThreadState *tstate = _PyThreadState_GET(); mod = import_find_extension(tstate, name, path); - if (mod != NULL || PyErr_Occurred()) { - Py_DECREF(name); - Py_DECREF(path); - return mod; + if (mod != NULL) { + const char *name_buf = PyUnicode_AsUTF8(name); + assert(name_buf != NULL); + if (_PyImport_CheckSubinterpIncompatibleExtensionAllowed(name_buf) < 0) { + Py_DECREF(mod); + mod = NULL; + } + goto finally; + } + else if (PyErr_Occurred()) { + goto finally; } if (file != NULL) { fp = _Py_fopen_obj(path, "r"); if (fp == NULL) { - Py_DECREF(name); - Py_DECREF(path); - return NULL; + goto finally; } } else @@ -2461,10 +2466,12 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) mod = _PyImport_LoadDynamicModuleWithSpec(spec, fp); - Py_DECREF(name); - Py_DECREF(path); if (fp) fclose(fp); + +finally: + Py_DECREF(name); + Py_DECREF(path); return mod; } diff --git a/Python/importdl.c b/Python/importdl.c index 91fa06f49c2897..eacaea55d61c6f 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -3,6 +3,7 @@ #include "Python.h" #include "pycore_call.h" +#include "pycore_import.h" #include "pycore_pystate.h" #include "pycore_runtime.h" @@ -206,6 +207,10 @@ _PyImport_LoadDynamicModuleWithSpec(PyObject *spec, FILE *fp) /* Fall back to single-phase init mechanism */ + if (_PyImport_CheckSubinterpIncompatibleExtensionAllowed(name_buf) < 0) { + goto error; + } + if (hook_prefix == nonascii_prefix) { /* don't allow legacy init for non-ASCII module names */ PyErr_Format( From 72ab9b6eeed6fab3e75446b5dbfeb397c8cc4d07 Mon Sep 17 00:00:00 2001 From: Eric Snow <ericsnowcurrently@gmail.com> Date: Wed, 2 Nov 2022 20:24:01 -0600 Subject: [PATCH 05/22] Add a NEWS entry. --- .../2022-11-02-20-23-47.gh-issue-98627.VJkdRM.rst | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2022-11-02-20-23-47.gh-issue-98627.VJkdRM.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-11-02-20-23-47.gh-issue-98627.VJkdRM.rst b/Misc/NEWS.d/next/Core and Builtins/2022-11-02-20-23-47.gh-issue-98627.VJkdRM.rst new file mode 100644 index 00000000000000..3d2d6f6eb0c41f --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2022-11-02-20-23-47.gh-issue-98627.VJkdRM.rst @@ -0,0 +1,5 @@ +When an interpreter is configured to check (and only then), importing an +extension module will now fail when the extension does not support multiple +interpreters (i.e. doesn't implement PEP 489 multi-phase init). This does +not apply to the main interpreter, nor to subinterpreters created with +``Py_NewInterpreter()``. From 9c24b347c5a7168be3e9aa210d885ffb183cc97d Mon Sep 17 00:00:00 2001 From: Eric Snow <ericsnowcurrently@gmail.com> Date: Mon, 21 Nov 2022 11:00:59 -0700 Subject: [PATCH 06/22] Add PyInterpreterState.override_multi_interp_extensions_check. --- Include/internal/pycore_interp.h | 1 + Python/import.c | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index 0e3d46852f2e6d..2c8eb9ad7b147c 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -123,6 +123,7 @@ struct _is { // override for config->use_frozen_modules (for tests) // (-1: "off", 1: "on", 0: no override) int override_frozen_modules; + int override_multi_interp_extensions_check; PyObject *codec_search_path; PyObject *codec_search_cache; diff --git a/Python/import.c b/Python/import.c index 41014f4b5e2cf9..ee2fb5df4f2335 100644 --- a/Python/import.c +++ b/Python/import.c @@ -2400,7 +2400,9 @@ _PyImport_CheckSubinterpIncompatibleExtensionAllowed(const char *name) { PyInterpreterState *interp = _PyInterpreterState_Get(); if (_PyInterpreterState_HasFeature( - interp, Py_RTFLAGS_MULTI_INTERP_EXTENSIONS)) { + interp, Py_RTFLAGS_MULTI_INTERP_EXTENSIONS) && + !interp->override_multi_interp_extensions_check + ) { assert(!_Py_IsMainInterpreter(interp)); PyErr_Format(PyExc_ImportError, "module %s does not support loading in subinterpreters", From 3c084eb64814eefc8035b0a40b2e23d7004b541f Mon Sep 17 00:00:00 2001 From: Eric Snow <ericsnowcurrently@gmail.com> Date: Mon, 21 Nov 2022 11:01:17 -0700 Subject: [PATCH 07/22] Add check_multi_interp_extensions(). --- Python/import.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/Python/import.c b/Python/import.c index ee2fb5df4f2335..8f1860a04c3fdb 100644 --- a/Python/import.c +++ b/Python/import.c @@ -2395,14 +2395,28 @@ exec_builtin_or_dynamic(PyObject *mod) { return PyModule_ExecDef(mod, def); } +static bool +check_multi_interp_extensions(PyInterpreterState *interp) +{ + int override = interp->override_multi_interp_extensions_check; + if (override < 0) { + return false; + } + else if (override > 0) { + return true; + } + else if (_PyInterpreterState_HasFeature( + interp, Py_RTFLAGS_MULTI_INTERP_EXTENSIONS)) { + return true; + } + return false; +} + int _PyImport_CheckSubinterpIncompatibleExtensionAllowed(const char *name) { PyInterpreterState *interp = _PyInterpreterState_Get(); - if (_PyInterpreterState_HasFeature( - interp, Py_RTFLAGS_MULTI_INTERP_EXTENSIONS) && - !interp->override_multi_interp_extensions_check - ) { + if (check_multi_interp_extensions(interp)) { assert(!_Py_IsMainInterpreter(interp)); PyErr_Format(PyExc_ImportError, "module %s does not support loading in subinterpreters", From a3d3a655c9215c3ffc60d2cbfa42c572e323307a Mon Sep 17 00:00:00 2001 From: Eric Snow <ericsnowcurrently@gmail.com> Date: Mon, 21 Nov 2022 11:18:34 -0700 Subject: [PATCH 08/22] Add _imp._override_multi_interp_extensions_check(). --- Python/clinic/import.c.h | 33 ++++++++++++++++++++++++++++++++- Python/import.c | 22 ++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/Python/clinic/import.c.h b/Python/clinic/import.c.h index 819fb1c75c15c3..cb74be6a422124 100644 --- a/Python/clinic/import.c.h +++ b/Python/clinic/import.c.h @@ -442,6 +442,37 @@ _imp__override_frozen_modules_for_tests(PyObject *module, PyObject *arg) return return_value; } +PyDoc_STRVAR(_imp__override_multi_interp_extensions_check__doc__, +"_override_multi_interp_extensions_check($module, override, /)\n" +"--\n" +"\n" +"(internal-only) Override PyInterpreterConfig.check_multi_interp_extensions.\n" +"\n" +"(-1: \"never\", 1: \"always\", 0: no override)"); + +#define _IMP__OVERRIDE_MULTI_INTERP_EXTENSIONS_CHECK_METHODDEF \ + {"_override_multi_interp_extensions_check", (PyCFunction)_imp__override_multi_interp_extensions_check, METH_O, _imp__override_multi_interp_extensions_check__doc__}, + +static PyObject * +_imp__override_multi_interp_extensions_check_impl(PyObject *module, + int override); + +static PyObject * +_imp__override_multi_interp_extensions_check(PyObject *module, PyObject *arg) +{ + PyObject *return_value = NULL; + int override; + + override = _PyLong_AsInt(arg); + if (override == -1 && PyErr_Occurred()) { + goto exit; + } + return_value = _imp__override_multi_interp_extensions_check_impl(module, override); + +exit: + return return_value; +} + #if defined(HAVE_DYNAMIC_LOADING) PyDoc_STRVAR(_imp_create_dynamic__doc__, @@ -617,4 +648,4 @@ _imp_source_hash(PyObject *module, PyObject *const *args, Py_ssize_t nargs, PyOb #ifndef _IMP_EXEC_DYNAMIC_METHODDEF #define _IMP_EXEC_DYNAMIC_METHODDEF #endif /* !defined(_IMP_EXEC_DYNAMIC_METHODDEF) */ -/*[clinic end generated code: output=806352838c3f7008 input=a9049054013a1b77]*/ +/*[clinic end generated code: output=b18d46e0036eff49 input=a9049054013a1b77]*/ diff --git a/Python/import.c b/Python/import.c index 8f1860a04c3fdb..463473777aae90 100644 --- a/Python/import.c +++ b/Python/import.c @@ -2371,6 +2371,27 @@ _imp__override_frozen_modules_for_tests_impl(PyObject *module, int override) Py_RETURN_NONE; } +/*[clinic input] +_imp._override_multi_interp_extensions_check + + override: int + / + +(internal-only) Override PyInterpreterConfig.check_multi_interp_extensions. + +(-1: "never", 1: "always", 0: no override) +[clinic start generated code]*/ + +static PyObject * +_imp__override_multi_interp_extensions_check_impl(PyObject *module, + int override) +/*[clinic end generated code: output=3ff043af52bbf280 input=e086a2ea181f92ae]*/ +{ + PyInterpreterState *interp = _PyInterpreterState_GET(); + interp->override_multi_interp_extensions_check = override; + Py_RETURN_NONE; +} + /* Common implementation for _imp.exec_dynamic and _imp.exec_builtin */ static int exec_builtin_or_dynamic(PyObject *mod) { @@ -2572,6 +2593,7 @@ static PyMethodDef imp_methods[] = { _IMP_IS_FROZEN_METHODDEF _IMP__FROZEN_MODULE_NAMES_METHODDEF _IMP__OVERRIDE_FROZEN_MODULES_FOR_TESTS_METHODDEF + _IMP__OVERRIDE_MULTI_INTERP_EXTENSIONS_CHECK_METHODDEF _IMP_CREATE_DYNAMIC_METHODDEF _IMP_EXEC_DYNAMIC_METHODDEF _IMP_EXEC_BUILTIN_METHODDEF From ad3fe36f48bf19a04d333e6f3ccb437d04984980 Mon Sep 17 00:00:00 2001 From: Eric Snow <ericsnowcurrently@gmail.com> Date: Mon, 21 Nov 2022 11:18:58 -0700 Subject: [PATCH 09/22] Add test.support.import_helper.multi_interp_extensions_check(). --- Lib/test/support/import_helper.py | 18 ++++++++++++++++++ Python/import.c | 3 ++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/Lib/test/support/import_helper.py b/Lib/test/support/import_helper.py index 63a8a7952db7a6..772c0987c2ebef 100644 --- a/Lib/test/support/import_helper.py +++ b/Lib/test/support/import_helper.py @@ -105,6 +105,24 @@ def frozen_modules(enabled=True): _imp._override_frozen_modules_for_tests(0) +@contextlib.contextmanager +def multi_interp_extensions_check(enabled=True): + """Force legacy modules to be allowed in subinterpreters (or not). + + ("legacy" == single-phase init) + + This only applies to modules that haven't been imported yet. + It overrides the PyInterpreterConfig.check_multi_interp_extensions + setting (see support.run_in_subinterp_with_config() and + _xxsubinterpreters.create()). + """ + old = _imp._override_multi_interp_extensions_check(1 if enabled else -1) + try: + yield + finally: + _imp._override_multi_interp_extensions_check(old) + + def import_fresh_module(name, fresh=(), blocked=(), *, deprecated=False, usefrozen=False, diff --git a/Python/import.c b/Python/import.c index 463473777aae90..9ab5f6d888039b 100644 --- a/Python/import.c +++ b/Python/import.c @@ -2388,8 +2388,9 @@ _imp__override_multi_interp_extensions_check_impl(PyObject *module, /*[clinic end generated code: output=3ff043af52bbf280 input=e086a2ea181f92ae]*/ { PyInterpreterState *interp = _PyInterpreterState_GET(); + int oldvalue = interp->override_multi_interp_extensions_check; interp->override_multi_interp_extensions_check = override; - Py_RETURN_NONE; + return PyLong_FromLong(oldvalue); } /* Common implementation for _imp.exec_dynamic and _imp.exec_builtin */ From 1defec3b30c7913e0b3864060c3056150bbcfd88 Mon Sep 17 00:00:00 2001 From: Eric Snow <ericsnowcurrently@gmail.com> Date: Mon, 21 Nov 2022 14:19:01 -0700 Subject: [PATCH 10/22] Add a test. --- Lib/test/test_capi/check_config.py | 53 ++++++++++++++++++ Lib/test/test_capi/test_misc.py | 86 +++++++++++++++++++++++++++++- Python/import.c | 6 +++ 3 files changed, 144 insertions(+), 1 deletion(-) create mode 100644 Lib/test/test_capi/check_config.py diff --git a/Lib/test/test_capi/check_config.py b/Lib/test/test_capi/check_config.py new file mode 100644 index 00000000000000..c8e4f348f51307 --- /dev/null +++ b/Lib/test/test_capi/check_config.py @@ -0,0 +1,53 @@ +# This script is used by test_misc. + +import _imp +import _testinternalcapi +import json +import os +import sys + + +def import_singlephase(): + assert '_singlephase' not in sys.modules + try: + import _singlephase + except ImportError: + return True + else: + del sys.modules['_singlephase'] + return False + + +def check(override): + settings_before = _testinternalcapi.get_interp_settings() + enabled_initial = import_singlephase() + + override_initial = _imp._override_multi_interp_extensions_check(override) + settings_after = _testinternalcapi.get_interp_settings() + enabled_after = import_singlephase() + + override_actual = _imp._override_multi_interp_extensions_check(override_initial) + settings_final = _testinternalcapi.get_interp_settings() + override_noop = _imp._override_multi_interp_extensions_check(override_initial) + settings_noop = _testinternalcapi.get_interp_settings() + enabled_restored = import_singlephase() + return { + 'settings_before': settings_before, + 'enabled_initial': enabled_initial, + 'override_initial': override_initial, + 'settings_after': settings_after, + 'enabled_after': enabled_after, + 'override_actual': override_actual, + 'settings_final': settings_final, + 'override_noop': override_noop, + 'settings_noop': settings_noop, + 'enabled_restored': enabled_restored, + } + + +def run_check(override, outfd): + with os.fdopen(outfd, 'w') as outfile: + sys.stdout = outfile + sys.stderr = outfile + results = check(override) + json.dump(results, outfile) diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index 1cd0c057ce151f..980d559f953040 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -31,6 +31,10 @@ import _testmultiphase except ImportError: _testmultiphase = None +try: + import _testsinglephase +except ImportError: + _testsinglephase = None # Skip this test if the _testcapi module isn't available. _testcapi = import_helper.import_module('_testcapi') @@ -1325,12 +1329,92 @@ def test_configured_settings(self): json.dump(settings, stdin) ''') with os.fdopen(r) as stdout: - support.run_in_subinterp_with_config(script, **kwargs) + ret = support.run_in_subinterp_with_config(script, **kwargs) + self.assertEqual(ret, 0) out = stdout.read() settings = json.loads(out) self.assertEqual(settings, expected) + @unittest.skipIf(_testsinglephase is None, "test requires _testsinglphase module") + @unittest.skipUnless(hasattr(os, "pipe"), "requires os.pipe()") + def test_overridden_setting_extensions_subinterp_check(self): + """ + PyInterpreterConfig.check_multi_interp_extensions can be overridden + with PyInterpreterState.override_multi_interp_extensions_check. + This verifies that the override works but does not modify + the underlying setting. + """ + import json + + EXTENSIONS = 1<<8 + THREADS = 1<<10 + DAEMON_THREADS = 1<<11 + FORK = 1<<15 + EXEC = 1<<16 + BASE_FLAGS = FORK | EXEC | THREADS | DAEMON_THREADS + base_kwargs = { + 'allow_fork': True, + 'allow_exec': True, + 'allow_threads': True, + 'allow_daemon_threads': True, + } + + def check(enabled, override): + kwargs = dict( + base_kwargs, + check_multi_interp_extensions=enabled, + ) + flags = BASE_FLAGS | EXTENSIONS if enabled else BASE_FLAGS + settings = { + 'feature_flags': flags, + } + if override == 0: + enabled_after = enabled + else: + enabled_after = (override > 0) + expected = { + 'settings_before': settings, + 'enabled_initial': enabled, + 'override_initial': 0, + 'settings_after': settings, + 'enabled_after': enabled_after, + 'override_actual': override, + 'settings_final': settings, + 'override_noop': 0, + 'settings_noop': settings, + 'enabled_restored': enabled, + } + + r, w = os.pipe() + script = textwrap.dedent(f''' + from test.test_capi.check_config import run_check + run_check({override}, {w}) + ''') + with os.fdopen(r) as stdout: + ret = support.run_in_subinterp_with_config(script, **kwargs) + self.assertEqual(ret, 0) + out = stdout.read() + results = json.loads(out) + + self.assertEqual(results, expected) + + # setting: check disabled + with self.subTest('config disabled, override disabled'): + check(False, -1) + with self.subTest('config disabled, override cleared'): + check(False, 0) + with self.subTest('config disabled, override enabled'): + check(False, 1) + + # setting: check enabled + with self.subTest('config enabled, override disabled'): + check(True, -1) + with self.subTest('config enabled, override cleared'): + check(True, 0) + with self.subTest('config enabled, override enabled'): + check(True, 1) + def test_mutate_exception(self): """ Exceptions saved in global module state get shared between diff --git a/Python/import.c b/Python/import.c index 9ab5f6d888039b..f2b385cda214fa 100644 --- a/Python/import.c +++ b/Python/import.c @@ -2388,6 +2388,12 @@ _imp__override_multi_interp_extensions_check_impl(PyObject *module, /*[clinic end generated code: output=3ff043af52bbf280 input=e086a2ea181f92ae]*/ { PyInterpreterState *interp = _PyInterpreterState_GET(); + if (_Py_IsMainInterpreter(interp)) { + PyErr_SetString(PyExc_RuntimeError, + "_imp._override_multi_interp_extensions_check() " + "cannot be used in the main interpreter"); + return NULL; + } int oldvalue = interp->override_multi_interp_extensions_check; interp->override_multi_interp_extensions_check = override; return PyLong_FromLong(oldvalue); From 3c3ed2b4de68c714940ec6bdbb3ec90493567c3c Mon Sep 17 00:00:00 2001 From: Eric Snow <ericsnowcurrently@gmail.com> Date: Thu, 12 Jan 2023 12:42:47 -0700 Subject: [PATCH 11/22] Fix a typo. --- Lib/test/test_capi/test_misc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index 980d559f953040..11153a9bedab12 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -1336,7 +1336,7 @@ def test_configured_settings(self): self.assertEqual(settings, expected) - @unittest.skipIf(_testsinglephase is None, "test requires _testsinglphase module") + @unittest.skipIf(_testsinglephase is None, "test requires _testsinglephase module") @unittest.skipUnless(hasattr(os, "pipe"), "requires os.pipe()") def test_overridden_setting_extensions_subinterp_check(self): """ From de6c791622754e98d2a9fa1b559bf5ce0f35cb27 Mon Sep 17 00:00:00 2001 From: Eric Snow <ericsnowcurrently@gmail.com> Date: Thu, 12 Jan 2023 12:53:48 -0700 Subject: [PATCH 12/22] Add some extra diagnostic info. --- Lib/test/test_capi/check_config.py | 1 + Lib/test/test_capi/test_misc.py | 1 + 2 files changed, 2 insertions(+) diff --git a/Lib/test/test_capi/check_config.py b/Lib/test/test_capi/check_config.py index c8e4f348f51307..b3b394779d7b1d 100644 --- a/Lib/test/test_capi/check_config.py +++ b/Lib/test/test_capi/check_config.py @@ -32,6 +32,7 @@ def check(override): settings_noop = _testinternalcapi.get_interp_settings() enabled_restored = import_singlephase() return { + 'override_requested': override, 'settings_before': settings_before, 'enabled_initial': enabled_initial, 'override_initial': override_initial, diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index 11153a9bedab12..7019a3a967de06 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -1374,6 +1374,7 @@ def check(enabled, override): else: enabled_after = (override > 0) expected = { + 'override_requested': override, 'settings_before': settings, 'enabled_initial': enabled, 'override_initial': 0, From af114f25014ac54f3f0d339aaeeb5e53e97a165e Mon Sep 17 00:00:00 2001 From: Eric Snow <ericsnowcurrently@gmail.com> Date: Thu, 12 Jan 2023 13:08:23 -0700 Subject: [PATCH 13/22] Clarify various names (e.g. data keys) in the test. --- Lib/test/test_capi/check_config.py | 52 +++++++++++++++++++----------- Lib/test/test_capi/test_misc.py | 24 +++++++------- 2 files changed, 46 insertions(+), 30 deletions(-) diff --git a/Lib/test/test_capi/check_config.py b/Lib/test/test_capi/check_config.py index b3b394779d7b1d..1ae7ce63b610ea 100644 --- a/Lib/test/test_capi/check_config.py +++ b/Lib/test/test_capi/check_config.py @@ -18,37 +18,51 @@ def import_singlephase(): return False -def check(override): - settings_before = _testinternalcapi.get_interp_settings() - enabled_initial = import_singlephase() +def check_singlephase(override): + # Check using the default setting. + settings_initial = _testinternalcapi.get_interp_settings() + allowed_initial = import_singlephase() + assert(_testinternalcapi.get_interp_settings() == settings_initial) + # Apply the override and check. override_initial = _imp._override_multi_interp_extensions_check(override) settings_after = _testinternalcapi.get_interp_settings() - enabled_after = import_singlephase() + allowed_after = import_singlephase() - override_actual = _imp._override_multi_interp_extensions_check(override_initial) - settings_final = _testinternalcapi.get_interp_settings() - override_noop = _imp._override_multi_interp_extensions_check(override_initial) + # Apply the override again and check. + override_after = _imp._override_multi_interp_extensions_check(override) settings_noop = _testinternalcapi.get_interp_settings() - enabled_restored = import_singlephase() + allowed_noop = import_singlephase() + + # Restore the original setting and check. + override_noop = _imp._override_multi_interp_extensions_check(override_initial) + settings_restored = _testinternalcapi.get_interp_settings() + allowed_restored = import_singlephase() + + # Restore the original setting again. + override_restored = _imp._override_multi_interp_extensions_check(override_initial) + assert(_testinternalcapi.get_interp_settings() == settings_restored) + return { - 'override_requested': override, - 'settings_before': settings_before, - 'enabled_initial': enabled_initial, - 'override_initial': override_initial, - 'settings_after': settings_after, - 'enabled_after': enabled_after, - 'override_actual': override_actual, - 'settings_final': settings_final, + 'requested': override, + 'override__initial': override_initial, + 'override_after': override_after, 'override_noop': override_noop, + 'override_restored': override_restored, + 'settings__initial': settings_initial, + 'settings_after': settings_after, 'settings_noop': settings_noop, - 'enabled_restored': enabled_restored, + 'settings_restored': settings_restored, + 'allowed__initial': allowed_initial, + 'allowed_after': allowed_after, + 'allowed_noop': allowed_noop, + 'allowed_restored': allowed_restored, } -def run_check(override, outfd): +def run_singlephase_check(override, outfd): with os.fdopen(outfd, 'w') as outfile: sys.stdout = outfile sys.stderr = outfile - results = check(override) + results = check_singlephase(override) json.dump(results, outfile) diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index 7019a3a967de06..697f3e892f8fda 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -1374,23 +1374,25 @@ def check(enabled, override): else: enabled_after = (override > 0) expected = { - 'override_requested': override, - 'settings_before': settings, - 'enabled_initial': enabled, - 'override_initial': 0, + 'requested': override, + 'override__initial': 0, + 'override_after': override, + 'override_noop': override, + 'override_restored': 0, + 'allowed__initial': enabled, + 'allowed_after': enabled_after, + 'allowed_noop': enabled_after, + 'allowed_restored': enabled, + 'settings__initial': settings, 'settings_after': settings, - 'enabled_after': enabled_after, - 'override_actual': override, - 'settings_final': settings, - 'override_noop': 0, 'settings_noop': settings, - 'enabled_restored': enabled, + 'settings_restored': settings, } r, w = os.pipe() script = textwrap.dedent(f''' - from test.test_capi.check_config import run_check - run_check({override}, {w}) + from test.test_capi.check_config import run_singlephase_check + run_singlephase_check({override}, {w}) ''') with os.fdopen(r) as stdout: ret = support.run_in_subinterp_with_config(script, **kwargs) From 282e6d361945474e713a527dd38d907b80fe4930 Mon Sep 17 00:00:00 2001 From: Eric Snow <ericsnowcurrently@gmail.com> Date: Thu, 12 Jan 2023 14:33:30 -0700 Subject: [PATCH 14/22] Allow long test output. --- Lib/test/test_capi/test_misc.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index 697f3e892f8fda..55ee59b96ae1b7 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -1402,6 +1402,8 @@ def check(enabled, override): self.assertEqual(results, expected) + self.maxDiff = None + # setting: check disabled with self.subTest('config disabled, override disabled'): check(False, -1) From 3cb86454b51030149951798a5d239c21dff57b04 Mon Sep 17 00:00:00 2001 From: Eric Snow <ericsnowcurrently@gmail.com> Date: Thu, 12 Jan 2023 14:27:06 -0700 Subject: [PATCH 15/22] Do not show the noop values unless different. --- Lib/test/test_capi/check_config.py | 14 +++++++++----- Lib/test/test_capi/test_misc.py | 3 --- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/Lib/test/test_capi/check_config.py b/Lib/test/test_capi/check_config.py index 1ae7ce63b610ea..fb188aad1cf5d0 100644 --- a/Lib/test/test_capi/check_config.py +++ b/Lib/test/test_capi/check_config.py @@ -30,12 +30,19 @@ def check_singlephase(override): allowed_after = import_singlephase() # Apply the override again and check. + noop = {} override_after = _imp._override_multi_interp_extensions_check(override) settings_noop = _testinternalcapi.get_interp_settings() + if settings_noop != settings_after: + noop['settings_noop'] = settings_noop allowed_noop = import_singlephase() + if allowed_noop != allowed_after: + noop['allowed_noop'] = allowed_noop # Restore the original setting and check. override_noop = _imp._override_multi_interp_extensions_check(override_initial) + if override_noop != override_after: + noop['override_noop'] = override_noop settings_restored = _testinternalcapi.get_interp_settings() allowed_restored = import_singlephase() @@ -43,21 +50,18 @@ def check_singlephase(override): override_restored = _imp._override_multi_interp_extensions_check(override_initial) assert(_testinternalcapi.get_interp_settings() == settings_restored) - return { + return dict({ 'requested': override, 'override__initial': override_initial, 'override_after': override_after, - 'override_noop': override_noop, 'override_restored': override_restored, 'settings__initial': settings_initial, 'settings_after': settings_after, - 'settings_noop': settings_noop, 'settings_restored': settings_restored, 'allowed__initial': allowed_initial, 'allowed_after': allowed_after, - 'allowed_noop': allowed_noop, 'allowed_restored': allowed_restored, - } + }, **noop) def run_singlephase_check(override, outfd): diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index 55ee59b96ae1b7..c42ff230a5ed1c 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -1377,15 +1377,12 @@ def check(enabled, override): 'requested': override, 'override__initial': 0, 'override_after': override, - 'override_noop': override, 'override_restored': 0, 'allowed__initial': enabled, 'allowed_after': enabled_after, - 'allowed_noop': enabled_after, 'allowed_restored': enabled, 'settings__initial': settings, 'settings_after': settings, - 'settings_noop': settings, 'settings_restored': settings, } From 81abbfbdab1c3ec67abae1b410346590501c02c1 Mon Sep 17 00:00:00 2001 From: Eric Snow <ericsnowcurrently@gmail.com> Date: Thu, 12 Jan 2023 14:31:21 -0700 Subject: [PATCH 16/22] Add comments to the expected values. --- Lib/test/test_capi/test_misc.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index c42ff230a5ed1c..faa4bb28a08f4f 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -1378,12 +1378,14 @@ def check(enabled, override): 'override__initial': 0, 'override_after': override, 'override_restored': 0, - 'allowed__initial': enabled, - 'allowed_after': enabled_after, - 'allowed_restored': enabled, + # The override should not affect the config or settings. 'settings__initial': settings, 'settings_after': settings, 'settings_restored': settings, + # These are the most likely values to be wrong. + 'allowed__initial': enabled, + 'allowed_after': enabled_after, + 'allowed_restored': enabled, } r, w = os.pipe() From db5d35a3b096fd96ded6955aa4fe3304c7098dbc Mon Sep 17 00:00:00 2001 From: Eric Snow <ericsnowcurrently@gmail.com> Date: Thu, 12 Jan 2023 14:33:10 -0700 Subject: [PATCH 17/22] Tweak the subtest labels. --- Lib/test/test_capi/test_misc.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index faa4bb28a08f4f..ad33f35382c913 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -1404,19 +1404,19 @@ def check(enabled, override): self.maxDiff = None # setting: check disabled - with self.subTest('config disabled, override disabled'): + with self.subTest('config: check disabled; override: disabled'): check(False, -1) - with self.subTest('config disabled, override cleared'): + with self.subTest('config: check disabled; override: use config'): check(False, 0) - with self.subTest('config disabled, override enabled'): + with self.subTest('config: check disabled; override: enabled'): check(False, 1) # setting: check enabled - with self.subTest('config enabled, override disabled'): + with self.subTest('config: check enabled; override: disabled'): check(True, -1) - with self.subTest('config enabled, override cleared'): + with self.subTest('config: check enabled; override: use config'): check(True, 0) - with self.subTest('config enabled, override enabled'): + with self.subTest('config: check enabled; override: enabled'): check(True, 1) def test_mutate_exception(self): From e0c55adce67641d0601b54d5c582f1202800ecbd Mon Sep 17 00:00:00 2001 From: Eric Snow <ericsnowcurrently@gmail.com> Date: Thu, 12 Jan 2023 15:24:59 -0700 Subject: [PATCH 18/22] Fix the expected results. --- Lib/test/test_capi/test_misc.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index ad33f35382c913..0414c8a315aa8e 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -1369,10 +1369,7 @@ def check(enabled, override): settings = { 'feature_flags': flags, } - if override == 0: - enabled_after = enabled - else: - enabled_after = (override > 0) + expected = { 'requested': override, 'override__initial': 0, @@ -1383,9 +1380,9 @@ def check(enabled, override): 'settings_after': settings, 'settings_restored': settings, # These are the most likely values to be wrong. - 'allowed__initial': enabled, - 'allowed_after': enabled_after, - 'allowed_restored': enabled, + 'allowed__initial': not enabled, + 'allowed_after': not ((override > 0) if override else enabled), + 'allowed_restored': not enabled, } r, w = os.pipe() From 3b2dd6d07cbd9811a9d45ab04eb1bee109bdd7ae Mon Sep 17 00:00:00 2001 From: Eric Snow <ericsnowcurrently@gmail.com> Date: Thu, 12 Jan 2023 15:26:55 -0700 Subject: [PATCH 19/22] Add a test just for how the setting is used. --- Lib/test/test_capi/check_config.py | 14 ++++++++--- Lib/test/test_capi/test_misc.py | 37 ++++++++++++++++++++++++++++-- 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_capi/check_config.py b/Lib/test/test_capi/check_config.py index fb188aad1cf5d0..ff2b8cf19a61f3 100644 --- a/Lib/test/test_capi/check_config.py +++ b/Lib/test/test_capi/check_config.py @@ -18,7 +18,7 @@ def import_singlephase(): return False -def check_singlephase(override): +def check_singlephase_override(override): # Check using the default setting. settings_initial = _testinternalcapi.get_interp_settings() allowed_initial = import_singlephase() @@ -64,9 +64,17 @@ def check_singlephase(override): }, **noop) -def run_singlephase_check(override, outfd): +def run_singlephase_check(outfd): with os.fdopen(outfd, 'w') as outfile: sys.stdout = outfile sys.stderr = outfile - results = check_singlephase(override) + allowed = import_singlephase() + json.dump({'allowed': allowed}, outfile) + + +def run_singlephase_override_check(override, outfd): + with os.fdopen(outfd, 'w') as outfile: + sys.stdout = outfile + sys.stderr = outfile + results = check_singlephase_override(override) json.dump(results, outfile) diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index 0414c8a315aa8e..162f30cd28267c 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -1336,6 +1336,39 @@ def test_configured_settings(self): self.assertEqual(settings, expected) + @unittest.skipIf(_testsinglephase is None, "test requires _testsinglephase module") + @unittest.skipUnless(hasattr(os, "pipe"), "requires os.pipe()") + def test_setting_extensions_subinterp_check(self): + """ + This verifies that the interpreter's configured setting + is used during import. + """ + import json + + for enabled in (True, False): + label = 'enabled' if enabled else 'disabled' + with self.subTest(f'config: check {label}'): + kwargs = { + 'allow_fork': True, + 'allow_exec': True, + 'allow_threads': True, + 'allow_daemon_threads': True, + 'check_multi_interp_extensions': enabled, + } + + r, w = os.pipe() + script = textwrap.dedent(f''' + from test.test_capi import check_config + check_config.run_singlephase_check({w}) + ''') + with os.fdopen(r) as stdout: + ret = support.run_in_subinterp_with_config(script, **kwargs) + self.assertEqual(ret, 0) + out = stdout.read() + results = json.loads(out) + + self.assertEqual(results, {'allowed': not enabled}) + @unittest.skipIf(_testsinglephase is None, "test requires _testsinglephase module") @unittest.skipUnless(hasattr(os, "pipe"), "requires os.pipe()") def test_overridden_setting_extensions_subinterp_check(self): @@ -1387,8 +1420,8 @@ def check(enabled, override): r, w = os.pipe() script = textwrap.dedent(f''' - from test.test_capi.check_config import run_singlephase_check - run_singlephase_check({override}, {w}) + from test.test_capi import check_config + check_config.run_singlephase_override_check({override}, {w}) ''') with os.fdopen(r) as stdout: ret = support.run_in_subinterp_with_config(script, **kwargs) From d648a7b4f867d99f976e4b25ed31561522069c87 Mon Sep 17 00:00:00 2001 From: Eric Snow <ericsnowcurrently@gmail.com> Date: Thu, 12 Jan 2023 15:40:18 -0700 Subject: [PATCH 20/22] Revert "Add a test just for how the setting is used." This reverts commit 3b2dd6d07cbd9811a9d45ab04eb1bee109bdd7ae. --- Lib/test/test_capi/check_config.py | 14 +++-------- Lib/test/test_capi/test_misc.py | 37 ++---------------------------- 2 files changed, 5 insertions(+), 46 deletions(-) diff --git a/Lib/test/test_capi/check_config.py b/Lib/test/test_capi/check_config.py index ff2b8cf19a61f3..fb188aad1cf5d0 100644 --- a/Lib/test/test_capi/check_config.py +++ b/Lib/test/test_capi/check_config.py @@ -18,7 +18,7 @@ def import_singlephase(): return False -def check_singlephase_override(override): +def check_singlephase(override): # Check using the default setting. settings_initial = _testinternalcapi.get_interp_settings() allowed_initial = import_singlephase() @@ -64,17 +64,9 @@ def check_singlephase_override(override): }, **noop) -def run_singlephase_check(outfd): +def run_singlephase_check(override, outfd): with os.fdopen(outfd, 'w') as outfile: sys.stdout = outfile sys.stderr = outfile - allowed = import_singlephase() - json.dump({'allowed': allowed}, outfile) - - -def run_singlephase_override_check(override, outfd): - with os.fdopen(outfd, 'w') as outfile: - sys.stdout = outfile - sys.stderr = outfile - results = check_singlephase_override(override) + results = check_singlephase(override) json.dump(results, outfile) diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index 162f30cd28267c..0414c8a315aa8e 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -1336,39 +1336,6 @@ def test_configured_settings(self): self.assertEqual(settings, expected) - @unittest.skipIf(_testsinglephase is None, "test requires _testsinglephase module") - @unittest.skipUnless(hasattr(os, "pipe"), "requires os.pipe()") - def test_setting_extensions_subinterp_check(self): - """ - This verifies that the interpreter's configured setting - is used during import. - """ - import json - - for enabled in (True, False): - label = 'enabled' if enabled else 'disabled' - with self.subTest(f'config: check {label}'): - kwargs = { - 'allow_fork': True, - 'allow_exec': True, - 'allow_threads': True, - 'allow_daemon_threads': True, - 'check_multi_interp_extensions': enabled, - } - - r, w = os.pipe() - script = textwrap.dedent(f''' - from test.test_capi import check_config - check_config.run_singlephase_check({w}) - ''') - with os.fdopen(r) as stdout: - ret = support.run_in_subinterp_with_config(script, **kwargs) - self.assertEqual(ret, 0) - out = stdout.read() - results = json.loads(out) - - self.assertEqual(results, {'allowed': not enabled}) - @unittest.skipIf(_testsinglephase is None, "test requires _testsinglephase module") @unittest.skipUnless(hasattr(os, "pipe"), "requires os.pipe()") def test_overridden_setting_extensions_subinterp_check(self): @@ -1420,8 +1387,8 @@ def check(enabled, override): r, w = os.pipe() script = textwrap.dedent(f''' - from test.test_capi import check_config - check_config.run_singlephase_override_check({override}, {w}) + from test.test_capi.check_config import run_singlephase_check + run_singlephase_check({override}, {w}) ''') with os.fdopen(r) as stdout: ret = support.run_in_subinterp_with_config(script, **kwargs) From dc8d877b250d3408b0cd23cba7e60ecc2e8f19cf Mon Sep 17 00:00:00 2001 From: Eric Snow <ericsnowcurrently@gmail.com> Date: Thu, 12 Jan 2023 16:22:45 -0700 Subject: [PATCH 21/22] Add a test for the various settings and overrides for a singlephase extension. --- Lib/test/test_import/__init__.py | 110 ++++++++++++++++++++++--------- 1 file changed, 79 insertions(+), 31 deletions(-) diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index ac5eff7cb8a8af..96815b3f758a5b 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -1418,9 +1418,16 @@ def pipe(self): os.set_blocking(r, False) return (r, w) - def import_script(self, name, fd): + def import_script(self, name, fd, check_override=None): + override_text = '' + if check_override is not None: + override_text = f''' + import _imp + _imp._override_multi_interp_extensions_check({check_override}) + ''' return textwrap.dedent(f''' import os, sys + {override_text} try: import {name} except ImportError as exc: @@ -1430,47 +1437,54 @@ def import_script(self, name, fd): os.write({fd}, text.encode('utf-8')) ''') - def check_compatible_shared(self, name, *, strict=False): - # Verify that the named module may be imported in a subinterpreter. - # - # The subinterpreter will be in the current process. - # The module will have already been imported in the main interpreter. - # Thus, for extension/builtin modules, the module definition will - # have been loaded already and cached globally. - # - # "strict" determines whether or not the interpreter will be - # configured to check for modules that are not compatible - # with use in multiple interpreters. - - # This check should always pass for all modules if not strict. - + def run_shared(self, name, *, + check_singlephase_setting=False, + check_singlephase_override=None, + ): + """ + Try importing the named module in a subinterpreter. + + The subinterpreter will be in the current process. + The module will have already been imported in the main interpreter. + Thus, for extension/builtin modules, the module definition will + have been loaded already and cached globally. + + "check_singlephase_setting" determines whether or not + the interpreter will be configured to check for modules + that are not compatible with use in multiple interpreters. + + This should always return "okay" for all modules if the + setting is False (with no override). + """ __import__(name) - r, w = self.pipe() - ret = run_in_subinterp_with_config( - self.import_script(name, w), + kwargs = dict( **self.RUN_KWARGS, - check_multi_interp_extensions=strict, + check_multi_interp_extensions=check_singlephase_setting, ) + + r, w = self.pipe() + script = self.import_script(name, w, check_singlephase_override) + + ret = run_in_subinterp_with_config(script, **kwargs) self.assertEqual(ret, 0) - out = os.read(r, 100) + return os.read(r, 100) + + def check_compatible_shared(self, name, *, strict=False): + # Verify that the named module may be imported in a subinterpreter. + # (See run_shared() for more info.) + out = self.run_shared(name, check_singlephase_setting=strict) self.assertEqual(out, b'okay') def check_incompatible_shared(self, name): # Differences from check_compatible_shared(): # * verify that import fails # * "strict" is always True - __import__(name) - - r, w = self.pipe() - ret = run_in_subinterp_with_config( - self.import_script(name, w), - **self.RUN_KWARGS, - check_multi_interp_extensions=True, + out = self.run_shared(name, check_singlephase_setting=True) + self.assertEqual( + out.decode('utf-8'), + f'ImportError: module {name} does not support loading in subinterpreters', ) - self.assertEqual(ret, 0) - out = os.read(r, 100).decode('utf-8') - self.assertEqual(out, f'ImportError: module {name} does not support loading in subinterpreters') def check_compatible_isolated(self, name, *, strict=False): # Differences from check_compatible_shared(): @@ -1530,7 +1544,7 @@ def test_frozen_compat(self): with self.subTest(f'{module}: strict, shared'): self.check_compatible_shared(module, strict=True) - @unittest.skipIf(_testsinglephase is None, "test requires _testsinglphase module") + @unittest.skipIf(_testsinglephase is None, "test requires _testsinglephase module") def test_single_init_extension_compat(self): module = '_testsinglephase' with self.subTest(f'{module}: not strict'): @@ -1561,6 +1575,40 @@ def test_python_compat(self): with self.subTest(f'{module}: strict, isolated'): self.check_compatible_isolated(module, strict=True) + @unittest.skipIf(_testsinglephase is None, "test requires _testsinglephase module") + def test_singlephase_check_with_setting_and_override(self): + module = '_testsinglephase' + + def check_compatible(setting, override): + out = self.run_shared( + module, + check_singlephase_setting=setting, + check_singlephase_override=override, + ) + self.assertEqual(out, b'okay') + + def check_incompatible(setting, override): + out = self.run_shared( + module, + check_singlephase_setting=setting, + check_singlephase_override=override, + ) + self.assertNotEqual(out, b'okay') + + with self.subTest('config: check enabled; override: enabled'): + check_incompatible(True, 1) + with self.subTest('config: check enabled; override: use config'): + check_incompatible(True, 0) + with self.subTest('config: check enabled; override: disabled'): + check_compatible(True, -1) + + with self.subTest('config: check disabled; override: enabled'): + check_incompatible(False, 1) + with self.subTest('config: check disabled; override: use config'): + check_compatible(False, 0) + with self.subTest('config: check disabled; override: disabled'): + check_compatible(False, -1) + if __name__ == '__main__': # Test needs to be a package, so we can do relative imports. From 5fba674960fa0ec981c9ca502b4d557c08e1c881 Mon Sep 17 00:00:00 2001 From: Eric Snow <ericsnowcurrently@gmail.com> Date: Thu, 2 Feb 2023 17:15:21 -0700 Subject: [PATCH 22/22] Fix check_config.py. --- Lib/test/test_capi/check_config.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/Lib/test/test_capi/check_config.py b/Lib/test/test_capi/check_config.py index fb188aad1cf5d0..aaedd82f39af50 100644 --- a/Lib/test/test_capi/check_config.py +++ b/Lib/test/test_capi/check_config.py @@ -8,14 +8,15 @@ def import_singlephase(): - assert '_singlephase' not in sys.modules + assert '_testsinglephase' not in sys.modules try: - import _singlephase + import _testsinglephase except ImportError: - return True - else: - del sys.modules['_singlephase'] + sys.modules.pop('_testsinglephase') return False + else: + del sys.modules['_testsinglephase'] + return True def check_singlephase(override): @@ -68,5 +69,9 @@ def run_singlephase_check(override, outfd): with os.fdopen(outfd, 'w') as outfile: sys.stdout = outfile sys.stderr = outfile - results = check_singlephase(override) - json.dump(results, outfile) + try: + results = check_singlephase(override) + json.dump(results, outfile) + finally: + sys.stdout = sys.__stdout__ + sys.stderr = sys.__stderr__