gh-155606: Increment the managed buffer export count atomically - #155882
Open
ayaangazali wants to merge 1 commit into
Open
gh-155606: Increment the managed buffer export count atomically#155882ayaangazali wants to merge 1 commit into
ayaangazali wants to merge 1 commit into
Conversation
mbuf_add_view() and mbuf_add_incomplete_view() registered a new view on the shared _PyManagedBufferObject with a plain mbuf->exports++. On free-threaded builds concurrent slices of one memoryview lose increments, so the count drops to zero while views are still alive and mbuf_release() frees the buffer early. The matching PyMemoryViewObject.exports counter already uses FT_ATOMIC_ADD_SSIZE; use it here too. On default builds the macro expands to a plain +=, so this is a no-op there.
|
The following commit authors need to sign the Contributor License Agreement: |
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
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
memoryviewslicing registers a new view on the shared_PyManagedBufferObjectand bumps its export count. Both registration sites do that with a plain++:https://github.com/python/cpython/blob/main/Objects/memoryobject.c#L699-L703
mbuf_add_view()andmbuf_add_incomplete_view()are reached frommemory_subscript(), so slicing one shared memoryview from several threads is concurrent unsynchronised read-modify-write onmbuf->exports. Increments get lost, the count ends up lower than the number of live views, and dropping those views walks it through zero early.mbuf_release()then frees the buffer while views are still using it, which is theValueError: operation forbidden on released memoryview objectfrom gh-155606. On a debug build it tripsassert(self->mbuf->exports > 0)in_memory_release()first.The sibling counter,
PyMemoryViewObject.exports, is already atomic viaFT_ATOMIC_ADD_SSIZE(added in gh-127085). This does the same for the managed buffer's counter. On default builds the macro expands to a plain+=, so nothing changes there.Verifying
Free-threaded debug build,
--disable-gil --with-pydebug, 8 threads slicing one sharedmemoryview. I built each combination separately and ran each several times:mbuf->exports++fixedSo this is one of two independent races on the same counter and it is not enough on its own. The decrement side in
_memory_release()is already covered by #154770 (open, for gh-127716), and I confirmed that PR does not fix gh-155606 by itself either. No overlapping hunks between the two, they touch different functions. Flagging it because reviewing this diff alone would suggest gh-155606 is closed, and it is not until both land.The added test covers the increment on its own: slices are created concurrently but only dropped afterwards on a single thread, so the decrement race cannot contribute. It aborts on unpatched main and passes with just this change, 5 runs each. It is skipped on default builds.
Also ran
test_memoryview,test_buffer,test_free_threading,test_capion both a normal and a free-threaded build, plus-R 3:3ontest_memoryviewandtest_buffer. All clean.pre-commitpasses on the touched files.One thing I did not do: I have no ThreadSanitizer build here, so the race is evidenced by the assert and the failure counts above rather than a TSan report.
apologies if I've missed something obvious in here, I worked through the logic myself and talked the design decisions over with Claude Code as a sanity check. still a freshman in college so I'm sure there's plenty I don't know yet, and I'd genuinely like the correction if I've got it wrong somewhere :)