gh-114763: Protect lazy loading modules from attribute access race by effigies · Pull Request #114781 · python/cpython · GitHub

As described in #114763, setting the __class__ attribute of a lazy-loading module to ModuleType enables other threads to attempt to access attributes before the loading is complete. This PR moves self.__class__ = types.ModuleType to be the final act.

This requires two additional pieces to work:

A lock, followed by a __class__ check to prevent threads that arrive while the module is being loaded from attempting to double-load the module.
An Event to indicate that the load is in progress to the loading thread, so it can permit dunder attribute access to the exec_module() call. This path also requires that the lock be reentrant.

The Event needs to be tied to the specific module, so I also tied the lock to the module, as opposed to a lock scoped to the importlib.util module. I used object.__getattribute__() for __spec__ and __dict__, which were previously accessed directly after self.__class__ was reset.

Otherwise, I tried to keep things as close as possible to the original. I will try to write a unit test fitting with the module style, but the minimal reproduction in the issue is resolved by these changes.

Closes #114763

Issue: Accessing attributes of a lazily-loaded module is not thread-safe #114763

@brettcannon Just checking in if you're waiting on me for anything. (No rush from my end.)

@effigies Nope, I'm just swamped right now, so I haven't had time to do another review yet.

No worries! I appreciate you taking the time, whenever you get it.

Some minor tweaks, but otherwise LGTM!

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Co-authored-by: Brett Cannon <brett@python.org>

@bedevere-bot I have made the requested changes; please review again.

Hi, can we use a bool flag instead of threading.Event? I see that access and modification to loader_state['is_loading'] is protected by the loader_state['lock'], so there is no thread safety issue.

Using an additional threading.Event would introduce unnecessary resource costs.

I agree, good catch. I will switch to a bool and push later today.

!buildbot wasi

🤖 New build scheduled with the buildbot fleet by @brettcannon for commit 023d65d 🤖

The command will test the builders whose names match following regular expression: wasi

The builders matched are:

wasm32-wasi Non-Debug PR
wasm32 WASI 8Core PR
wasm32-wasi PR

Thanks @effigies for the PR, and @brettcannon for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

…aces (pythonGH-114781) Setting the __class__ attribute of a lazy-loading module to ModuleType enables other threads to attempt to access attributes before the loading is complete. Now that is protected by a lock. (cherry picked from commit 200271c) Co-authored-by: Chris Markiewicz <effigies@gmail.com>

GH-115870 is a backport of this pull request to the 3.12 branch.

…aces (pythonGH-114781) Setting the __class__ attribute of a lazy-loading module to ModuleType enables other threads to attempt to access attributes before the loading is complete. Now that is protected by a lock. (cherry picked from commit 200271c) Co-authored-by: Chris Markiewicz <effigies@gmail.com>

GH-115871 is a backport of this pull request to the 3.11 branch.

…races (GH-114781) (GH-115870) gh-114763: Protect lazy loading modules from attribute access races (GH-114781) Setting the __class__ attribute of a lazy-loading module to ModuleType enables other threads to attempt to access attributes before the loading is complete. Now that is protected by a lock. (cherry picked from commit 200271c) Co-authored-by: Chris Markiewicz <effigies@gmail.com>

…races (GH-114781) (GH-115871) gh-114763: Protect lazy loading modules from attribute access races (GH-114781) Setting the __class__ attribute of a lazy-loading module to ModuleType enables other threads to attempt to access attributes before the loading is complete. Now that is protected by a lock. (cherry picked from commit 200271c) Co-authored-by: Chris Markiewicz <effigies@gmail.com>

…aces (pythonGH-114781) Setting the __class__ attribute of a lazy-loading module to ModuleType enables other threads to attempt to access attributes before the loading is complete. Now that is protected by a lock.

Please see #117178 for a potential regression in 3.11 - 3.13.

…aces (pythonGH-114781) Setting the __class__ attribute of a lazy-loading module to ModuleType enables other threads to attempt to access attributes before the loading is complete. Now that is protected by a lock.

effigies requested review from brettcannon, ericsnowcurrently, ncoghlan and warsaw as code owners January 31, 2024 03:43

bedevere-app Bot mentioned this pull request Jan 31, 2024

Accessing attributes of a lazily-loaded module is not thread-safe #114763

Closed

bedevere-app Bot added the awaiting review label Jan 31, 2024

brettcannon reviewed Jan 31, 2024

View reviewed changes

Comment thread Lib/importlib/util.py

Comment thread Lib/importlib/util.py Outdated

Comment thread Lib/importlib/util.py Outdated

Comment thread Lib/test/test_importlib/test_lazy.py

effigies mentioned this pull request Feb 1, 2024

Broken thread safety scientific-python/lazy-loader#88

Closed

effigies added 5 commits February 5, 2024 09:28

pythongh-114763 : Protect lazy loading modules from attribute access race

d57f4eb

TEST: Reproduce pythongh-114763

3db717c

Add new blurb

5424d0c

Improve comments around the branching

96c08c5

TEST: Use TestCase.assert* methods

3642ddc

effigies force-pushed the fix-issue-114763 branch from dded16c to 3642ddc Compare February 5, 2024 14:29

Merge branch 'main' into fix-issue-114763

46238e5

brettcannon self-requested a review February 13, 2024 00:29

brettcannon requested changes Feb 16, 2024

View reviewed changes

Comment thread Lib/importlib/util.py Outdated

Comment thread Lib/importlib/util.py Outdated

Comment thread Lib/importlib/util.py Outdated

Comment thread Lib/importlib/util.py Outdated

Comment thread Lib/test/test_importlib/test_lazy.py

bedevere-app Bot removed the awaiting review label Feb 16, 2024

bedevere-app Bot added the awaiting changes label Feb 16, 2024

effigies and others added 3 commits February 16, 2024 19:32

Apply suggestions from code review

6accf0c

Co-authored-by: Brett Cannon <brett@python.org>

TEST: Require working threading for test_module_load_race

bb2292f

Move threading import to module level of importlib.util

57fe084

aisk reviewed Feb 17, 2024

View reviewed changes

effigies added 2 commits February 17, 2024 10:15

Make is_loading a bool instead of Event

40383a0

Merge branch 'main' into fix-issue-114763

6322dd6

brettcannon added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes type-bug An unexpected behavior, bug, or error topic-importlib labels Feb 23, 2024

brettcannon approved these changes Feb 23, 2024

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting changes labels Feb 23, 2024

brettcannon merged commit 200271c into python:main Feb 24, 2024

bedevere-app Bot removed the awaiting merge label Feb 24, 2024

bedevere-app Bot removed the needs backport to 3.12 only security fixes label Feb 24, 2024

bedevere-app Bot removed the needs backport to 3.11 only security fixes label Feb 24, 2024

effigies deleted the fix-issue-114763 branch February 24, 2024 00:03

rokm mentioned this pull request Mar 23, 2024

3.13.0a5 broke importlib.util.LazyLoader #117178

Closed

aisk mentioned this pull request Mar 23, 2024

gh-117178: Recover lazy loading of self-referential modules #117179

Merged

This was referenced Apr 17, 2024

GH-114781 potentially breaks gevent: threading becomes pre-imported at startup #117983

Closed

gh-117182: Allow lazily loaded modules to modify their own __class__ #117185

Merged

emlys mentioned this pull request Jul 28, 2026

Add pygeoprocessing.include_dir() and lazily import the dependency tree natcap/pygeoprocessing#487

Merged

                       loader_state['__dict__'] = module.__dict__.copy()
                       loader_state['__class__'] = module.__class__
+                      loader_state['lock'] = threading.RLock()
+                      loader_state['is_loading'] = threading.Event()

gh-114763: Protect lazy loading modules from attribute access race by effigies · Pull Request #114781 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-114763: Protect lazy loading modules from attribute access race - #114781

gh-114763: Protect lazy loading modules from attribute access race#114781brettcannon merged 12 commits intopython:mainpython/cpython:mainfrom effigies:fix-issue-114763effigies/cpython:fix-issue-114763Copy head branch name to clipboard

Conversation

effigies commented Jan 31, 2024 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

effigies commented Feb 12, 2024

Uh oh!

brettcannon commented Feb 13, 2024

Uh oh!

effigies commented Feb 13, 2024

Uh oh!

brettcannon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-app Bot commented Feb 16, 2024

Uh oh!

effigies commented Feb 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aisk Feb 17, 2024

Choose a reason for hiding this comment

Uh oh!

effigies Feb 17, 2024

Choose a reason for hiding this comment

Uh oh!

brettcannon commented Feb 23, 2024

Uh oh!

bedevere-bot commented Feb 23, 2024

Uh oh!

miss-islington-app Bot commented Feb 24, 2024

Uh oh!

bedevere-app Bot commented Feb 24, 2024

Uh oh!

bedevere-app Bot commented Feb 24, 2024

Uh oh!

hugovk commented Mar 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gh-114763: Protect lazy loading modules from attribute access race#114781
brettcannon merged 12 commits into
python:mainfrom
effigies:fix-issue-114763

effigies commented Jan 31, 2024 •
edited by bedevere-app Bot
Loading

effigies commented Feb 17, 2024 •
edited
Loading