Skip to content

gh-150505: fix data race in Unpickler_set_memo under free-threading#150550

Open
AhmedMagdy9876 wants to merge 4 commits into
python:mainfrom
AhmedMagdy9876:gh-150505-fix-unpickler-memo-race
Open

gh-150505: fix data race in Unpickler_set_memo under free-threading#150550
AhmedMagdy9876 wants to merge 4 commits into
python:mainfrom
AhmedMagdy9876:gh-150505-fix-unpickler-memo-race

Conversation

@AhmedMagdy9876
Copy link
Copy Markdown

@AhmedMagdy9876 AhmedMagdy9876 commented May 28, 2026 •
edited by bedevere-app Bot
Loading

Problem

Unpickler_set_memo modified self->memo and self->memo_size without any critical section. Under free-threading (--disable-gil), two threads assigning to .memo on the same Unpickler instance concurrently could race: one thread could set self->memo = NULL while another was still indexing into it via _Unpickler_MemoPut, causing a NULL-pointer dereference and segfault.

TSAN confirmed the race:

WARNING: ThreadSanitizer: data race
  Read of size 8 by thread T2:
    #0 _Unpickler_MemoPut Modules/_pickle.c:1631
  Previous write of size 8 by thread T1:
    #0 Unpickler_set_memo Modules/_pickle.c:7816

Fix

  • Wrap the memo population loop (all _Unpickler_MemoPut calls) in a single Py_BEGIN_CRITICAL_SECTION / Py_END_CRITICAL_SECTION on self, so the entire assignment is atomic with respect to other threads
  • Swap self->memo and self->memo_size under the same critical section
  • Free the old memo array and decref its contents outside the lock, since Py_XDECREF can trigger arbitrary Python code and should not be called while holding the lock

Testing

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 to Lib/test/test_pickle.py to catch regressions.

Fixes #150505

@bedevere-app bedevere-app Bot mentioned this pull request May 28, 2026
Open
@AhmedMagdy9876 AhmedMagdy9876 force-pushed the gh-150505-fix-unpickler-memo-race branch 3 times, most recently from 194ca8e to 486a6e4 Compare May 28, 2026 10:03
@picnixz
Copy link
Copy Markdown
Member

picnixz commented May 28, 2026

  1. Don't force push. You should have read https://devguide.python.org/getting-started/pull-request-lifecycle/.
  2. Don't update the branch as well unless CI is failing.
  3. How much of an AI was used?

pythongh-150505: fix data race in Unpickler_set_memo under free-threa…
61839ca 
…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.
@AhmedMagdy9876 AhmedMagdy9876 force-pushed the gh-150505-fix-unpickler-memo-race branch from 486a6e4 to 61839ca Compare May 28, 2026 10:19
@picnixz
Copy link
Copy Markdown
Member

picnixz commented May 28, 2026

I really feel getting ignored. Please read the contributions guidelines of our project first.

Ahmed Magdy Ali added 2 commits May 29, 2026 00:28
pythongh-150505: Revert "fix data race in Unpickler_set_memo under fr…
6f465f9 
…ee-threading"
pythongh-150505: document that Pickler and Unpickler are not thread-safe
aef265f 
Sharing Pickler or Unpickler instances across threads has no defined
semantics. The internal state (memo, stream position, stack) forms a
single coherent parse/serialize state that cannot be meaningfully
used across threads simultaneously.
@read-the-docs-community
Copy link
Copy Markdown

read-the-docs-community Bot commented May 28, 2026 •
edited
Loading

Documentation build overview

📚 cpython-previews | 🛠️ Build #32894593 | 📁 Comparing a540d41 against main (baf11a4)

  🔍 Preview build  

1 file changed
± library/pickle.html

@bedevere-app
Copy link
Copy Markdown

bedevere-app Bot commented May 28, 2026

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@AhmedMagdy9876
Copy link
Copy Markdown
Author

AhmedMagdy9876 commented May 28, 2026 •
edited
Loading

@picnixz apologies, never meant to ignore you, genuinely missed your comments and saw them later:

  • repeated force pushes is on me, should have read the contribution guidelines more carefully. the force-pushed changes were just me chasing CI check pass
  • AI used as a learning tool, review code changes, refine docs wording, and generate a scaffold for PR description/commit messages (strictly reviewed by me). Decisions and code changes are mine.
  • As per discussion in the related issue, I reverted the old approach and resorted to only doc note
  • NEWS entry apparently not needed for this one now. Should be labeled with "skip news"

@dpdani
Copy link
Copy Markdown
Contributor

dpdani commented Jun 4, 2026

@AhmedMagdy9876 I see you reverted a change that I assume fixed the segmentation fault, can I ask you why? The documentation change is looking good, but we probably want to prevent the segfault as well.

@AhmedMagdy9876
Copy link
Copy Markdown
Author

AhmedMagdy9876 commented Jun 4, 2026 •
edited
Loading

@dpdani thanks for the feedback. The revert was based on the discussion in the linked issue (#150505). @kumaraditya303 questioned whether sharing Unpickler instances across threads should be a supported use case. A critical section fix would prevent the crash but wouldn't validate shared usage.
reintroducing the fix is easy though if the discussion concludes to something different. honestly I am more inclined to keeping both the critical section fix and the doc note. your take in the issue discussion would be appreciated

@dpdani
Copy link
Copy Markdown
Contributor

dpdani commented Jun 4, 2026

@kumaraditya303 correct me if I'm wrong, but I didn't read your comment as implying to keep the segmentation fault?

I think it makes sense to keep a stance similar to #133885: use locks to prevent segmentation faults, but still allow undefined output. I don't see why would like something different here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Segfault in _Unpickler_MemoPut (_pickle.c) from Unpickler.memo assignment with free-threading

3 participants

@AhmedMagdy9876 @picnixz @dpdani