gh-150505: fix data race in Unpickler_set_memo under free-threading#150550
Open
AhmedMagdy9876 wants to merge 1 commit into
Open
gh-150505: fix data race in Unpickler_set_memo under free-threading#150550AhmedMagdy9876 wants to merge 1 commit into
AhmedMagdy9876 wants to merge 1 commit into
Conversation
4f2960a to
be6a4a2
Compare
…ding Unpickler_set_memo modified self->memo and self->memo_size without any critical section, allowing concurrent threads to race on the same Unpickler instance. Wrap the memo population loop and the pointer swap in Py_BEGIN_CRITICAL_SECTION/Py_END_CRITICAL_SECTION to serialize access.
be6a4a2 to
194ca8e
Compare
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.
Problem
Unpickler_set_memomodifiedself->memoandself->memo_sizewithout any critical section. Under free-threading (--disable-gil), two threads assigning to.memoon the sameUnpicklerinstance concurrently could race: one thread could setself->memo = NULLwhile another was still indexing into it via_Unpickler_MemoPut, causing a NULL-pointer dereference and segfault.TSAN confirmed the race:
Fix
_Unpickler_MemoPutcalls) in a singlePy_BEGIN_CRITICAL_SECTION/Py_END_CRITICAL_SECTIONonself, so the entire assignment is atomic with respect to other threadsself->memoandself->memo_sizeunder the same critical sectionPy_XDECREFcan trigger arbitrary Python code and should not be called while holding the lockTesting
Verified with a TSAN build (
--disable-gil+-fsanitize=thread) using the reproducer from the issue. TSAN no longer reports a data race after this fix. Added a multithreaded test toLib/test/test_pickle.pyto catch regressions.Fixes #150505
_Unpickler_MemoPut(_pickle.c) fromUnpickler.memoassignment with free-threading #150505