FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-127716: Fix data race in `memoryview.release()` by weixlu · Pull Request #154770 · python/cpython · GitHub

/ cpython Public

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

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

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

Conversation

weixlu commented Jul 27, 2026
edited by bedevere-app Bot
Loading

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) --exports → 0, 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.

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.

weixlu commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

This will be fix in #156248

weixlu closed this Aug 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL