Skip to content

gh-154928: Fix data race on the recursion count of threading.RLock - #155381

Open
deadlovelll wants to merge 1 commit into
python:mainfrom
deadlovelll:gh-154928-rlock-level
Open

gh-154928: Fix data race on the recursion count of threading.RLock#155381
deadlovelll wants to merge 1 commit into
python:mainfrom
deadlovelll:gh-154928-rlock-level

Conversation

@deadlovelll

Copy link
Copy Markdown
Contributor

Fix data race on the recursion count of threading.RLock

For more details see gh-154928

@deadlovelll

deadlovelll commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

I want to give a little disclaimer about my solution, maybe it would answer the possible questions during the review.

At the first attempt i tried to change type of _PyRecursiveMutex's level from size_t to Py_ssize_t. At first look that was nice solution, Py_ssize_t have atomics macro, but! size_t is unsigned type, Py_ssize_t is signed.

where it shoots into the knee:

import _thread
r = _thread.RLock()
r._acquire_restore((0, _thread.get_ident()))
r.release()

When we execute this code with Py_ssize_t version we will get the assertion error:

    Assertion failed: (m->level == 0), function _PyRecursiveMutex_TryUnlock,
    file lock.c, line 466.

Because in _thread_RLock__acquire_restore_impl count - 1 stores at the self->lock.level, our count is 0, so we got -1, size_t cant be -1 and it transforms to 18446744073709551615. In Py_ssize_t solution it will be -1

Then in _PyRecursiveMutex_TryUnlock in size_t solution we will end a the check if (m->level > 0), in Py_ssize_t we will go to the assert and then fail

int
_PyRecursiveMutex_TryUnlock(_PyRecursiveMutex *m)
{
    PyThread_ident_t thread = PyThread_get_thread_ident_ex();
    if (!recursive_mutex_is_owned_by(m, thread)) {
        return -1;
    }
    if (m->level > 0) {
        FT_ATOMIC_STORE_SIZE_RELAXED(m->level, m->level - 1);
        return 0;
    }
    assert(m->level == 0);
    _Py_atomic_store_ullong_relaxed(&m->thread, 0);
    PyMutex_Unlock(&m->mutex);
    return 0;
}

So in this fix I preferred to be conservative and keep the current semantics, despite the bigger diff.

My first version - deadlovelll@c2f316e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant