Skip to content

gh-127716: Fix data race in memoryview.release() - #154770

Open
weixlu wants to merge 1 commit into
python:mainfrom
weixlu:mv
Open

gh-127716: Fix data race in memoryview.release()#154770
weixlu wants to merge 1 commit into
python:mainfrom
weixlu:mv

Conversation

@weixlu

@weixlu weixlu commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

As reported in #127716, on no-gil build, several threads calling memoryview.release() on views of the same buffer will cause problem.

Root cause

Let's take a look at memoryview.release()

static void _memory_release(PyMemoryViewObject *self)
{
    assert(get_exports(self) == 0);
    if (self->flags & _Py_MEMORYVIEW_RELEASED)   // (1) test
        return;

    self->flags |= _Py_MEMORYVIEW_RELEASED;      // (2) set
    assert(self->mbuf->exports > 0);
    if (--self->mbuf->exports == 0) {            // (3) non-atomic decrement
        mbuf_release(self->mbuf);
    }
}

Here "test the flag" (1) and "set the flag" (2) are two separate, unsynchronized operations. When mbuf->exports == 1, two threads race like this:

step thread A thread B
test flag (1) sees not released → continues sees not released → continues
set flag (2) sets RELEASED sets RELEASED (again, no-op)
decrement (3) --exports0, calls mbuf_release() --exports-1

Both threads get past the guard, so both run the decrement. As a result, mbuf->exports drops below 0.

Fix

To fix this, we have to make self->flags's test-and-set a single atomic operation, so that exactly one thread wins the transition and performs the decrement. Here I'm using _Py_atomic_or_uint32. It is used in both release entry points:

  • _memory_release() — This code path belongs to the case "Debug build with ASan" in the original issue.
  • mbuf_release() — This code path belongs to the case "Plain debug build (no ASan)"

Testing

  • The reproducer from the original issue no longer aborts (with and without ASan).
  • python -m test test_memoryview passes (with and without GIL).
  • In the default (with-GIL) build, the atomic ops degrade to the original plain ones, so behavior and performance are unchanged.

@nascheme

Copy link
Copy Markdown
Member

This would need a fair amount of work before it's ready to merge:

  • Need regression test and NEWS entry.
  • mbuf->exports still has non-atomic updates, e.g. mbuf_add_view() and mbuf_add_incomplete_view() use non-atomic increments. Need to check all operations on the counters to ensure they use the correct atomics or locking.
  • Release and buffer acquisition/access need a locking or equivalent state protocol. Right now, a thread checks if the memory view is released, if not, accesses the buffer. Another thread can call release() between those steps. The new atomic operation only prevents two threads from releasing the same view twice.

GH-155882 fixes the missing atomics on the export increments but doesn't fix the last issue.

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.

2 participants