bpo-35943: PyImport_GetModule() can return partially-initialized module#15057
bpo-35943: PyImport_GetModule() can return partially-initialized module#15057
Conversation
gnprice
left a comment
There was a problem hiding this 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
left a comment
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm looking back at what the
goto errorin 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 returnedmodto 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 returnNULLand 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?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Same thing @serhiy-storchaka suggested at the other call site.
| @@ -0,0 +1 @@ | ||
| Added optimization to prevent `PyImport_GetModule()` from returning partially-initialized module. No newline at end of file |
There was a problem hiding this comment.
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.
|
@gnprice and @serhiy-storchaka, I have made some changes PTAL. |
gnprice
left a comment
There was a problem hiding this comment.
LGTM! Thanks @nanjekyejoannah for this patch, and for all the updates.
|
LGTM. @ericsnowcurrently, please take a look. |
Co-Authored-By: Serhiy Storchaka <storchaka@gmail.com>
Co-Authored-By: Serhiy Storchaka <storchaka@gmail.com>
|
I have made the requested changes @ericsnowcurrently , @serhiy-storchaka , @gnprice PTAL . |
| stuffing the new module in sys.modules. | ||
| */ | ||
| spec = _PyObject_GetAttrId(mod, &PyId___spec__); | ||
| if (spec == NULL) { |
There was a problem hiding this comment.
_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. |
My bad. Thanks for the reminder. |
|
Thanks for making the requested changes! @serhiy-storchaka, @ericsnowcurrently: please review the changes made to this pull request. |
|
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 ✨ 🍰 ✨ ;-) |
|
Thanks @nanjekyejoannah for the PR, and @brettcannon for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7. |
|
Sorry, @nanjekyejoannah and @brettcannon, I could not cleanly backport this to |
Added optimization to prevent
PyImport_GetModule()from returning partially-initialized module.https://bugs.python.org/issue35943