gh-101659: Isolate "obmalloc" State to Each Interpreter (gh-101660)

ericsnowcurrently · web-flow · commit df3173d28ef2 · 2023-04-24T17:23:57.000-06:00
This is strictly about moving the "obmalloc" runtime state from
`_PyRuntimeState` to `PyInterpreterState`.  Doing so improves isolation
between interpreters, specifically most of the memory (incl. objects)
allocated for each interpreter's use.  This is important for a
per-interpreter GIL, but such isolation is valuable even without it.

FWIW, a per-interpreter obmalloc is the proverbial
canary-in-the-coalmine when it comes to the isolation of objects between
interpreters.  Any object that leaks (unintentionally) to another
interpreter is highly likely to cause a crash (on debug builds at
least).  That's a useful thing to know, relative to interpreter
isolation.
diff --git a/Include/cpython/initconfig.h b/Include/cpython/initconfig.h
@@ -245,6 +245,8 @@ PyAPI_FUNC(PyStatus) PyConfig_SetWideStringList(PyConfig *config,
 /* --- PyInterpreterConfig ------------------------------------ */
 
 typedef struct {
+    // XXX "allow_object_sharing"?  "own_objects"?
+    int use_main_obmalloc;
     int allow_fork;
     int allow_exec;
     int allow_threads;
@@ -254,6 +256,7 @@ typedef struct {
 
 #define _PyInterpreterConfig_INIT \
     { \
+        .use_main_obmalloc = 0, \
         .allow_fork = 0, \
         .allow_exec = 0, \
         .allow_threads = 1, \
@@ -263,6 +266,7 @@ typedef struct {
 
 #define _PyInterpreterConfig_LEGACY_INIT \
     { \
+        .use_main_obmalloc = 1, \
         .allow_fork = 1, \
         .allow_exec = 1, \
         .allow_threads = 1, \
diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h
@@ -11,6 +11,10 @@ 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 the interpreter share obmalloc runtime state
+   with the main interpreter. */
+#define Py_RTFLAGS_USE_MAIN_OBMALLOC (1UL << 5)
+
 /* Set if import should check a module for subinterpreter support. */
 #define Py_RTFLAGS_MULTI_INTERP_EXTENSIONS (1UL << 8)
 
diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h
@@ -23,11 +23,12 @@ extern "C" {
 #include "pycore_function.h"      // FUNC_MAX_WATCHERS
 #include "pycore_genobject.h"     // struct _Py_async_gen_state
 #include "pycore_gc.h"            // struct _gc_runtime_state
+#include "pycore_global_objects.h"  // struct _Py_interp_static_objects
 #include "pycore_import.h"        // struct _import_state
 #include "pycore_instruments.h"   // PY_MONITORING_EVENTS
 #include "pycore_list.h"          // struct _Py_list_state
-#include "pycore_global_objects.h"  // struct _Py_interp_static_objects
 #include "pycore_object_state.h"   // struct _py_object_state
+#include "pycore_obmalloc.h"      // struct obmalloc_state
 #include "pycore_tuple.h"         // struct _Py_tuple_state
 #include "pycore_typeobject.h"    // struct type_cache
 #include "pycore_unicodeobject.h" // struct _Py_unicode_state
@@ -82,6 +83,8 @@ struct _is {
     int _initialized;
     int finalizing;
 
+    struct _obmalloc_state obmalloc;
+
     struct _ceval_state ceval;
     struct _gc_runtime_state gc;
 
diff --git a/Include/internal/pycore_obmalloc.h b/Include/internal/pycore_obmalloc.h
@@ -657,8 +657,12 @@ struct _obmalloc_usage {
 #endif /* WITH_PYMALLOC_RADIX_TREE */
 
 
-struct _obmalloc_state {
+struct _obmalloc_global_state {
     int dump_debug_stats;
+    Py_ssize_t interpreter_leaks;
+};
+
+struct _obmalloc_state {
     struct _obmalloc_pools pools;
     struct _obmalloc_mgmt mgmt;
     struct _obmalloc_usage usage;
@@ -675,7 +679,11 @@ void _PyObject_VirtualFree(void *, size_t size);
 
 
 /* This function returns the number of allocated memory blocks, regardless of size */
-PyAPI_FUNC(Py_ssize_t) _Py_GetAllocatedBlocks(void);
+extern Py_ssize_t _Py_GetGlobalAllocatedBlocks(void);
+#define _Py_GetAllocatedBlocks() \
+    _Py_GetGlobalAllocatedBlocks()
+extern Py_ssize_t _PyInterpreterState_GetAllocatedBlocks(PyInterpreterState *);
+extern void _PyInterpreterState_FinalizeAllocatedBlocks(PyInterpreterState *);
 
 
 #ifdef WITH_PYMALLOC
diff --git a/Include/internal/pycore_obmalloc_init.h b/Include/internal/pycore_obmalloc_init.h
@@ -54,9 +54,13 @@ extern "C" {
 #  error "NB_SMALL_SIZE_CLASSES should be less than 64"
 #endif
 
-#define _obmalloc_state_INIT(obmalloc) \
+#define _obmalloc_global_state_INIT \
     { \
         .dump_debug_stats = -1, \
+    }
+
+#define _obmalloc_state_INIT(obmalloc) \
+    { \
         .pools = { \
             .used = _obmalloc_pools_INIT(obmalloc.pools), \
         }, \
diff --git a/Include/internal/pycore_pylifecycle.h b/Include/internal/pycore_pylifecycle.h
@@ -64,6 +64,7 @@ extern void _PyAtExit_Fini(PyInterpreterState *interp);
 extern void _PyThread_FiniType(PyInterpreterState *interp);
 extern void _Py_Deepfreeze_Fini(void);
 extern void _PyArg_Fini(void);
+extern void _Py_FinalizeAllocatedBlocks(_PyRuntimeState *);
 
 extern PyStatus _PyGILState_Init(PyInterpreterState *interp);
 extern PyStatus _PyGILState_SetTstate(PyThreadState *tstate);
diff --git a/Include/internal/pycore_pystate.h b/Include/internal/pycore_pystate.h
@@ -33,6 +33,13 @@ _Py_IsMainInterpreter(PyInterpreterState *interp)
     return (interp == _PyInterpreterState_Main());
 }
 
+static inline int
+_Py_IsMainInterpreterFinalizing(PyInterpreterState *interp)
+{
+    return (_PyRuntimeState_GetFinalizing(interp->runtime) != NULL &&
+            interp == &interp->runtime->_main_interpreter);
+}
+
 
 static inline const PyConfig *
 _Py_GetMainConfig(void)
diff --git a/Include/internal/pycore_runtime.h b/Include/internal/pycore_runtime.h
@@ -21,7 +21,6 @@ extern "C" {
 #include "pycore_pymem.h"           // struct _pymem_allocators
 #include "pycore_pyhash.h"          // struct pyhash_runtime_state
 #include "pycore_pythread.h"        // struct _pythread_runtime_state
-#include "pycore_obmalloc.h"        // struct obmalloc_state
 #include "pycore_signal.h"          // struct _signals_runtime_state
 #include "pycore_time.h"            // struct _time_runtime_state
 #include "pycore_tracemalloc.h"     // struct _tracemalloc_runtime_state
@@ -88,7 +87,7 @@ typedef struct pyruntimestate {
     _Py_atomic_address _finalizing;
 
     struct _pymem_allocators allocators;
-    struct _obmalloc_state obmalloc;
+    struct _obmalloc_global_state obmalloc;
     struct pyhash_runtime_state pyhash_state;
     struct _time_runtime_state time;
     struct _pythread_runtime_state threads;
diff --git a/Include/internal/pycore_runtime_init.h b/Include/internal/pycore_runtime_init.h
@@ -29,7 +29,7 @@ extern PyTypeObject _PyExc_MemoryError;
             _pymem_allocators_debug_INIT, \
             _pymem_allocators_obj_arena_INIT, \
         }, \
-        .obmalloc = _obmalloc_state_INIT(runtime.obmalloc), \
+        .obmalloc = _obmalloc_global_state_INIT, \
         .pyhash_state = pyhash_state_INIT, \
         .signals = _signals_RUNTIME_INIT, \
         .interpreters = { \
@@ -93,6 +93,7 @@ extern PyTypeObject _PyExc_MemoryError;
     { \
         .id_refcount = -1, \
         .imports = IMPORTS_INIT, \
+        .obmalloc = _obmalloc_state_INIT(INTERP.obmalloc), \
         .ceval = { \
             .recursion_limit = Py_DEFAULT_RECURSION_LIMIT, \
         }, \
diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py
@@ -1211,20 +1211,25 @@ def test_configured_settings(self):
         """
         import json
 
+        OBMALLOC = 1<<5
         EXTENSIONS = 1<<8
         THREADS = 1<<10
         DAEMON_THREADS = 1<<11
         FORK = 1<<15
         EXEC = 1<<16
 
-        features = ['fork', 'exec', 'threads', 'daemon_threads', 'extensions']
+        features = ['obmalloc', 'fork', 'exec', 'threads', 'daemon_threads',
+                    'extensions']
         kwlist = [f'allow_{n}' for n in features]
+        kwlist[0] = 'use_main_obmalloc'
         kwlist[-1] = 'check_multi_interp_extensions'
+
+        # expected to work
         for config, expected in {
-            (True, True, True, True, True):
-                FORK | EXEC | THREADS | DAEMON_THREADS | EXTENSIONS,
-            (False, False, False, False, False): 0,
-            (False, False, True, False, True): THREADS | EXTENSIONS,
+            (True, True, True, True, True, True):
+                OBMALLOC | FORK | EXEC | THREADS | DAEMON_THREADS | EXTENSIONS,
+            (True, False, False, False, False, False): OBMALLOC,
+            (False, False, False, True, False, True): THREADS | EXTENSIONS,
         }.items():
             kwargs = dict(zip(kwlist, config))
             expected = {
@@ -1246,6 +1251,20 @@ def test_configured_settings(self):
 
                 self.assertEqual(settings, expected)
 
+        # expected to fail
+        for config in [
+            (False, False, False, False, False, False),
+        ]:
+            kwargs = dict(zip(kwlist, config))
+            with self.subTest(config):
+                script = textwrap.dedent(f'''
+                    import _testinternalcapi
+                    _testinternalcapi.get_interp_settings()
+                    raise NotImplementedError('unreachable')
+                    ''')
+                with self.assertRaises(RuntimeError):
+                    support.run_in_subinterp_with_config(script, **kwargs)
+
     @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):
@@ -1257,13 +1276,15 @@ def test_overridden_setting_extensions_subinterp_check(self):
         """
         import json
 
+        OBMALLOC = 1<<5
         EXTENSIONS = 1<<8
         THREADS = 1<<10
         DAEMON_THREADS = 1<<11
         FORK = 1<<15
         EXEC = 1<<16
-        BASE_FLAGS = FORK | EXEC | THREADS | DAEMON_THREADS
+        BASE_FLAGS = OBMALLOC | FORK | EXEC | THREADS | DAEMON_THREADS
         base_kwargs = {
+            'use_main_obmalloc': True,
             'allow_fork': True,
             'allow_exec': True,
             'allow_threads': True,
diff --git a/Lib/test/test_embed.py b/Lib/test/test_embed.py
@@ -1656,6 +1656,7 @@ def test_init_use_frozen_modules(self):
                                        api=API_PYTHON, env=env)
 
     def test_init_main_interpreter_settings(self):
+        OBMALLOC = 1<<5
         EXTENSIONS = 1<<8
         THREADS = 1<<10
         DAEMON_THREADS = 1<<11
@@ -1664,7 +1665,7 @@ def test_init_main_interpreter_settings(self):
         expected = {
             # All optional features should be enabled.
             'feature_flags':
-                FORK | EXEC | THREADS | DAEMON_THREADS,
+                OBMALLOC | 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
@@ -1636,7 +1636,12 @@ class SubinterpImportTests(unittest.TestCase):
         allow_exec=False,
         allow_threads=True,
         allow_daemon_threads=False,
+        # Isolation-related config values aren't included here.
     )
+    ISOLATED = dict(
+        use_main_obmalloc=False,
+    )
+    NOT_ISOLATED = {k: not v for k, v in ISOLATED.items()}
 
     @unittest.skipUnless(hasattr(os, "pipe"), "requires os.pipe()")
     def pipe(self):
@@ -1669,6 +1674,7 @@ def import_script(self, name, fd, check_override=None):
     def run_here(self, name, *,
                  check_singlephase_setting=False,
                  check_singlephase_override=None,
+                 isolated=False,
                  ):
         """
         Try importing the named module in a subinterpreter.
@@ -1689,6 +1695,7 @@ def run_here(self, name, *,
 
         kwargs = dict(
             **self.RUN_KWARGS,
+            **(self.ISOLATED if isolated else self.NOT_ISOLATED),
             check_multi_interp_extensions=check_singlephase_setting,
         )
 
@@ -1699,33 +1706,36 @@ def run_here(self, name, *,
         self.assertEqual(ret, 0)
         return os.read(r, 100)
 
-    def check_compatible_here(self, name, *, strict=False):
+    def check_compatible_here(self, name, *, strict=False, isolated=False):
         # Verify that the named module may be imported in a subinterpreter.
         # (See run_here() for more info.)
         out = self.run_here(name,
                             check_singlephase_setting=strict,
+                            isolated=isolated,
                             )
         self.assertEqual(out, b'okay')
 
-    def check_incompatible_here(self, name):
+    def check_incompatible_here(self, name, *, isolated=False):
         # Differences from check_compatible_here():
         #  * verify that import fails
         #  * "strict" is always True
         out = self.run_here(name,
                             check_singlephase_setting=True,
+                            isolated=isolated,
                             )
         self.assertEqual(
             out.decode('utf-8'),
             f'ImportError: module {name} does not support loading in subinterpreters',
         )
 
-    def check_compatible_fresh(self, name, *, strict=False):
+    def check_compatible_fresh(self, name, *, strict=False, isolated=False):
         # Differences from check_compatible_here():
         #  * subinterpreter in a new process
         #  * module has never been imported before in that process
         #  * this tests importing the module for the first time
         kwargs = dict(
             **self.RUN_KWARGS,
+            **(self.ISOLATED if isolated else self.NOT_ISOLATED),
             check_multi_interp_extensions=strict,
         )
         _, out, err = script_helper.assert_python_ok('-c', textwrap.dedent(f'''
@@ -1743,12 +1753,13 @@ def check_compatible_fresh(self, name, *, strict=False):
         self.assertEqual(err, b'')
         self.assertEqual(out, b'okay')
 
-    def check_incompatible_fresh(self, name):
+    def check_incompatible_fresh(self, name, *, isolated=False):
         # Differences from check_compatible_fresh():
         #  * verify that import fails
         #  * "strict" is always True
         kwargs = dict(
             **self.RUN_KWARGS,
+            **(self.ISOLATED if isolated else self.NOT_ISOLATED),
             check_multi_interp_extensions=True,
         )
         _, out, err = script_helper.assert_python_ok('-c', textwrap.dedent(f'''
@@ -1854,6 +1865,14 @@ def check_incompatible(setting, override):
         with self.subTest('config: check disabled; override: disabled'):
             check_compatible(False, -1)
 
+    def test_isolated_config(self):
+        module = 'threading'
+        require_pure_python(module)
+        with self.subTest(f'{module}: strict, not fresh'):
+            self.check_compatible_here(module, strict=True, isolated=True)
+        with self.subTest(f'{module}: strict, fresh'):
+            self.check_compatible_fresh(module, strict=True, isolated=True)
+
 
 class TestSinglePhaseSnapshot(ModuleSnapshot):
 
diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py
@@ -1343,6 +1343,7 @@ def func():
             import test.support
             test.support.run_in_subinterp_with_config(
                 {subinterp_code!r},
+                use_main_obmalloc=True,
                 allow_fork=True,
                 allow_exec=True,
                 allow_threads={allowed},
diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c
@@ -1482,6 +1482,7 @@ static PyObject *
 run_in_subinterp_with_config(PyObject *self, PyObject *args, PyObject *kwargs)
 {
     const char *code;
+    int use_main_obmalloc = -1;
     int allow_fork = -1;
     int allow_exec = -1;
     int allow_threads = -1;
@@ -1493,19 +1494,25 @@ run_in_subinterp_with_config(PyObject *self, PyObject *args, PyObject *kwargs)
     PyCompilerFlags cflags = {0};
 
     static char *kwlist[] = {"code",
+                             "use_main_obmalloc",
                              "allow_fork",
                              "allow_exec",
                              "allow_threads",
                              "allow_daemon_threads",
                              "check_multi_interp_extensions",
                              NULL};
     if (!PyArg_ParseTupleAndKeywords(args, kwargs,
-                    "s$ppppp:run_in_subinterp_with_config", kwlist,
-                    &code, &allow_fork, &allow_exec,
+                    "s$pppppp:run_in_subinterp_with_config", kwlist,
+                    &code, &use_main_obmalloc,
+                    &allow_fork, &allow_exec,
                     &allow_threads, &allow_daemon_threads,
                     &check_multi_interp_extensions)) {
         return NULL;
     }
+    if (use_main_obmalloc < 0) {
+        PyErr_SetString(PyExc_ValueError, "missing use_main_obmalloc");
+        return NULL;
+    }
     if (allow_fork < 0) {
         PyErr_SetString(PyExc_ValueError, "missing allow_fork");
         return NULL;
@@ -1532,6 +1539,7 @@ run_in_subinterp_with_config(PyObject *self, PyObject *args, PyObject *kwargs)
     PyThreadState_Swap(NULL);
 
     const _PyInterpreterConfig config = {
+        .use_main_obmalloc = use_main_obmalloc,
         .allow_fork = allow_fork,
         .allow_exec = allow_exec,
         .allow_threads = allow_threads,
diff --git a/Objects/object.c b/Objects/object.c
diff --git a/Objects/obmalloc.c b/Objects/obmalloc.c
diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c
diff --git a/Python/pystate.c b/Python/pystate.c
diff --git a/Python/sysmodule.c b/Python/sysmodule.c
diff --git a/Tools/c-analyzer/cpython/ignored.tsv b/Tools/c-analyzer/cpython/ignored.tsv
