Skip to content
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

Memory access violation when calling cpdef'ed method of extension type #2823

Closed
cgohlke opened this issue Feb 5, 2019 · 5 comments
Closed

Comments

@cgohlke
Copy link
Contributor

cgohlke commented Feb 5, 2019

During testing of scikit-image master with Cython 0.29.4 on Windows, I get a reproducible segfault in the scikit-image's _mcp Cython extension module as mentioned at scikit-image/scikit-image#3696 (comment).

The segfault happens when calling the cpdef goal_reached method of the cdef class MCP (Cython extension type) at https://github.com/scikit-image/scikit-image/blob/master/skimage/graph/_mcp.pyx#L541.

The segfault disappears when overriding the cpdef method from Python or when adding a __dict__ to the cdef class.

The segfault is in the generated code

obj_dict_version = likely(Py_TYPE(((PyObject *)__pyx_v_self))->tp_dictoffset) ? __PYX_GET_DICT_VERSION(_PyObject_GetDictPtr(((PyObject *)__pyx_v_self))) : 0;

where __pyx_v_self is a valid python object, tp_dictoffset seems reasonable (1936), but _PyObject_GetDictPtr(((PyObject *)__pyx_v_self)) points to an out of bounds address such that __PYX_GET_DICT_VERSION either segfaults or returns an invalid or random value.

It seems to me that either the cdef class should have a zero tp_dictoffset, or allocate memory for a __dict__.

Here's the top of the call stack:

>	_mcp.cp37-win_amd64.pyd!__pyx_f_7skimage_5graph_4_mcp_3MCP_goal_reached(__pyx_obj_7skimage_5graph_4_mcp_MCP * __pyx_v_self=0x000001a23db99858, __int64 __pyx_v_index=9, double __pyx_v_cumcost=1.0000000000000000, int __pyx_skip_dispatch=0) Line 7580	C
 	_mcp.cp37-win_amd64.pyd!__pyx_pf_7skimage_5graph_4_mcp_3MCP_6find_costs(__pyx_obj_7skimage_5graph_4_mcp_MCP * __pyx_v_self=0x000001a23db99858, _object * __pyx_v_starts=0x000001a203e47d08, _object * __pyx_v_ends=0x00007ff9b3067ce0, _object * __pyx_v_find_all_ends=0x00007ff9b305c950, _object * __pyx_v_max_coverage=0x000001a2420fb870, _object * __pyx_v_max_cumulative_cost=0x00007ff9b3067ce0, _object * __pyx_v_max_cost=0x00007ff9b3067ce0) Line 8683	C
 	_mcp.cp37-win_amd64.pyd!__pyx_pw_7skimage_5graph_4_mcp_3MCP_7find_costs(_object * __pyx_v_self=0x000001a23db99858, _object * __pyx_args=0x000001a203e9c160, _object * __pyx_kwds=0x0000000000000000) Line 7869	C
 	python37.dll!_PyMethodDef_RawFastCallKeywords(PyMethodDef * method, _object * self=0x000001a23db99858, _object * const * args=0x000001a23db97fc0, __int64 nargs=1, _object * kwnames=0x0000000000000000) Line 690	C
 	[Inline Frame] python37.dll!_PyMethodDescr_FastCallKeywords(_object *) Line 288	C
@robertwb
Copy link
Contributor

robertwb commented Feb 6, 2019

Yeah, this looks like a bug. tp_dictoffset should be 0 iff we have a dict. Can you come up with a minimal reproducible example?

@cgohlke
Copy link
Contributor Author

cgohlke commented Feb 6, 2019

Can you come up with a minimal reproducible example?

Not yet, but I experimented with this:

# module.pyx

from __future__ import print_function

from cpython.object cimport Py_TYPE, PyObject


cdef extern from 'object.h':
    PyObject** _PyObject_GetDictPtr(object)


cdef print_info(self):
    cdef Py_ssize_t dictoffset
    cdef PyObject** objptr

    print('* id(self)', id(self))

    dictoffset = Py_TYPE(self).tp_dictoffset
    print('* dictoffset', dictoffset)

    objptr = _PyObject_GetDictPtr(self)
    print('* _PyObject_GetDictPtr(self)', <Py_ssize_t>objptr)
    if objptr != NULL:
        print('* _PyObject_GetDictPtr(self)[0]', <Py_ssize_t>(objptr[0]))

    if hasattr(self, '__dict__'):
        print('* id(self.__dict__)', id(self.__dict__))
        print(self.__dict__)


cdef class CdefClass:

    def __init__(self):
        self.obj = 1

    cpdef cpdef_func(self):
        pass

    def def_func(self):
        print_info(self)
        self.cpdef_func()


class PyClassDerived(CdefClass):
    pass


class PyClass:

    def __init__(self):
        self.obj = 1

    def def_func(self):
        print_info(self)


print('CdefClass')
c = CdefClass()
c.def_func()

print('PyClassDerived')
c = PyClassDerived()
c.def_func()

print('PyClass')
c = PyClass()
c.def_func()

When using the CdefClass definition with __dict__, the output looks as expected (to me):

# module.pyd

cdef class CdefClass:
    cdef dict __dict__
    cdef object obj
    cpdef cpdef_func(self)
CdefClass
* id(self) 1693360024072
* dictoffset 24
* _PyObject_GetDictPtr(self) 1693360024096
* _PyObject_GetDictPtr(self)[0] 1693360002464
* id(self.__dict__) 1693360002464
{}

PyClassDerived
* id(self) 1693360003040
* dictoffset 24
* _PyObject_GetDictPtr(self) 1693360003064
* _PyObject_GetDictPtr(self)[0] 1693360002968
* id(self.__dict__) 1693360002968
{}

PyClass
* id(self) 1693360088512
* dictoffset 16
* _PyObject_GetDictPtr(self) 1693360088528
* _PyObject_GetDictPtr(self)[0] 1693360002464
* id(self.__dict__) 1693360002464
{'obj': 1}

Note that _PyObject_GetDictPtr(self)[0] == id(self.__dict__) in all cases.

When using the CdefClass definition without __dict__, the output for the PyClassDerived case looks suspicious (_PyObject_GetDictPtr(self)[0] == 0 != id(self.__dict__) ):

# module.pyd

cdef class CdefClass:
    # cdef dict __dict__
    cdef object obj
    cpdef cpdef_func(self)
CdefClass
* id(self) 2538454214080
* dictoffset 0
* _PyObject_GetDictPtr(self) 0

PyClassDerived
* id(self) 2538454128608
* dictoffset 32
* _PyObject_GetDictPtr(self) 2538454128640
* _PyObject_GetDictPtr(self)[0] 0
* id(self.__dict__) 2538454127960
{}

@scoder scoder added this to the 0.29.5 milestone Feb 8, 2019
@scoder
Copy link
Contributor

scoder commented Feb 8, 2019

I think it simply means that the dict is supported by the object but has not been initialised yet (because nothing was assigned to it). Looks like we must handle that case.

scoder added a commit that referenced this issue Feb 8, 2019
…n types again that do not initialise their own dict.

This lead to a crash with dict versioning in Py3.6+.
Closes GH-2823.
@cgohlke
Copy link
Contributor Author

cgohlke commented Feb 8, 2019

Thank you! 253c25a fixes the crash in scikit-image.

@cgohlke cgohlke closed this as completed Feb 8, 2019
@scoder
Copy link
Contributor

scoder commented Feb 8, 2019

Fix released in 0.29.5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants