| msg192845 - (view) |
Author: Ivan Johansen (Padowan) |
Date: 2013-07-11 06:36 |
| In Python/importdl.c around line 99 in the function _PyImport_LoadDynamicModule() you can find the code: def = PyModule_GetDef(m); def->m_base.m_init = p; If the module m, which is returned from a newly imported extension, is not created by PyModule_Create() but in some other way then PyModule_GetDef(m) will return NULL. The next line will then dereference a NULL pointer and crash. I suggest a check for this is added: def = PyModule_GetDef(m); if(def != NULL) def->m_base.m_init = p; |
|
|
| msg192852 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2013-07-11 09:24 |
| New changeset 4343dfaca8e2 by Christian Heimes in branch '3.3': Issue #18426: Fix NULL pointer dereference in C extension import when http://hg.python.org/cpython/rev/4343dfaca8e2 New changeset 9fb3656b178a by Christian Heimes in branch 'default': Issue #18426: Fix NULL pointer dereference in C extension import when http://hg.python.org/cpython/rev/9fb3656b178a |
|
|
| msg192853 - (view) |
Author: Christian Heimes (christian.heimes) *  |
Date: 2013-07-11 09:26 |
| I used a slightly different patch: if (def == NULL) goto error; |
|
|
| msg192854 - (view) |
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * |
Date: 2013-07-11 09:32 |
| I'm not sure the fix is correct: PyModule_GetDef() can return NULL without setting an error, for example when the init function returns a regular Python module. I'm OK to require the init function to return a module created with PyModule_Create(), and fail when it's not the case. But an exception should be set. (and a unit test would help...) |
|
|
| msg192856 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2013-07-11 09:57 |
| In theory you are right. m->md_def could be NULL, too. But in practice it's only going to happen when you have a faulty C extension. The code tries to load a dynamic module (ELF shared library, Windows DLL, ...) with _PyImport_GetDynLoadFunc() a couple of lines before PyModule_GetDef(). Any invalid file is rejected: >>> imp.load_dynamic("os", "Lib/os.py") Traceback (most recent call last): File "", line 1, in ImportError: Lib/os.py: invalid ELF header But an extra check doesn't hurt. How do you like this? def = PyModule_GetDef(m); if (def == NULL) { if (!PyErr_Occured()) { /* m->md_def == NULL */ PyErr_BadArgument(); } goto error; } |
|
|
| msg192857 - (view) |
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * |
Date: 2013-07-11 10:14 |
| I was thinking of a message similar to the one above. The eventual exception set by PyModule_GetDef(m) is less explicit. if (def == NULL) { PyErr_Format(PyExc_SystemError, "initialization of %s did not return an extension module", shortname); goto error; } |
|
|
| msg192864 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2013-07-11 11:03 |
| New changeset fce581643cb6 by Christian Heimes in branch '3.3': Issue #18426: improve exception message. Courtesy of Amaury http://hg.python.org/cpython/rev/fce581643cb6 New changeset 7a50d3c0aa61 by Christian Heimes in branch 'default': Issue #18426: improve exception message. Courtesy of Amaury http://hg.python.org/cpython/rev/7a50d3c0aa61 |
|
|
| msg192870 - (view) |
Author: Ivan Johansen (Padowan) |
Date: 2013-07-11 12:43 |
| If possible it would be nice if any module could be returned from a C extension. Specifically I was trying to subclass module (PyModule_Type) and use that. But an error message is better than a crash. |
|
|
| msg192871 - (view) |
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * |
Date: 2013-07-11 13:24 |
| Returning another kind of module can be dangerous here, because extension modules are handled specially (see _PyImport_FixupExtensionObject), and it's not obvious what this does to normal modules. But I agree that _imp.load_dynamic() could be extended to plain modules. A bit like class/type unification. |
|
|
| msg200905 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2013-10-22 09:35 |
| Is there anything left to do for this ticket? |
|
|
| msg200911 - (view) |
Author: Ivan Johansen (Padowan) |
Date: 2013-10-22 10:06 |
| Probably not. I am setting status to closed with resolution fixed. |
|
|