gh-154916: Fix data race in ga_iter_reduce under free-threading - #154944
Open
tekinertekin wants to merge 2 commits into
Open
gh-154916: Fix data race in ga_iter_reduce under free-threading#154944tekinertekin wants to merge 2 commits into
tekinertekin wants to merge 2 commits into
Conversation
ga_iternext takes gi->obj out with an atomic exchange and then drops the reference, while ga_iter_reduce read the same field twice without synchronisation. The two reads can straddle the exchange, so the guard can observe a non-NULL pointer that is then passed to Py_BuildValue after the owning thread has already released it. Take a single strong reference instead, and mark the stored object as maybe-weakref in ga_iter, which _Py_XGetRef requires of the writer.
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.
Issue: #154916
ga_iternexttakesgi->objout with_Py_atomic_exchange_ptrand then drops thereference, while
ga_iter_reducereads the same field twice without synchronisation:The two reads can straddle that exchange, so the guard can observe a non-NULL
pointer that is then passed to
Py_BuildValueafter the owning thread hasalready called
Py_DECREFon it. That makes this a potential use-after-freerather than only a torn read. ThreadSanitizer trips on the guard read first,
which is what the issue reports.
The fix takes a single strong reference instead:
_Py_XGetRefin the free-threaded build, plainPy_XNewRefotherwise, with"N(N)"soPy_BuildValueconsumes the reference we now own._Py_XGetRefdocuments that the writer must set maybe-weakref on the storedobject, otherwise its try-incref can never succeed from another thread and it
spins.
ga_iterstores with a plainPy_NewRef, so the writer side needs_PyObject_SetMaybeWeakref.Racing with
next()may legitimately observe either the object or the exhaustediterator; both reductions are correct and that does not change here.
Verification
Built on arm64 macOS with
--disable-gil --with-thread-sanitizer --with-pydebug.Reproducer: 4 threads calling
next()and 4 calling__reduce__()on one sharediter(list[int]).ga_iternext:946↔ga_iter_reduce:1000, both orderingstest_genericalias,test_types,test_typing: 913 tests pass.test_genericalias test_types -R 3:3: no leaks. TheO→Nchange movesownership of the reference, so this seemed worth checking explicitly.
__reduce__output and pickle round-trip are unchanged for both a live and anexhausted iterator.
One open question
I kept this lock-free to stay symmetric with
ga_iternextas gh-154108 left it.The alternative is the
list_item_implpattern — a critical section plus_Py_NewRefWithLock— but that only synchronises ifga_iternexttakes the locktoo, which would mean revising the design just merged in gh-154108.
_Py_XGetRefhas no other call site in the tree, so if you would rather have thecritical-section version, I am happy to redo it that way.
types.GenericAliasiterator inga_iter_reduceunder free-threading (follow-up to gh-154043) #154916