gh-155606: Increment the managed buffer export count atomically - #155882
gh-155606: Increment the managed buffer export count atomically#155882ayaangazali 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: |
BHUVANSH855
left a comment
BHUVANSH855
left a comment
There was a problem hiding this comment.
CLA test failing, kindly sign CLA so that maintainers can proceed with review process.
|
Thanks for the nudge. The CLA is with the account owner and is being sorted out, I'll comment here once it clears so nobody spends review time on a blocked PR. One note on the other red check while this sits: |
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 :)