-
-
Notifications
You must be signed in to change notification settings - Fork 29.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-35943: PyImport_GetModule() can return partially-initialized module #15057
Conversation
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Python/import.c
Outdated
int value = import_ensure_initialized(tstate, mod, name); | ||
if (value == -1) { | ||
remove_importlib_frames(tstate); | ||
return NULL; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 returnedmod
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 returnNULL
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?
There was a problem hiding this comment.
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 .
Python/import.c
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing @serhiy-storchaka suggested at the other call site.
@@ -0,0 +1 @@ | |||
Added optimization to prevent `PyImport_GetModule()` from returning partially-initialized module. |
There was a problem hiding this comment.
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.
@gnprice and @serhiy-storchaka, I have made some changes PTAL. |
Misc/NEWS.d/next/Library/2019-07-31-15-52-51.bpo-35943.-KswoB.rst
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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.
Misc/NEWS.d/next/Library/2019-07-31-15-52-51.bpo-35943.-KswoB.rst
Outdated
Show resolved
Hide resolved
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 . |
Python/import.c
Outdated
stuffing the new module in sys.modules. | ||
*/ | ||
spec = _PyObject_GetAttrId(mod, &PyId___spec__); | ||
if (spec == NULL) { |
There was a problem hiding this comment.
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. |
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