gh-114763: Protect lazy loading modules from attribute access race #114781
Conversation
effigies
requested review from
brettcannon,
ericsnowcurrently,
ncoghlan and
warsaw
as code owners
effigies
force-pushed
the
fix-issue-114763
branch
from
dded16c
to
3642ddc
Compare
|
@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. |
|
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 |
Co-authored-by: Brett Cannon <brett@python.org>
|
@bedevere-bot I have made the requested changes; please review again. |
Lib/importlib/util.py
Outdated
| @@ -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() | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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: The builders matched are:
|
brettcannon
added
needs backport to 3.11
|
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.
As described in #114763, setting the
__class__attribute of a lazy-loading module toModuleTypeenables other threads to attempt to access attributes before the loading is complete. This PR movesself.__class__ = types.ModuleTypeto be the final act.This requires two additional pieces to work:
__class__check to prevent threads that arrive while the module is being loaded from attempting to double-load the module.Eventto indicate that the load is in progress to the loading thread, so it can permit dunder attribute access to theexec_module()call. This path also requires that the lock be reentrant.The
Eventneeds to be tied to the specific module, so I also tied the lock to the module, as opposed to a lock scoped to theimportlib.utilmodule. I usedobject.__getattribute__()for__spec__and__dict__, which were previously accessed directly afterself.__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