1berry/cpython
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

gh-105699: Use a _Py_hashtable_t for the PyModuleDef Cache (gh-106974)

Browse Source 
This fixes a crasher due to a race condition, triggered infrequently when two isolated (own GIL) subinterpreters simultaneously initialize their sys or builtins modules.  The crash happened due the combination of the "detached" thread state we were using and the "last holder" logic we use for the GIL.  It turns out it's tricky to use the same thread state for different threads.  Who could have guessed?

We solve the problem by eliminating the one object we were still sharing between interpreters.  We replace it with a low-level hashtable, using the "raw" allocator to avoid tying it to the main interpreter.

We also remove the accommodations for "detached" thread states, which were a dubious idea to start with.
This commit is contained in:
Eric Snow 2023-07-28 14:39:08 -06:00 committed by GitHub
parent 55ed85e49c
commit 8ba4df91ae
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 129 additions and 191 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
Include/internal/pycore_import.h
View File

@ -5,6 +5,7 @@
extern "C" { extern "C" {
#endif #endif
#include "pycore_hashtable.h" // _Py_hashtable_t
#include "pycore_time.h" // _PyTime_t #include "pycore_time.h" // _PyTime_t
extern int _PyImport_IsInitialized(PyInterpreterState *); extern int _PyImport_IsInitialized(PyInterpreterState *);
@ -36,19 +37,15 @@ struct _import_runtime_state {
See PyInterpreterState.modules_by_index for more info. */ See PyInterpreterState.modules_by_index for more info. */
Py_ssize_t last_module_index; Py_ssize_t last_module_index;
struct { struct {
/* A thread state tied to the main interpreter, /* A lock to guard the cache. */
used exclusively for when the extensions dict is access/modified
from an arbitrary thread. */
PyThreadState main_tstate;
/* A lock to guard the dict. */
PyThread_type_lock mutex; PyThread_type_lock mutex;
/* A dict mapping (filename, name) to PyModuleDef for modules. /* The actual cache of (filename, name, PyModuleDef) for modules.
Only legacy (single-phase init) extension modules are added Only legacy (single-phase init) extension modules are added
and only if they support multiple initialization (m_size >- 0) and only if they support multiple initialization (m_size >- 0)
or are imported in the main interpreter. or are imported in the main interpreter.
This is initialized lazily in _PyImport_FixupExtensionObject(). This is initialized lazily in _PyImport_FixupExtensionObject().
Modules are added there and looked up in _imp.find_extension(). */ Modules are added there and looked up in _imp.find_extension(). */
PyObject *dict; _Py_hashtable_t *hashtable;
} extensions; } extensions;
/* Package context -- the full module name for package imports */ /* Package context -- the full module name for package imports */
const char * pkgcontext; const char * pkgcontext;

5
Include/internal/pycore_pystate.h
View File

@ -125,11 +125,6 @@ extern PyThreadState * _PyThreadState_New(PyInterpreterState *interp);
extern void _PyThreadState_Bind(PyThreadState *tstate); extern void _PyThreadState_Bind(PyThreadState *tstate);
extern void _PyThreadState_DeleteExcept(PyThreadState *tstate); extern void _PyThreadState_DeleteExcept(PyThreadState *tstate);
extern void _PyThreadState_InitDetached(PyThreadState *, PyInterpreterState *);
extern void _PyThreadState_ClearDetached(PyThreadState *);
extern void _PyThreadState_BindDetached(PyThreadState *);
extern void _PyThreadState_UnbindDetached(PyThreadState *);
// Export for '_testinternalcapi' shared extension // Export for '_testinternalcapi' shared extension
PyAPI_FUNC(PyObject*) _PyThreadState_GetDict(PyThreadState *tstate); PyAPI_FUNC(PyObject*) _PyThreadState_GetDict(PyThreadState *tstate);

5
Include/internal/pycore_runtime_init.h
View File

@ -97,11 +97,6 @@ extern PyTypeObject _PyExc_MemoryError;
in accordance with the specification. */ \ in accordance with the specification. */ \
.autoTSSkey = Py_tss_NEEDS_INIT, \ .autoTSSkey = Py_tss_NEEDS_INIT, \
.parser = _parser_runtime_state_INIT, \ .parser = _parser_runtime_state_INIT, \
.imports = { \
.extensions = { \
.main_tstate = _PyThreadState_INIT, \
}, \
}, \
.ceval = { \ .ceval = { \
.perf = _PyEval_RUNTIME_PERF_INIT, \ .perf = _PyEval_RUNTIME_PERF_INIT, \
}, \ }, \

226
Python/import.c
View File

@ -2,10 +2,12 @@
#include "Python.h" #include "Python.h"
#include "pycore_hashtable.h" // _Py_hashtable_new_full()
#include "pycore_import.h" // _PyImport_BootstrapImp() #include "pycore_import.h" // _PyImport_BootstrapImp()
#include "pycore_initconfig.h" // _PyStatus_OK() #include "pycore_initconfig.h" // _PyStatus_OK()
#include "pycore_interp.h" // struct _import_runtime_state #include "pycore_interp.h" // struct _import_runtime_state
#include "pycore_namespace.h" // _PyNamespace_Type #include "pycore_namespace.h" // _PyNamespace_Type
#include "pycore_object.h" // _Py_SetImmortal()
#include "pycore_pyerrors.h" // _PyErr_SetString() #include "pycore_pyerrors.h" // _PyErr_SetString()
#include "pycore_pyhash.h" // _Py_KeyedHash() #include "pycore_pyhash.h" // _Py_KeyedHash()
#include "pycore_pylifecycle.h" #include "pycore_pylifecycle.h"
@ -905,35 +907,79 @@ extensions_lock_release(void)
dictionary, to avoid loading shared libraries twice. dictionary, to avoid loading shared libraries twice.
*/ */
static void static void *
_extensions_cache_init(void) hashtable_key_from_2_strings(PyObject *str1, PyObject *str2, const char sep)
{ {
/* The runtime (i.e. main interpreter) must be initializing, Py_ssize_t str1_len, str2_len;
so we don't need to worry about the lock. */ const char *str1_data = PyUnicode_AsUTF8AndSize(str1, &str1_len);
_PyThreadState_InitDetached(&EXTENSIONS.main_tstate, const char *str2_data = PyUnicode_AsUTF8AndSize(str2, &str2_len);
_PyInterpreterState_Main()); if (str1_data == NULL || str2_data == NULL) {
return NULL;
}
/* Make sure sep and the NULL byte won't cause an overflow. */
assert(SIZE_MAX - str1_len - str2_len > 2);
size_t size = str1_len + 1 + str2_len + 1;
char *key = PyMem_RawMalloc(size);
if (key == NULL) {
PyErr_NoMemory();
return NULL;
}
strncpy(key, str1_data, str1_len);
key[str1_len] = sep;
strncpy(key + str1_len + 1, str2_data, str2_len + 1);
assert(strlen(key) == size - 1);
return key;
} }
static Py_uhash_t
hashtable_hash_str(const void *key)
{
return _Py_HashBytes(key, strlen((const char *)key));
}
static int
hashtable_compare_str(const void *key1, const void *key2)
{
return strcmp((const char *)key1, (const char *)key2) == 0;
}
static void
hashtable_destroy_str(void *ptr)
{
PyMem_RawFree(ptr);
}
#define HTSEP ':'
static PyModuleDef * static PyModuleDef *
_extensions_cache_get(PyObject *filename, PyObject *name) _extensions_cache_get(PyObject *filename, PyObject *name)
{ {
PyModuleDef *def = NULL; PyModuleDef *def = NULL;
void *key = NULL;
extensions_lock_acquire(); extensions_lock_acquire();
PyObject *key = PyTuple_Pack(2, filename, name); if (EXTENSIONS.hashtable == NULL) {
goto finally;
}
key = hashtable_key_from_2_strings(filename, name, HTSEP);
if (key == NULL) { if (key == NULL) {
goto finally; goto finally;
} }
_Py_hashtable_entry_t *entry = _Py_hashtable_get_entry(
PyObject *extensions = EXTENSIONS.dict; EXTENSIONS.hashtable, key);
if (extensions == NULL) { if (entry == NULL) {
goto finally; goto finally;
} }
def = (PyModuleDef *)PyDict_GetItemWithError(extensions, key); def = (PyModuleDef *)entry->value;
finally: finally:
Py_XDECREF(key);
extensions_lock_release(); extensions_lock_release();
if (key != NULL) {
PyMem_RawFree(key);
}
return def; return def;
} }
@ -941,124 +987,99 @@ static int
_extensions_cache_set(PyObject *filename, PyObject *name, PyModuleDef *def) _extensions_cache_set(PyObject *filename, PyObject *name, PyModuleDef *def)
{ {
int res = -1; int res = -1;
PyThreadState *oldts = NULL;
extensions_lock_acquire(); extensions_lock_acquire();
/* Swap to the main interpreter, if necessary. This matters if if (EXTENSIONS.hashtable == NULL) {
the dict hasn't been created yet or if the item isn't in the _Py_hashtable_allocator_t alloc = {PyMem_RawMalloc, PyMem_RawFree};
dict yet. In both cases we must ensure the relevant objects EXTENSIONS.hashtable = _Py_hashtable_new_full(
are created using the main interpreter. */ hashtable_hash_str,
PyThreadState *main_tstate = &EXTENSIONS.main_tstate; hashtable_compare_str,
PyInterpreterState *interp = _PyInterpreterState_GET(); hashtable_destroy_str, // key
if (!_Py_IsMainInterpreter(interp)) { /* There's no need to decref the def since it's immortal. */
_PyThreadState_BindDetached(main_tstate); NULL, // value
oldts = _PyThreadState_Swap(interp->runtime, main_tstate); &alloc
assert(!_Py_IsMainInterpreter(oldts->interp)); );
if (EXTENSIONS.hashtable == NULL) {
/* Make sure the name and filename objects are owned PyErr_NoMemory();
by the main interpreter. */ goto finally;
name = PyUnicode_InternFromString(PyUnicode_AsUTF8(name)); }
assert(name != NULL);
filename = PyUnicode_InternFromString(PyUnicode_AsUTF8(filename));
assert(filename != NULL);
} }
PyObject *key = PyTuple_Pack(2, filename, name); void *key = hashtable_key_from_2_strings(filename, name, HTSEP);
if (key == NULL) { if (key == NULL) {
goto finally; goto finally;
} }
PyObject *extensions = EXTENSIONS.dict; int already_set = 0;
if (extensions == NULL) { _Py_hashtable_entry_t *entry = _Py_hashtable_get_entry(
extensions = PyDict_New(); EXTENSIONS.hashtable, key);
if (extensions == NULL) { if (entry == NULL) {
if (_Py_hashtable_set(EXTENSIONS.hashtable, key, def) < 0) {
PyMem_RawFree(key);
PyErr_NoMemory();
goto finally; goto finally;
} }
EXTENSIONS.dict = extensions;
} }
else {
PyModuleDef *actual = (PyModuleDef *)PyDict_GetItemWithError(extensions, key); if (entry->value == NULL) {
if (PyErr_Occurred()) { entry->value = def;
goto finally;
} }
else if (actual != NULL) { else {
/* We expect it to be static, so it must be the same pointer. */ /* We expect it to be static, so it must be the same pointer. */
assert(def == actual); assert((PyModuleDef *)entry->value == def);
res = 0; already_set = 1;
goto finally;
} }
PyMem_RawFree(key);
/* This might trigger a resize, which is why we must switch }
to the main interpreter. */ if (!already_set) {
res = PyDict_SetItem(extensions, key, (PyObject *)def); /* We assume that all module defs are statically allocated
if (res < 0) { and will never be freed. Otherwise, we would incref here. */
res = -1; _Py_SetImmortal(def);
goto finally;
} }
res = 0; res = 0;
finally: finally:
Py_XDECREF(key);
if (oldts != NULL) {
_PyThreadState_Swap(interp->runtime, oldts);
_PyThreadState_UnbindDetached(main_tstate);
Py_DECREF(name);
Py_DECREF(filename);
}
extensions_lock_release(); extensions_lock_release();
return res; return res;
} }
static int static void
_extensions_cache_delete(PyObject *filename, PyObject *name) _extensions_cache_delete(PyObject *filename, PyObject *name)
{ {
int res = -1; void *key = NULL;
PyThreadState *oldts = NULL;
extensions_lock_acquire(); extensions_lock_acquire();
PyObject *key = PyTuple_Pack(2, filename, name); if (EXTENSIONS.hashtable == NULL) {
/* It was never added. */
goto finally;
}
key = hashtable_key_from_2_strings(filename, name, HTSEP);
if (key == NULL) { if (key == NULL) {
goto finally; goto finally;
} }
PyObject *extensions = EXTENSIONS.dict; _Py_hashtable_entry_t *entry = _Py_hashtable_get_entry(
if (extensions == NULL) { EXTENSIONS.hashtable, key);
res = 0; if (entry == NULL) {
/* It was never added. */
goto finally; goto finally;
} }
if (entry->value == NULL) {
PyModuleDef *actual = (PyModuleDef *)PyDict_GetItemWithError(extensions, key); /* It was already removed. */
if (PyErr_Occurred()) {
goto finally; goto finally;
} }
else if (actual == NULL) { /* If we hadn't made the stored defs immortal, we would decref here.
/* It was already removed or never added. */ However, this decref would be problematic if the module def were
res = 0; dynamically allocated, it were the last ref, and this function
goto finally; were called with an interpreter other than the def's owner. */
} entry->value = NULL;
/* Swap to the main interpreter, if necessary. */
PyThreadState *main_tstate = &EXTENSIONS.main_tstate;
PyInterpreterState *interp = _PyInterpreterState_GET();
if (!_Py_IsMainInterpreter(interp)) {
_PyThreadState_BindDetached(main_tstate);
oldts = _PyThreadState_Swap(interp->runtime, main_tstate);
assert(!_Py_IsMainInterpreter(oldts->interp));
}
if (PyDict_DelItem(extensions, key) < 0) {
goto finally;
}
res = 0;
finally: finally:
if (oldts != NULL) {
_PyThreadState_Swap(interp->runtime, oldts);
_PyThreadState_UnbindDetached(main_tstate);
}
Py_XDECREF(key);
extensions_lock_release(); extensions_lock_release();
return res; if (key != NULL) {
PyMem_RawFree(key);
}
} }
static void static void
@ -1066,11 +1087,12 @@ _extensions_cache_clear_all(void)
{ {
/* The runtime (i.e. main interpreter) must be finalizing, /* The runtime (i.e. main interpreter) must be finalizing,
so we don't need to worry about the lock. */ so we don't need to worry about the lock. */
// XXX assert(_Py_IsMainInterpreter(_PyInterpreterState_GET())); _Py_hashtable_destroy(EXTENSIONS.hashtable);
Py_CLEAR(EXTENSIONS.dict); EXTENSIONS.hashtable = NULL;
_PyThreadState_ClearDetached(&EXTENSIONS.main_tstate);
} }
#undef HTSEP
static bool static bool
check_multi_interp_extensions(PyInterpreterState *interp) check_multi_interp_extensions(PyInterpreterState *interp)
@ -1231,6 +1253,8 @@ import_find_extension(PyThreadState *tstate, PyObject *name,
PyObject *m_copy = def->m_base.m_copy; PyObject *m_copy = def->m_base.m_copy;
/* Module does not support repeated initialization */ /* Module does not support repeated initialization */
if (m_copy == NULL) { if (m_copy == NULL) {
/* It might be a core module (e.g. sys & builtins),
for which we don't set m_copy. */
m_copy = get_core_module_dict(tstate->interp, name, filename); m_copy = get_core_module_dict(tstate->interp, name, filename);
if (m_copy == NULL) { if (m_copy == NULL) {
return NULL; return NULL;
@ -1300,9 +1324,7 @@ clear_singlephase_extension(PyInterpreterState *interp,
} }
/* Clear the cached module def. */ /* Clear the cached module def. */
if (_extensions_cache_delete(filename, name) < 0) { _extensions_cache_delete(filename, name);
return -1;
}
return 0; return 0;
} }
@ -3051,6 +3073,8 @@ void
_PyImport_Fini(void) _PyImport_Fini(void)
{ {
/* Destroy the database used by _PyImport_{Fixup,Find}Extension */ /* Destroy the database used by _PyImport_{Fixup,Find}Extension */
// XXX Should we actually leave them (mostly) intact, since we don't
// ever dlclose() the module files?
_extensions_cache_clear_all(); _extensions_cache_clear_all();
/* Use the same memory allocator as _PyImport_Init(). */ /* Use the same memory allocator as _PyImport_Init(). */
@ -3088,10 +3112,6 @@ _PyImport_Fini2(void)
PyStatus PyStatus
_PyImport_InitCore(PyThreadState *tstate, PyObject *sysmod, int importlib) _PyImport_InitCore(PyThreadState *tstate, PyObject *sysmod, int importlib)
{ {
if (_Py_IsMainInterpreter(tstate->interp)) {
_extensions_cache_init();
}
// XXX Initialize here: interp->modules and interp->import_func. // XXX Initialize here: interp->modules and interp->import_func.
// XXX Initialize here: sys.modules and sys.meta_path. // XXX Initialize here: sys.modules and sys.meta_path.

69
Python/pystate.c
View File

@ -1640,75 +1640,6 @@ _PyThreadState_DeleteExcept(PyThreadState *tstate)
} }
//-------------------------
// "detached" thread states
//-------------------------
void
_PyThreadState_InitDetached(PyThreadState *tstate, PyInterpreterState *interp)
{
_PyRuntimeState *runtime = interp->runtime;
HEAD_LOCK(runtime);
interp->threads.next_unique_id += 1;
uint64_t id = interp->threads.next_unique_id;
HEAD_UNLOCK(runtime);
init_threadstate(tstate, interp, id);
// We do not call add_threadstate().
}
void
_PyThreadState_ClearDetached(PyThreadState *tstate)
{
assert(!tstate->_status.bound);
assert(!tstate->_status.bound_gilstate);
assert(tstate->datastack_chunk == NULL);
assert(tstate->thread_id == 0);
assert(tstate->native_thread_id == 0);
assert(tstate->next == NULL);
assert(tstate->prev == NULL);
PyThreadState_Clear(tstate);
clear_datastack(tstate);
}
void
_PyThreadState_BindDetached(PyThreadState *tstate)
{
assert(!_Py_IsMainInterpreter(
current_fast_get(tstate->interp->runtime)->interp));
assert(_Py_IsMainInterpreter(tstate->interp));
bind_tstate(tstate);
/* Unlike _PyThreadState_Bind(), we do not modify gilstate TSS. */
}
void
_PyThreadState_UnbindDetached(PyThreadState *tstate)
{
assert(!_Py_IsMainInterpreter(
current_fast_get(tstate->interp->runtime)->interp));
assert(_Py_IsMainInterpreter(tstate->interp));
assert(tstate_is_alive(tstate));
assert(!tstate->_status.active);
assert(gilstate_tss_get(tstate->interp->runtime) != tstate);
unbind_tstate(tstate);
/* This thread state may be bound/unbound repeatedly,
so we must erase evidence that it was ever bound (or unbound). */
tstate->_status.bound = 0;
tstate->_status.unbound = 0;
/* We must fully unlink the thread state from any OS thread,
to allow it to be bound more than once. */
tstate->thread_id = 0;
#ifdef PY_HAVE_THREAD_NATIVE_ID
tstate->native_thread_id = 0;
#endif
}
//---------- //----------
// accessors // accessors
//---------- //----------