◐ Shell
reader mode source ↗
Skip to content

bpo-35943: PyImport_GetModule() can return partially-initialized module#15057

Merged
brettcannon merged 11 commits into
python:masterfrom
nanjekyejoannah:issue35943
Sep 11, 2019
Merged

bpo-35943: PyImport_GetModule() can return partially-initialized module#15057
brettcannon merged 11 commits into
python:masterfrom
nanjekyejoannah:issue35943

Conversation

@nanjekyejoannah

@nanjekyejoannah nanjekyejoannah commented Jul 31, 2019 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

Added optimization to prevent PyImport_GetModule() from returning partially-initialized module.

https://bugs.python.org/issue35943

gnprice
gnprice reviewed Aug 1, 2019
View reviewed changes

@gnprice gnprice left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

This feels like a somewhat tricky bit of logic that it'd be best not to duplicate.

How about pulling it out from PyImport_ImportModuleLevelObject as its own function? I think I'd take the whole body of the if (mod != NULL ...) block. A good description of what it means might be "ensure initialized", so perhaps spelled import_ensure_initialized.

(I'd make it a static function, so it isn't visible outside this file -- that avoids any need to worry about the API.)

Then this function here only needs to add a couple of lines, invoking that one.

gnprice
gnprice reviewed Aug 1, 2019
View reviewed changes

@gnprice gnprice left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

Thanks @nanjekyejoannah for the update! I think this refactor worked out very well.

@nanjekyejoannah

nanjekyejoannah commented Aug 2, 2019

Copy link
Copy Markdown
Contributor Author

@gnprice and @serhiy-storchaka, I have made some changes PTAL.

gnprice
gnprice approved these changes Aug 3, 2019
View reviewed changes

@gnprice gnprice left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

LGTM! Thanks @nanjekyejoannah for this patch, and for all the updates.

@serhiy-storchaka

serhiy-storchaka commented Aug 3, 2019

Copy link
Copy Markdown
Member

LGTM. @ericsnowcurrently, please take a look.

nanjekyejoannah and others added 2 commits August 3, 2019 12:32
@nanjekyejoannah @serhiy-storchaka
Update Misc/NEWS.d/next/Library/2019-07-31-15-52-51.bpo-35943.-KswoB.rst …
6c1c28e 
Co-Authored-By: Serhiy Storchaka <storchaka@gmail.com>
@nanjekyejoannah @serhiy-storchaka
Update Python/import.c …
13f438e 
Co-Authored-By: Serhiy Storchaka <storchaka@gmail.com>
24 hidden items Load more…
@nanjekyejoannah

nanjekyejoannah commented Aug 16, 2019

Copy link
Copy Markdown
Contributor Author

I have made the requested changes @ericsnowcurrently , @serhiy-storchaka , @gnprice PTAL .

@brettcannon

brettcannon commented Aug 23, 2019

Copy link
Copy Markdown
Member

@nanjekyejoannah FYI you forgot to use the phrase "I have made the requested changes; please review again" to tell the bot to flag this PR as ready for re-review. I'll go ahead and do the label change manually.

@nanjekyejoannah

nanjekyejoannah commented Aug 24, 2019

Copy link
Copy Markdown
Contributor Author

@nanjekyejoannah FYI you forgot to use the phrase "I have made the requested changes; please review again" to tell the bot to flag this PR as ready for re-review. I'll go ahead and do the label change manually.

My bad. Thanks for the reminder.

@bedevere-bot

bedevere-bot commented Aug 24, 2019

Copy link
Copy Markdown

Thanks for making the requested changes!

@serhiy-storchaka, @ericsnowcurrently: please review the changes made to this pull request.

@brettcannon brettcannon merged commit 37c2220 into python:master Sep 11, 2019
@nanjekyejoannah nanjekyejoannah deleted the issue35943 branch September 11, 2019 13:06
@vstinner

vstinner commented Sep 11, 2019

Copy link
Copy Markdown
Member

I didn't follow this change closely. When I look at the final merged change, it looks simple. But here I see a long discussion, which makes me understand that writing this change was quite difficult. So, well done Joannah for helping to make Python more correct ✨ 🍰 ✨ ;-)

DinoV pushed a commit to DinoV/cpython that referenced this pull request Sep 12, 2019
@nanjekyejoannah @DinoV
bpo-35943: Prevent PyImport_GetModule() from returning a partially-in… …
a192e8c 
…itialized module (pythonGH-15057)
@miss-islington

miss-islington commented Nov 20, 2019

Copy link
Copy Markdown
Contributor

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

@miss-islington

miss-islington commented Nov 20, 2019

Copy link
Copy Markdown
Contributor

Sorry, @nanjekyejoannah and @brettcannon, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 37c22206981f52ae35c28b39f7530f8438afbfdb 3.7

@mgasner mgasner mentioned this pull request Feb 20, 2020
Closed
websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
@nanjekyejoannah @websurfer5
bpo-35943: Prevent PyImport_GetModule() from returning a partially-in… …
cd30fa0 
…itialized module (pythonGH-15057)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@brettcannon brettcannon brettcannon approved these changes

@encukou encukou Awaiting requested review from encukou

@ncoghlan ncoghlan Awaiting requested review from ncoghlan

@warsaw warsaw Awaiting requested review from warsaw

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently

+1 more reviewer

@gnprice gnprice gnprice approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@brettcannon brettcannon

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@nanjekyejoannah @serhiy-storchaka @bedevere-bot @ericsnowcurrently @brettcannon @vstinner @miss-islington @gnprice @the-knights-who-say-ni