Skip to content

gh-150157: fix: critical section for PyDict_Next in _pickle.c#150158

Open
KowalskiThomas wants to merge 6 commits into
python:mainfrom
KowalskiThomas:kowalski/fix-pickle-whichmodule-has-a-uaf-on-free-threading
Open

gh-150157: fix: critical section for PyDict_Next in _pickle.c#150158
KowalskiThomas wants to merge 6 commits into
python:mainfrom
KowalskiThomas:kowalski/fix-pickle-whichmodule-has-a-uaf-on-free-threading

Conversation

@KowalskiThomas
Copy link
Copy Markdown
Contributor

@KowalskiThomas KowalskiThomas commented May 20, 2026

@KowalskiThomas KowalskiThomas changed the title fix: critical section for PyDict_Next in _pickle.c gh-150157: fix: critical section for PyDict_Next in _pickle.c May 20, 2026
Comment thread Modules/_pickle.c
@KowalskiThomas KowalskiThomas marked this pull request as ready for review May 21, 2026 18:50
@KowalskiThomas KowalskiThomas requested a review from vstinner May 22, 2026 07:01
@@ -0,0 +1,2 @@
Fix a heap use-after-free in :mod:`pickle`\'s ``whichmodule`` on
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whichmodule is an internal detail not visible to users. Describe the cause in general words, like it looks from user's point.

Copy link
Copy Markdown
Contributor Author

@KowalskiThomas KowalskiThomas May 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does the following sound? (I pushed it for now.)

Fix a potential crash occurring when pickling objects
concurrently in free-threaded builds.

Tried to make it clear from and end-user perspective while not sounding too scary (since it's a rare race condition), hence the potential.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not just a matter of luck. There have to be several conditions for this to happen. First, we must pickle by name an object without the __module__ attribute. Second, sys.modules should be concurrently modified (in a specific way, but we will leave this).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I feared this might be a little bit too detail-y. I'll rephrase it again 😅

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to this.

Fix a crash in free-threaded builds that occurs when pickling
an object without a ``__module__`` attribute while :data:`sys.modules`
is concurrently being modified.

@KowalskiThomas KowalskiThomas force-pushed the kowalski/fix-pickle-whichmodule-has-a-uaf-on-free-threading branch from eb71e46 to 88e8cb8 Compare May 30, 2026 15:26
@KowalskiThomas KowalskiThomas force-pushed the kowalski/fix-pickle-whichmodule-has-a-uaf-on-free-threading branch from 88e8cb8 to 3981cfc Compare May 30, 2026 15:28
Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, this is not correct, most objects do not have such issue even without having __module__, only objects pickled by name (some singletons, named constants or enum-like objects). But if you think that the current wording is enough, we can leave it.

@KowalskiThomas
Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka Right... Thanks for bearing with me; I've updated it again and I think it should be clear and accurate now 😅

Latest version is:

Fix a crash in free-threaded builds that occurs when pickling by name
objects without a ``__module__`` attribute while :data:`sys.modules`
is concurrently being modified.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing critical section for PyDict_Next in _pickle.c

3 participants