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

effigies · 2024-01-31T03:43:04Z

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

Lib/importlib/util.py

Lib/test/test_importlib/test_lazy.py

effigies · 2024-02-12T16:57:46Z

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

brettcannon · 2024-02-13T00:29:17Z

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

effigies · 2024-02-13T00:34:15Z

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

brettcannon

Some minor tweaks, but otherwise LGTM!

Lib/importlib/util.py

Lib/test/test_importlib/test_lazy.py

bedevere-app · 2024-02-16T23:34:44Z

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>

effigies · 2024-02-17T00:43:01Z

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

aisk · 2024-02-17T05:16:44Z

Lib/importlib/util.py

@@ -244,5 +263,7 @@ def exec_module(self, module):
        loader_state = {}
        loader_state['__dict__'] = module.__dict__.copy()
        loader_state['__class__'] = module.__class__
+        loader_state['lock'] = threading.RLock()
+        loader_state['is_loading'] = threading.Event()


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.

brettcannon · 2024-02-23T22:47:46Z

!buildbot wasi

bedevere-bot · 2024-02-23T22:47:49Z

🤖 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

miss-islington-app · 2024-02-24T00:02:19Z

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>

bedevere-app · 2024-02-24T00:02:27Z

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>

bedevere-app · 2024-02-24T00:02:33Z

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.

hugovk · 2024-03-23T10:16:31Z

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

Lib/importlib/util.py Show resolved Hide resolved

Lib/importlib/util.py Outdated Show resolved Hide resolved

Lib/importlib/util.py Outdated Show resolved Hide resolved

Lib/test/test_importlib/test_lazy.py Show resolved Hide resolved

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

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 self-requested a review February 23, 2024 19:34

Merge branch 'main' into fix-issue-114763

023d65d

brettcannon added needs backport to 3.11 only security fixes needs backport to 3.12 bug and 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
35 of 40 checks passed

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

bedevere-app bot removed the needs backport to 3.12 bug and 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

effigies commented Jan 31, 2024 •

edited by bedevere-app bot

Loading

effigies commented Feb 12, 2024

brettcannon commented Feb 13, 2024

effigies commented Feb 13, 2024

brettcannon left a comment

bedevere-app bot commented Feb 16, 2024

effigies commented Feb 17, 2024 •

edited

Loading

aisk Feb 17, 2024

effigies Feb 17, 2024

brettcannon commented Feb 23, 2024

bedevere-bot commented Feb 23, 2024

miss-islington-app bot commented Feb 24, 2024

bedevere-app bot commented Feb 24, 2024

bedevere-app bot commented Feb 24, 2024

hugovk commented Mar 23, 2024

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

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

Conversation

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

effigies commented Feb 12, 2024

brettcannon commented Feb 13, 2024

effigies commented Feb 13, 2024

brettcannon left a comment

Choose a reason for hiding this comment

bedevere-app bot commented Feb 16, 2024

effigies commented Feb 17, 2024 • edited Loading

aisk Feb 17, 2024

Choose a reason for hiding this comment

effigies Feb 17, 2024

Choose a reason for hiding this comment

brettcannon commented Feb 23, 2024

bedevere-bot commented Feb 23, 2024

miss-islington-app bot commented Feb 24, 2024

bedevere-app bot commented Feb 24, 2024

bedevere-app bot commented Feb 24, 2024

hugovk commented Mar 23, 2024

effigies commented Jan 31, 2024 •

edited by bedevere-app bot

Loading

effigies commented Feb 17, 2024 •

edited

Loading