[BUG] Py_TPFLAGS_HEAPTYPE is set on static PyTypeObject

[undingen]

I'm working on the pyston python implementation (pyston v2 is mostly a cpython 3.8 fork and pretends to be cpython to cython) and some users notified us that they run into issues using gevent with pyston.
I tracked it down to a memory corruption issue/out of bounds write generated by the cythonized code in connection with ours.

It's caused by cython setting Py_TPFLAGS_HEAPTYPE temporarily when calling __Pyx_PyType_Ready:

cython/Cython/Utility/ExtensionTypes.c

Line 118 in 6889482

t->tp_flags |= Py_TPFLAGS_HEAPTYPE;

Pyston assumes (because Py_TPFLAGS_HEAPTYPE is set) that it got a larger PyHeapTypeObject and will clear some fields (which overwrite some random memory which causes the gevent bug)

https://github.com/pyston/pyston/blob/832ec0d76facb2e1e958324c7c1ce2c3879283ca/Objects/typeobject.c#L281

Maybe we can workaround the issue in pyston but in general I think setting this flag could be dangerous for cpython too it seems to check for Py_TPFLAGS_HEAPTYPE several times and the cython comment about it maybe outdated.

What do you think?

To Reproduce
Code to reproduce the behaviour:

taken from the pyston bug report:
david@davitude:~$ pyenv install pyston-2.2
david@davitude:~$ ~/.pyenv/versions/pyston-2.2/bin/python -m pip install gevent
david@davitude:~$ ~/.pyenv/versions/pyston-2.2/bin/python -c "from gevent.monkey import is_object_patched"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/david/.pyenv/versions/pyston-2.2/lib/pyston3.8/site-packages/gevent/__init__.py", line 85, in <module>
    from gevent._config import config
  File "/home/david/.pyenv/versions/pyston-2.2/lib/pyston3.8/site-packages/gevent/_config.py", line 699, in <module>
    Loop().get()
  File "/home/david/.pyenv/versions/pyston-2.2/lib/pyston3.8/site-packages/gevent/_config.py", line 146, in get
    self.value = self.validate(self._default())
  File "/home/david/.pyenv/versions/pyston-2.2/lib/pyston3.8/site-packages/gevent/_config.py", line 248, in validate
    return self._import_one_of([self.shortname_map.get(x, x) for x in value])
  File "/home/david/.pyenv/versions/pyston-2.2/lib/pyston3.8/site-packages/gevent/_config.py", line 219, in _import_one_of
    return self._import_one(item)
  File "/home/david/.pyenv/versions/pyston-2.2/lib/pyston3.8/site-packages/gevent/_config.py", line 237, in _import_one
    module = importlib.import_module(module)
  File "/home/david/.pyenv/versions/pyston-2.2/lib/pyston3.8/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "src/gevent/libev/corecext.pyx", line 979, in init gevent.libev.corecext
SystemError: Bad call flags for CyFunction

Expected behavior
Py_TPFLAGS_HEAPTYPE is only set if the PyTypeObject is a subclass of PyHeapTypeObject.

Environment (please complete the following information):

• OS: Linux
• Python version Pyston 2.2
• Cython version 3.0a6

Additional context
Pyston bug report: pyston/pyston#42

[da-woods]

From the version you report, I think you have the fix in #3603, but it's worth checking that you do?

One of the suggestions on that issue (that we didn't take) was to front-pad our non-heap classes to give them space that's safe to overwrite - re-visiting that might be a solution.

We could disable Py_TPFLAGS_HEAPTYPE for Pyston but it'd (probably) break multiple-inheritance for cdef classes on Pyston. That might be acceptable in exchange for it working in general?

[undingen]

Thanks for the link to the other related bug and looking into my issue.
I can confirm that I see the gc.disable() call. Unfortunately with Pyston this memory corruption is not fixed by disabling the gc during PyType_Ready because of the additional fields we need to initialize as pointed out above (but we also need the gc.disable code or we would likely run into the same issue as cpython).
I tried using setting CFLAGS=-DCYTHON_USE_TYPE_SPECS=1 during pip install (with disabled cache to make sure it recompiles) but this did not help I'm not sure if this flag got picked up. But if this causes some other incompatibilities it's not really a good solution for us because we want to support same stuff as cpython. So maybe special casing this cases in pyston is needed - I'm just not really sure how to do it...

[da-woods]

CYTHON_USE_TYPE_SPECS is a separate mode that's currently in quite an experimental stage. It stands a decent chance of getting rid of this error but is likely to have a bunch of issues of its own. I would not use it as your solution to this (yet)

So maybe special casing this cases in pyston is needed - I'm just not really sure how to do it...

The main thing that Cython would need to know to implement any special-cases is a macro to test (e.g. #if defined(PYSTON_VERSION) or something similar). The simplest short-term fix would just be to disable this whole block of code for Pyston. This would break multiply inherited cdef class (with a useful import-time error rather than a confusing crash), but it would let the vast majority of Cython code work successfully.

[mattip]

... a macro to test (e.g. #if defined(PYSTON_VERSION)) ...

Pyston has version macros

#define PYSTON_MAJOR_VERSION        2
#define PYSTON_MINOR_VERSION        2
#define PYSTON_MICRO_VERSION        0

[da-woods]

In which case, @undingen have a look at https://github.com/da-woods/cython/tree/pyston-tpflags and see if it fixes most of your issues.

I still thing longer-term a better solution might be useful though.

[undingen]

Thanks for your workaround I can confirm that it solves the crashes. It may be a good workaround for pyston 2.2.0 but for upcoming version I would like to be able to support also the case with mixed static and heap allocated classes.

I still think that in general setting the flag is wrong.
E.g quick search showed that cpython 3.10 uses it to see if it should set Py_TPFLAGS_IMMUTABLETYPE (but also uses it other places):

(myenv) marius@antidote:~/tmp/ext$ cat helloworld.pyx 
cdef class MyClass:
    def __init__(self):
        pass
(myenv) marius@antidote:~/tmp/ext$ cat setup.py 
from setuptools import setup
from Cython.Build import cythonize
setup(
    ext_modules = cythonize("helloworld.pyx")
)

python 3.10b:

>>> import helloworld
>>> C = helloworld.MyClass
>>> C
<class 'helloworld.MyClass'>
>>> C.__name__ = "bla"
>>> C
<class 'bla'>

python 3.8.5:

>>> import helloworld
>>> C = helloworld.MyClass
>>> C
<class 'helloworld.MyClass'>
>>> C.__name__ = "bla"
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: can't set attributes of built-in/extension type 'helloworld.MyClass'

Concerning the pyston special casing I noticed that cython contains support for pyston v1, I will put up PR to remove the support because it's dead and v2 should not require (that much) special casing so it's just a unnecessary maintenance burden on you folks.

[mattip]

Does Pyston have something like PY_VERSION_HEX or PYPY_VERSION_NUM that holds all the major-minor-release versions in one value? This would make it easier to conditionally include/avoid codepaths based on the exact release version.

[undingen]

There is unfortunately no define which contains all the version numbers :/.

I created a new PR which removes the old pyston v1 special casing.
And I think the fix from @da-woods is a good workaround for now because it seems like most of cythons tests are passing (I'm getting failures=4, errors=6 but some of them seem to be just slightly different error messages or the mixed inheritance).
Thanks again.

[da-woods]

I still think that in general setting the flag is wrong.

Yeah, it's clearly a pretty awful hack. However, I'm not sure there's a better way to persuade CPython to allow us to do multiple inheritance. If there was a good way of getting the feature without the flag then we'd do it!

E.g quick search showed that cpython 3.10 uses it to see if it should set Py_TPFLAGS_IMMUTABLETYPE (but also uses it other places):

That's possibly something we should fix. Although it's possibly not a huge moral problem whether types have mutable attributes or not - I don't think it's something that Cython has intentionally designed for, and it may well change with CYTHON_USE_TYPE_SPECS

[robertwb]

The check that this is a heap type in CPython in this particular case is overly strict; another workaround would be to relax that in Pyston and elide this hack altogether there.

[undingen]

@robertwb good idea, I merged a commit into pyston which disabled the error messages. Which means the next pyston releases will not have any problems with mixed static/heap subclasses and the fix from @da-woods is enough.
Thanks again.

[undingen]

@da-woods Whats the next step to get your fix for pyston into cython? Should I create a new pull request with your fix or are you going to commit it?

[scoder]

I committed the change in b19b8d8

[scoder]

I think we can close this ticket, right?
While it's not resolved in general (the hack still applies to CPython), I don't think there is currently anything left that hurts. At least not for Pyston2, for which this ticket was opened.

[undingen]

Yes I agree thanks for committing the fix!