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

nanjekyejoannah · 2019-07-31T15:51:06Z

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

https://bugs.python.org/issue35943

Python/import.c

gnprice

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.

… into issue36833

Python/import.c

gnprice

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

gnprice · 2019-08-01T20:44:28Z

Python/import.c

+       int value = import_ensure_initialized(tstate, mod, name);
+       if (value == -1) {
+            remove_importlib_frames(tstate);
+            return NULL;


I'm looking back at what the goto error in the original context does, as a guide to what this error case should likely do.

One is remove_importlib_frames -- great. A couple of other steps there don't appear to apply.

But another one is Py_XDECREF(mod);. And in fact it does look like we should do that here. (If there hadn't been an error, we'd have returned mod to the caller -- which must mean the caller would own the reference, so it must be that we own the reference here. Which means if we return NULL and so don't pass the pointer on to some other code that will own the reference, it's up to us here to decref it away.)

I think that's then all of it.

I'm looking back at what the goto error in the original context does, as a guide to what this error case should likely do.

One is remove_importlib_frames -- great. A couple of other steps there don't appear to apply.

But another one is Py_XDECREF(mod);. And in fact it does look like we should do that here. (If there hadn't been an error, we'd have returned mod to the caller -- which must mean the caller would own the reference, so it must be that we own the reference here. Which means if we return NULL and so don't pass the pointer on to some other code that will own the reference, it's up to us here to decref it away.)

One is remove_importlib_frames -- great. A couple of other steps there don't appear to apply.

Am also returning null following a review from @serhiy-storchaka above. no?

I agree about doing Py_XDECREF(mod). Thanks @gnprice .

gnprice · 2019-08-01T20:45:12Z

Python/import.c

+   mod = import_get_module(tstate, name);
+   if (mod != NULL && mod != Py_None) {
+       int value = import_ensure_initialized(tstate, mod, name);
+       if (value == -1) {


Same thing @serhiy-storchaka suggested at the other call site.

gnprice · 2019-08-01T21:46:02Z

Misc/NEWS.d/next/Library/2019-07-31-15-52-51.bpo-35943.-KswoB.rst

@@ -0,0 +1 @@
+Added optimization to prevent `PyImport_GetModule()` from returning partially-initialized module.


Oh also:

This NEWS entry (and the commit message) say "optimization", but the change is actually better than that! This changes the function's behavior, in a way that makes the API safer and makes it easier to avoid bugs when using it. So let's write the description to make that clear. 🙂

One good description might be:

The function :c:func:`PyImport_GetModule` now ensures any module it returns is fully initialized.

Also I think C API rather than Library would be the most helpful home for the NEWS entry.

nanjekyejoannah · 2019-08-02T14:22:42Z

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

Python/import.c

Misc/NEWS.d/next/Library/2019-07-31-15-52-51.bpo-35943.-KswoB.rst

Python/import.c

gnprice

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

Misc/NEWS.d/next/Library/2019-07-31-15-52-51.bpo-35943.-KswoB.rst

Python/import.c

serhiy-storchaka · 2019-08-03T07:24:37Z

LGTM. @ericsnowcurrently, please take a look.

Co-Authored-By: Serhiy Storchaka <storchaka@gmail.com>

nanjekyejoannah · 2019-08-16T14:07:36Z

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

serhiy-storchaka · 2019-08-18T06:28:15Z

Python/import.c

+       stuffing the new module in sys.modules.
+    */
+    spec = _PyObject_GetAttrId(mod, &PyId___spec__);
+    if (spec == NULL) {


_PyModuleSpec_IsInitializing() was specially designed for convenience, so there is no need to check spec for NULL. You can save 4 lines of code.

Python/import.c

brettcannon · 2019-08-23T23:58:49Z

@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 · 2019-08-24T14:44:46Z

@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 · 2019-08-24T14:44:50Z

Thanks for making the requested changes!

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

vstinner · 2019-09-11T21:30:27Z

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 ✨ 🍰 ✨ ;-)

…itialized module (pythonGH-15057)

miss-islington · 2019-11-20T16:52:36Z

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

miss-islington · 2019-11-20T16:52:41Z

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

…itialized module (pythonGH-15057)

PyImport_GetModule() can return partially-initialized module

c374e0a

nanjekyejoannah requested review from brettcannon, encukou, ericsnowcurrently, ncoghlan and warsaw as code owners July 31, 2019 15:51

the-knights-who-say-ni added the CLA signed label Jul 31, 2019

bedevere-bot added the awaiting review label Jul 31, 2019

📜🤖 Added by blurb_it.

01fe079

serhiy-storchaka reviewed Jul 31, 2019

View reviewed changes

Python/import.c Outdated Show resolved Hide resolved

gnprice reviewed Aug 1, 2019

View reviewed changes

Joannah nanjekye added 2 commits August 1, 2019 11:53

Merge branch 'issue35943' of https://github.com/nanjekyejoannah/cpython…

7d1f7ee

… into issue36833

Isolate duplicate code in a function

f2be697

serhiy-storchaka reviewed Aug 1, 2019

View reviewed changes

Python/import.c Outdated Show resolved Hide resolved

Python/import.c Show resolved Hide resolved

Python/import.c Outdated Show resolved Hide resolved

Python/import.c Outdated Show resolved Hide resolved

gnprice reviewed Aug 1, 2019

View reviewed changes

ensure any module returned is initialized

0925812

serhiy-storchaka reviewed Aug 2, 2019

View reviewed changes

Python/import.c Outdated Show resolved Hide resolved

Python/import.c Outdated Show resolved Hide resolved

Misc/NEWS.d/next/Library/2019-07-31-15-52-51.bpo-35943.-KswoB.rst Outdated Show resolved Hide resolved

Python/import.c Outdated Show resolved Hide resolved

change to Py_DECREF and fix doc

57a6231

gnprice approved these changes Aug 3, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Aug 3, 2019

serhiy-storchaka approved these changes Aug 3, 2019

View reviewed changes

Misc/NEWS.d/next/Library/2019-07-31-15-52-51.bpo-35943.-KswoB.rst Outdated Show resolved Hide resolved

bedevere-bot added awaiting merge and removed awaiting core review labels Aug 3, 2019

serhiy-storchaka reviewed Aug 3, 2019

View reviewed changes

Python/import.c Outdated Show resolved Hide resolved

nanjekyejoannah and others added 2 commits August 3, 2019 12:32

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>

Update Python/import.c

13f438e

Co-Authored-By: Serhiy Storchaka <storchaka@gmail.com>

changes to import_ensure_initialized

fc83ad3

serhiy-storchaka reviewed Aug 18, 2019

View reviewed changes

stop checking for spec

ff771e7

serhiy-storchaka approved these changes Aug 19, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Aug 19, 2019

serhiy-storchaka reviewed Aug 19, 2019

View reviewed changes

Python/import.c Outdated Show resolved Hide resolved

Python/import.c Outdated Show resolved Hide resolved

Remove extra vertical space

57dbdf5

brettcannon requested a review from ericsnowcurrently August 23, 2019 23:58

bedevere-bot added awaiting change review and removed awaiting merge labels Aug 24, 2019

brettcannon approved these changes Sep 11, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Sep 11, 2019

brettcannon merged commit 37c2220 into python:master Sep 11, 2019

bedevere-bot removed the awaiting merge label Sep 11, 2019

nanjekyejoannah deleted the issue35943 branch September 11, 2019 13:06

DinoV pushed a commit to DinoV/cpython that referenced this pull request Sep 12, 2019

bpo-35943: Prevent PyImport_GetModule() from returning a partially-in…

a192e8c

…itialized module (pythonGH-15057)

nanjekyejoannah added the needs backport to 3.7 label Nov 20, 2019

miss-islington assigned brettcannon Nov 20, 2019

mgasner mentioned this pull request Feb 20, 2020

Dask test failures due to importlib thread unsafety? dagster-io/dagster#2181

Closed

websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020

bpo-35943: Prevent PyImport_GetModule() from returning a partially-in…

cd30fa0

…itialized module (pythonGH-15057)

		@@ -0,0 +1 @@
		Added optimization to prevent `PyImport_GetModule()` from returning partially-initialized module. No newline at end of file

Uh oh!

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

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

Uh oh!

Conversation

nanjekyejoannah commented Jul 31, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice Aug 1, 2019

Choose a reason for hiding this comment

Uh oh!

nanjekyejoannah Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

nanjekyejoannah Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

gnprice Aug 1, 2019

Choose a reason for hiding this comment

Uh oh!

gnprice Aug 1, 2019

Choose a reason for hiding this comment

Uh oh!

nanjekyejoannah commented Aug 2, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka commented Aug 3, 2019

Uh oh!

nanjekyejoannah commented Aug 16, 2019

Uh oh!

serhiy-storchaka Aug 18, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

brettcannon commented Aug 23, 2019

Uh oh!

nanjekyejoannah commented Aug 24, 2019

Uh oh!

bedevere-bot commented Aug 24, 2019

Uh oh!

vstinner commented Sep 11, 2019

Uh oh!

miss-islington commented Nov 20, 2019

Uh oh!

miss-islington commented Nov 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

nanjekyejoannah commented Jul 31, 2019 •

edited by bedevere-bot

Loading