bpo-35943: PyImport_GetModule() can return partially-initialized module by nanjekyejoannah · Pull Request #15057

bpo-35943: PyImport_GetModule() can return partially-initialized module by nanjekyejoannah · Pull Request #15057 · python/cpython

Choose a reason for hiding this comment

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

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.

Joannah nanjekye added 2 commits

August 1, 2019 11:53

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

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

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

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

Choose a reason for hiding this comment

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

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

@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 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.

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

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

websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request

Jul 20, 2020