[3.13] gh-117657: Fix TSAN race involving import lock (GH-118523) by miss-islington · Pull Request #120169 · python/cpython · GitHub
Skip to content

[3.13] gh-117657: Fix TSAN race involving import lock (GH-118523) #120169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
colesbury merged 1 commit into from
Jun 6, 2024
Merged

[3.13] gh-117657: Fix TSAN race involving import lock (GH-118523) #120169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 2 additions & 16 deletions Include/internal/pycore_import.h
View file
Open in desktop
12 changes: 12 additions & 0 deletions Include/internal/pycore_lock.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,18 @@ _PyOnceFlag_CallOnce(_PyOnceFlag *flag, _Py_once_fn_t *fn, void *arg)
return _PyOnceFlag_CallOnceSlow(flag, fn, arg);
}

// A recursive mutex. The mutex should zero-initialized.
typedef struct {
PyMutex mutex;
unsigned long long thread; // i.e., PyThread_get_thread_ident_ex()
size_t level;
} _PyRecursiveMutex;

PyAPI_FUNC(int) _PyRecursiveMutex_IsLockedByCurrentThread(_PyRecursiveMutex *m);
PyAPI_FUNC(void) _PyRecursiveMutex_Lock(_PyRecursiveMutex *m);
PyAPI_FUNC(void) _PyRecursiveMutex_Unlock(_PyRecursiveMutex *m);


// A readers-writer (RW) lock. The lock supports multiple concurrent readers or
// a single writer. The lock is write-preferring: if a writer is waiting while
// the lock is read-locked then, new readers will be blocked. This avoids
Expand Down
25 changes: 25 additions & 0 deletions Modules/_testinternalcapi/test_lock.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include "parts.h"
#include "pycore_lock.h"
#include "pycore_pythread.h" // PyThread_get_thread_ident_ex()

#include "clinic/test_lock.c.h"

Expand Down Expand Up @@ -476,6 +477,29 @@ test_lock_rwlock(PyObject *self, PyObject *obj)
Py_RETURN_NONE;
}

static PyObject *
test_lock_recursive(PyObject *self, PyObject *obj)
{
_PyRecursiveMutex m = (_PyRecursiveMutex){0};
assert(!_PyRecursiveMutex_IsLockedByCurrentThread(&m));

_PyRecursiveMutex_Lock(&m);
assert(m.thread == PyThread_get_thread_ident_ex());
assert(PyMutex_IsLocked(&m.mutex));
assert(m.level == 0);

_PyRecursiveMutex_Lock(&m);
assert(m.level == 1);
_PyRecursiveMutex_Unlock(&m);

_PyRecursiveMutex_Unlock(&m);
assert(m.thread == 0);
assert(!PyMutex_IsLocked(&m.mutex));
assert(m.level == 0);

Py_RETURN_NONE;
}

static PyMethodDef test_methods[] = {
{"test_lock_basic", test_lock_basic, METH_NOARGS},
{"test_lock_two_threads", test_lock_two_threads, METH_NOARGS},
Expand All @@ -485,6 +509,7 @@ static PyMethodDef test_methods[] = {
{"test_lock_benchmark", test_lock_benchmark, METH_NOARGS},
{"test_lock_once", test_lock_once, METH_NOARGS},
{"test_lock_rwlock", test_lock_rwlock, METH_NOARGS},
{"test_lock_recursive", test_lock_recursive, METH_NOARGS},
{NULL, NULL} /* sentinel */
};

Expand Down
11 changes: 2 additions & 9 deletions Modules/posixmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
#include "pycore_call.h" // _PyObject_CallNoArgs()
#include "pycore_ceval.h" // _PyEval_ReInitThreads()
#include "pycore_fileutils.h" // _Py_closerange()
#include "pycore_import.h" // _PyImport_ReInitLock()
#include "pycore_initconfig.h" // _PyStatus_EXCEPTION()
#include "pycore_long.h" // _PyLong_IsNegative()
#include "pycore_moduleobject.h" // _PyModule_GetState()
Expand Down Expand Up @@ -627,10 +626,7 @@ PyOS_AfterFork_Parent(void)
_PyEval_StartTheWorldAll(&_PyRuntime);

PyInterpreterState *interp = _PyInterpreterState_GET();
if (_PyImport_ReleaseLock(interp) <= 0) {
Py_FatalError("failed releasing import lock after fork");
}

_PyImport_ReleaseLock(interp);
run_at_forkers(interp->after_forkers_parent, 0);
}

Expand Down Expand Up @@ -675,10 +671,7 @@ PyOS_AfterFork_Child(void)
_PyEval_StartTheWorldAll(&_PyRuntime);
_PyThreadState_DeleteList(list);

status = _PyImport_ReInitLock(tstate->interp);
if (_PyStatus_EXCEPTION(status)) {
goto fatal_error;
}
_PyImport_ReleaseLock(tstate->interp);

_PySignal_AfterFork();

Expand Down
83 changes: 7 additions & 76 deletions Python/import.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,7 @@ static struct _inittab *inittab_copy = NULL;
(interp)->imports.import_func

#define IMPORT_LOCK(interp) \
(interp)->imports.lock.mutex
#define IMPORT_LOCK_THREAD(interp) \
(interp)->imports.lock.thread
#define IMPORT_LOCK_LEVEL(interp) \
(interp)->imports.lock.level
(interp)->imports.lock

#define FIND_AND_LOAD(interp) \
(interp)->imports.find_and_load
Expand All @@ -115,74 +111,14 @@ static struct _inittab *inittab_copy = NULL;
void
_PyImport_AcquireLock(PyInterpreterState *interp)
{
unsigned long me = PyThread_get_thread_ident();
if (me == PYTHREAD_INVALID_THREAD_ID)
return; /* Too bad */
if (IMPORT_LOCK(interp) == NULL) {
IMPORT_LOCK(interp) = PyThread_allocate_lock();
if (IMPORT_LOCK(interp) == NULL)
return; /* Nothing much we can do. */
}
if (IMPORT_LOCK_THREAD(interp) == me) {
IMPORT_LOCK_LEVEL(interp)++;
return;
}
if (IMPORT_LOCK_THREAD(interp) != PYTHREAD_INVALID_THREAD_ID ||
!PyThread_acquire_lock(IMPORT_LOCK(interp), 0))
{
PyThreadState *tstate = PyEval_SaveThread();
PyThread_acquire_lock(IMPORT_LOCK(interp), WAIT_LOCK);
PyEval_RestoreThread(tstate);
}
assert(IMPORT_LOCK_LEVEL(interp) == 0);
IMPORT_LOCK_THREAD(interp) = me;
IMPORT_LOCK_LEVEL(interp) = 1;
_PyRecursiveMutex_Lock(&IMPORT_LOCK(interp));
}

int
void
_PyImport_ReleaseLock(PyInterpreterState *interp)
{
unsigned long me = PyThread_get_thread_ident();
if (me == PYTHREAD_INVALID_THREAD_ID || IMPORT_LOCK(interp) == NULL)
return 0; /* Too bad */
if (IMPORT_LOCK_THREAD(interp) != me)
return -1;
IMPORT_LOCK_LEVEL(interp)--;
assert(IMPORT_LOCK_LEVEL(interp) >= 0);
if (IMPORT_LOCK_LEVEL(interp) == 0) {
IMPORT_LOCK_THREAD(interp) = PYTHREAD_INVALID_THREAD_ID;
PyThread_release_lock(IMPORT_LOCK(interp));
}
return 1;
}

#ifdef HAVE_FORK
/* This function is called from PyOS_AfterFork_Child() to ensure that newly
created child processes do not share locks with the parent.
We now acquire the import lock around fork() calls but on some platforms
(Solaris 9 and earlier? see isue7242) that still left us with problems. */
PyStatus
_PyImport_ReInitLock(PyInterpreterState *interp)
{
if (IMPORT_LOCK(interp) != NULL) {
if (_PyThread_at_fork_reinit(&IMPORT_LOCK(interp)) < 0) {
return _PyStatus_ERR("failed to create a new lock");
}
}

if (IMPORT_LOCK_LEVEL(interp) > 1) {
/* Forked as a side effect of import */
unsigned long me = PyThread_get_thread_ident();
PyThread_acquire_lock(IMPORT_LOCK(interp), WAIT_LOCK);
IMPORT_LOCK_THREAD(interp) = me;
IMPORT_LOCK_LEVEL(interp)--;
} else {
IMPORT_LOCK_THREAD(interp) = PYTHREAD_INVALID_THREAD_ID;
IMPORT_LOCK_LEVEL(interp) = 0;
}
return _PyStatus_OK();
_PyRecursiveMutex_Unlock(&IMPORT_LOCK(interp));
}
#endif


/***************/
Expand Down Expand Up @@ -4111,11 +4047,6 @@ _PyImport_FiniCore(PyInterpreterState *interp)
PyErr_FormatUnraisable("Exception ignored on clearing sys.modules");
}

if (IMPORT_LOCK(interp) != NULL) {
PyThread_free_lock(IMPORT_LOCK(interp));
IMPORT_LOCK(interp) = NULL;
}

_PyImport_ClearCore(interp);
}

Expand Down Expand Up @@ -4248,8 +4179,7 @@ _imp_lock_held_impl(PyObject *module)
/*[clinic end generated code: output=8b89384b5e1963fc input=9b088f9b217d9bdf]*/
{
PyInterpreterState *interp = _PyInterpreterState_GET();
return PyBool_FromLong(
IMPORT_LOCK_THREAD(interp) != PYTHREAD_INVALID_THREAD_ID);
return PyBool_FromLong(PyMutex_IsLocked(&IMPORT_LOCK(interp).mutex));
}

/*[clinic input]
Expand Down Expand Up @@ -4283,11 +4213,12 @@ _imp_release_lock_impl(PyObject *module)
/*[clinic end generated code: output=7faab6d0be178b0a input=934fb11516dd778b]*/
{
PyInterpreterState *interp = _PyInterpreterState_GET();
if (_PyImport_ReleaseLock(interp) < 0) {
if (!_PyRecursiveMutex_IsLockedByCurrentThread(&IMPORT_LOCK(interp))) {
PyErr_SetString(PyExc_RuntimeError,
"not holding the import lock");
return NULL;
}
_PyImport_ReleaseLock(interp);
Py_RETURN_NONE;
}

Expand Down
42 changes: 42 additions & 0 deletions Python/lock.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,48 @@ _PyOnceFlag_CallOnceSlow(_PyOnceFlag *flag, _Py_once_fn_t *fn, void *arg)
}
}

static int
recursive_mutex_is_owned_by(_PyRecursiveMutex *m, PyThread_ident_t tid)
{
return _Py_atomic_load_ullong_relaxed(&m->thread) == tid;
}

int
_PyRecursiveMutex_IsLockedByCurrentThread(_PyRecursiveMutex *m)
{
return recursive_mutex_is_owned_by(m, PyThread_get_thread_ident_ex());
}

void
_PyRecursiveMutex_Lock(_PyRecursiveMutex *m)
{
PyThread_ident_t thread = PyThread_get_thread_ident_ex();
if (recursive_mutex_is_owned_by(m, thread)) {
m->level++;
return;
}
PyMutex_Lock(&m->mutex);
_Py_atomic_store_ullong_relaxed(&m->thread, thread);
assert(m->level == 0);
}

void
_PyRecursiveMutex_Unlock(_PyRecursiveMutex *m)
{
PyThread_ident_t thread = PyThread_get_thread_ident_ex();
if (!recursive_mutex_is_owned_by(m, thread)) {
Py_FatalError("unlocking a recursive mutex that is not owned by the"
" current thread");
}
if (m->level > 0) {
m->level--;
return;
}
assert(m->level == 0);
_Py_atomic_store_ullong_relaxed(&m->thread, 0);
PyMutex_Unlock(&m->mutex);
}

#define _Py_WRITE_LOCKED 1
#define _PyRWMutex_READER_SHIFT 2
#define _Py_RWMUTEX_MAX_READERS (UINTPTR_MAX >> _PyRWMutex_READER_SHIFT)
Expand Down
4 changes: 0 additions & 4 deletions Tools/tsan/suppressions_free_threading.txt
View file
Open in desktop