Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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

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.

Sign up for GitHub

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

Merged
brettcannon merged 11 commits into from
Sep 11, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c374e0a
PyImport_GetModule() can return partially-initialized module
Jul 31, 2019
01fe079
📜🤖 Added by blurb_it.
blurb-it[bot] Jul 31, 2019
7d1f7ee
Merge branch 'issue35943' of https://github.com/nanjekyejoannah/cpyth…
Aug 1, 2019
f2be697
Isolate duplicate code in a function
Aug 1, 2019
0925812
ensure any module returned is initialized
nanjekyejoannah Aug 2, 2019
57a6231
change to Py_DECREF and fix doc
nanjekyejoannah Aug 2, 2019
6c1c28e
Update Misc/NEWS.d/next/Library/2019-07-31-15-52-51.bpo-35943.-KswoB.rst
nanjekyejoannah Aug 3, 2019
13f438e
Update Python/import.c
nanjekyejoannah Aug 3, 2019
fc83ad3
changes to import_ensure_initialized
nanjekyejoannah Aug 16, 2019
ff771e7
stop checking for spec
nanjekyejoannah Aug 19, 2019
57dbdf5
Remove extra vertical space
nanjekyejoannah Aug 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions 2 Misc/NEWS.d/next/Library/2019-07-31-15-52-51.bpo-35943.-KswoB.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
The function :c:func:`PyImport_GetModule` now ensures any module it returns is fully initialized.
Patch by Joannah Nanjekye.
69 changes: 46 additions & 23 deletions 69 Python/import.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,11 +387,33 @@ import_get_module(PyThreadState *tstate, PyObject *name)
}


PyObject *
PyImport_GetModule(PyObject *name)
static int
import_ensure_initialized(PyThreadState *tstate, PyObject *mod, PyObject *name)

@ericsnowcurrently ericsnowcurrently Aug 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FWIW, I always prefer to pass in as little as possible. Here PyThreadState is quite broad, since we only need PyInterpreterState.importlib. At the same time, for smaller code like this it can help to keep the common indirections in the function. So I guess I'd leave it as-is. :)

{
PyThreadState *tstate = _PyThreadState_GET();
return import_get_module(tstate, name);
PyInterpreterState *interp = tstate->interp;
PyObject *spec;

_Py_IDENTIFIER(__spec__);

@ericsnowcurrently ericsnowcurrently Aug 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please move both these statics to the top of the file (next to CACHEDIR) as globals.

@gnprice gnprice Aug 10, 2019 •
edited
Loading

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.

I think it would be better to make that as a separate change. This style where they're block-local is the way the existing code works, which keeps the diff simple.

It's also the way that every use of _Py_IDENTIFIER in this file works. So I think the cleanest way to change the style would be to sweep through this whole file (or more) in one PR, separate from any substantive changes.

@nanjekyejoannah nanjekyejoannah Aug 16, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree with @gnprice . To keep this diff simple, I have opened a separate bpo issue to track this refactor. I will work on it If this PR is merged.

@ericsnowcurrently ericsnowcurrently Sep 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sounds good

_Py_IDENTIFIER(_lock_unlock_module);

/* Optimization: only call _bootstrap._lock_unlock_module() if
__spec__._initializing is true.
NOTE: because of this, initializing must be set *before*
stuffing the new module in sys.modules.
*/
spec = _PyObject_GetAttrId(mod, &PyId___spec__);

@ericsnowcurrently ericsnowcurrently Aug 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

_PyModuleSpec_IsInitializing() is a little too clever IMHO, so lets make things a little more obvious here:

Suggested change
spec = _PyObject_GetAttrId(mod, &PyId___spec__);
spec = _PyObject_GetAttrId(mod, &PyId___spec__);
if (spec == NULL) {
PyErr_Clear();
return 0;
}

@serhiy-storchaka serhiy-storchaka Aug 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

_PyModuleSpec_IsInitializing() does this. I do not see reason to duplicate code.

int busy = _PyModuleSpec_IsInitializing(spec);
Py_XDECREF(spec);
if (busy) {
/* Wait until module is done importing. */
PyObject *value = _PyObject_CallMethodIdOneArg(
Comment thread
brettcannon marked this conversation as resolved.
interp->importlib, &PyId__lock_unlock_module, name);
if (value == NULL) {
return -1;
}
Py_DECREF(value);
Comment thread
serhiy-storchaka marked this conversation as resolved.
}
return 0;
}


Expand Down Expand Up @@ -1461,6 +1483,7 @@ PyImport_ImportModule(const char *name)
return result;
}


/* Import a module without blocking
*
* At first it tries to fetch the module from sys.modules. If the module was
Expand Down Expand Up @@ -1760,6 +1783,23 @@ import_find_and_load(PyThreadState *tstate, PyObject *abs_name)
return mod;
}

PyObject *
PyImport_GetModule(PyObject *name)
{
PyThreadState *tstate = _PyThreadState_GET();
PyObject *mod;

mod = import_get_module(tstate, name);
if (mod != NULL && mod != Py_None) {
if (import_ensure_initialized(tstate, mod, name) < 0) {
Py_DECREF(mod);
remove_importlib_frames(tstate);

@ericsnowcurrently ericsnowcurrently Aug 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What made you add this remove_importlib_frames() call? (I'm sure it's correct, BTW.)

@gnprice gnprice Aug 10, 2019

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.

@serhiy-storchaka had requested it -- I think most likely because it's found in the corresponding error: path in PyImport_ImportModuleLevelObject.

return NULL;
}
}
return mod;
}

PyObject *
PyImport_ImportModuleLevelObject(PyObject *name, PyObject *globals,
PyObject *locals, PyObject *fromlist,
Expand Down Expand Up @@ -1815,26 +1855,9 @@ PyImport_ImportModuleLevelObject(PyObject *name, PyObject *globals,
}

if (mod != NULL && mod != Py_None) {
_Py_IDENTIFIER(__spec__);
_Py_IDENTIFIER(_lock_unlock_module);
PyObject *spec;

/* Optimization: only call _bootstrap._lock_unlock_module() if
__spec__._initializing is true.
NOTE: because of this, initializing must be set *before*
stuffing the new module in sys.modules.
*/
spec = _PyObject_GetAttrId(mod, &PyId___spec__);
if (_PyModuleSpec_IsInitializing(spec)) {
PyObject *value = _PyObject_CallMethodIdOneArg(
interp->importlib, &PyId__lock_unlock_module, abs_name);
if (value == NULL) {
Py_DECREF(spec);
goto error;
}
Py_DECREF(value);
if (import_ensure_initialized(tstate, mod, name) < 0) {
goto error;
}
Py_XDECREF(spec);
}
else {
Py_XDECREF(mod);
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.