classification
Title: Crash when extension does not use PyModule_Create()
Type: crash Stage: resolved
Components: Versions: Python 3.4, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Padowan, amaury.forgeotdarc, christian.heimes, python-dev
Priority: normal Keywords:

Created on 2013-07-11 06:36 by Padowan, last changed 2013-10-22 10:06 by Padowan. This issue is now closed.

Messages (11)
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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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 "<stdin>", line 1, in <module>
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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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.
History
Date User Action Args
2013-10-22 10:06:30Padowansetstatus: pending -> closed
resolution: fixed
messages: + msg200911
2013-10-22 09:35:04christian.heimessetstatus: open -> pending
assignee: christian.heimes ->
messages: + msg200905
2013-07-11 13:24:34amaury.forgeotdarcsetmessages: + msg192871
2013-07-11 12:43:28Padowansetmessages: + msg192870
2013-07-11 11:03:47python-devsetmessages: + msg192864
2013-07-11 10:14:27amaury.forgeotdarcsetmessages: + msg192857
2013-07-11 09:57:43christian.heimessetassignee: christian.heimes
resolution: fixed -> (no value)
2013-07-11 09:57:06christian.heimessetmessages: + msg192856
2013-07-11 09:32:45amaury.forgeotdarcsetstatus: closed -> open
2013-07-11 09:32:32amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg192854
2013-07-11 09:26:18christian.heimessetstatus: open -> closed

versions: - Python 3.1, Python 3.2, Python 3.5
nosy: + christian.heimes

messages: + msg192853
resolution: fixed
stage: resolved
2013-07-11 09:24:12python-devsetnosy: + python-dev
messages: + msg192852
2013-07-11 06:36:51Padowancreate