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

__Pyx_PyType_Ready() can garble memory #3603

Closed
pitrou opened this issue May 12, 2020 · 19 comments
Closed

__Pyx_PyType_Ready() can garble memory #3603

pitrou opened this issue May 12, 2020 · 19 comments

Comments

@pitrou
Copy link
Contributor

pitrou commented May 12, 2020

After some debugging, we found out that __Pyx_PyType_Ready can garble arbitrary memory.
Indeed, it has the following lines:

#if PY_VERSION_HEX >= 0x03050000
    t->tp_flags |= Py_TPFLAGS_HEAPTYPE;
#endif
    r = PyType_Ready(t);
#if PY_VERSION_HEX >= 0x03050000
    t->tp_flags &= ~Py_TPFLAGS_HEAPTYPE;
#endif

The problem is if PyType_Ready indirectly triggers the garbage collector. The static type t will be incorrectly considered as a heap type, and therefore as a GC-enabled object. When traversing this object, the GC will lookup the GC header by subtracting 16 bytes from t's beginning, which may point to any other C static variable. Then it will temporarily "decref" the GC header, modifying other data.
(in our case, that other data is a Cython function's ml_flags, rendering it unusable, but that might be something else, depending what your Cython module contains)

@pitrou
Copy link
Contributor Author

pitrou commented May 12, 2020

@scoder

@pitrou
Copy link
Contributor Author

pitrou commented May 12, 2020

Perhaps a possible solution is for Cython to allocate static types like this:

typedef struct {
    PyGC_Head gc_head;
    PyTypeObject typeobj;
} SafeStaticType;

static SafeStaticType __pyx_safetype_7pyarrow_3lib_KeyValueMetadata = {
    .gc = {
        .gc_refs = -2  /* _PyGC_REFS_UNTRACKED */
    },
    .typeobj = {
        PyVarObject_HEAD_INIT(0, 0)
        /* etc. */
    }
};

static PyTypeObject* __pyx_type_7pyarrow_3lib_KeyValueMetadata = \
    &__pyx_safetype_7pyarrow_3lib_KeyValueMetadata.typeobj;

@pitrou
Copy link
Contributor Author

pitrou commented May 12, 2020

In the meantime, we're probably going to disable the GC on module initialization:
https://github.com/apache/arrow/pull/7160/files

@scoder
Copy link
Contributor

scoder commented May 12, 2020

Lovely. Thanks for the investigations.
Here is the full code. Note the comment.

#if PY_VERSION_HEX >= 0x03050000
// As of https://bugs.python.org/issue22079
// PyType_Ready enforces that all bases of a non-heap type are
// non-heap. We know that this is the case for the solid base but
// other bases are heap allocated and are kept alive through the
// tp_bases reference.
// Other than this check, the Py_TPFLAGS_HEAPTYPE flag is unused
// in PyType_Ready().
t->tp_flags |= Py_TPFLAGS_HEAPTYPE;
#endif
r = PyType_Ready(t);
#if PY_VERSION_HEX >= 0x03050000
t->tp_flags &= ~Py_TPFLAGS_HEAPTYPE;
#endif

@pitrou
Copy link
Contributor Author

pitrou commented May 12, 2020

Yeah, I saw the comment. I think the BPO issue would deserve a proper fix, but I'm not involved very much nowadays. Perhaps @jdemeyer feels brave enough? :-)

@pitrou
Copy link
Contributor Author

pitrou commented May 12, 2020

Perhaps Cython should always use PyType_FromSpec, not always in limited API builds?

@scoder scoder added this to the 3.0 milestone May 12, 2020
@scoder
Copy link
Contributor

scoder commented May 12, 2020

Perhaps Cython should always use PyType_FromSpec

Yes, I was thinking that anyway. The main reason for keeping the current static types is Py2.7 support. That'll be ripped out in Cython 3.1, shortly after the 3.0 release. But we could already start by switching to heap types for Py3.4+ in Cython 3.0. (No, this will not change in 0.29.x any more.)

We could also mitigate the issue by only setting the heaptype flag if there actually are heap types involved, I think. That could also be done in 0.29.x.
Edit: turns out, this code is really only run if there are heap type bases.

@scoder
Copy link
Contributor

scoder commented May 14, 2020

And regarding always using PyType_FromSpec() in Py3, then we run into bugs like https://bugs.python.org/issue38140
So, I guess the answer is no, we can't currently use PyType_FromSpec(), at least not for Py<3.9.

@scoder
Copy link
Contributor

scoder commented May 14, 2020

Anyway, I started splitting the current Limited-API special-casing into separate feature flags in #3611. Let's see where that gets us. We might be able to work around the limitations of PyType_FromSpec() when we're not limited to the limited API.

@scoder
Copy link
Contributor

scoder commented May 14, 2020

… and, obviously, PyType_FromSpec() calls PyType_Ready() internally, so that's even worse in the end. :-/

Edit: I keep getting confused by all of those different APIs and their impact. When using PyType_FromSpec(), then the resulting extension type is also a heap type, and we won't have this problem. Back to #3611.

@da-woods
Copy link
Contributor

It's quite likely I'm missing something, but is there a reason not to turn off the GC within __Pyx_PyTypeReady? It might be possible to do in C but even if not then it's only a module import and Python function call.

@scoder
Copy link
Contributor

scoder commented May 14, 2020

Yeah, I looked at that, too, but that was only changed fairly recently. In older CPython versions, the GC switch is a static C variable in the gc module, so it's not trivial to switch it off there. It's still an option, though.

@scoder
Copy link
Contributor

scoder commented May 16, 2020

#3611 seems to work now (as of a0d4511) with -DCYTHON_USE_TYPE_SPECS=1 enabled, i.e. using type specs for all types that currently have specs. Only the types of generators and coroutines are missing, I'll also look into those. If it all turns out good, I think this can become the default setting for Py3+ in Cython 3.0.

@pitrou could you give that branch a try on your side?

@pitrou
Copy link
Contributor Author

pitrou commented May 16, 2020

Is it CYTHON_USE_TYPE_FROM_SPEC or CYTHON_USE_TYPE_SPECS? It's not clear from the diff.

@scoder
Copy link
Contributor

scoder commented May 16, 2020 via email

@scoder scoder closed this as completed in b2868f9 May 18, 2020
@scoder
Copy link
Contributor

scoder commented May 18, 2020

I've opted for disabling the GC during these calls to PyType_Ready() as a way to fix this specific bug.

Getting type specs to work is too big a change for now, since they really don't provide equal functionality with static types (e.g. there's no buffer interface support).

@scoder
Copy link
Contributor

scoder commented May 18, 2020

Thanks for figuring out the details, @pitrou.

scoder added a commit that referenced this issue May 18, 2020
… type for an actual non-heap type, because it can lead to crashes if GC visits such a hacked type.

Closes #3603
@scoder scoder modified the milestones: 3.0, 0.29.18 May 18, 2020
@pitrou
Copy link
Contributor Author

pitrou commented May 18, 2020

@scoder Do you still want me to try type specs on PyArrow?

@scoder
Copy link
Contributor

scoder commented May 18, 2020 via email

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