GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-150505: fix data race in Unpickler_set_memo under free-threading by AhmedMagdy9876 · Pull Request #150550 · python/cpython

Conversation

@AhmedMagdy9876

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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

  • Issue: Segfault in _Unpickler_MemoPut (_pickle.c) from Unpickler.memo assignment with free-threading #150505

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot mentioned this pull request May 28, 2026
Segfault in _Unpickler_MemoPut (_pickle.c) from Unpickler.memo assignment with free-threading #150505
Open
@bedevere-app bedevere-app Bot added the awaiting review label May 28, 2026
@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

picnixz commented May 28, 2026

Copy link
Copy Markdown
Member
  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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Ahmed Magdy Ali
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

picnixz commented May 28, 2026

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Ahmed Magdy Ali added 2 commits May 29, 2026 00:28
Ahmed Magdy Ali
pythongh-150505: Revert "fix data race in Unpickler_set_memo under fr…
6f465f9 
…ee-threading"
Ahmed Magdy Ali
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

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Documentation build overview

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

  🔍 Preview build  

1 file changed
± library/pickle.html
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Ahmed Magdy Ali
pythongh-150505: remove unnecessary NEWS entry for docs-only change
a540d41
@bedevere-app

bedevere-app Bot commented May 28, 2026

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AhmedMagdy9876

AhmedMagdy9876 commented May 28, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

@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"
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dpdani

dpdani commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AhmedMagdy9876

AhmedMagdy9876 commented Jun 4, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dpdani

dpdani commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 90 days with no activity.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review stale Stale PR or inactive for long period of time.

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

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@AhmedMagdy9876 @picnixz @dpdani
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.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.