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

Don't use __Pyx_PyObject_CallMethO on cyfunctions #1728

Merged
merged 2 commits into from Jul 9, 2017

Conversation

Projects
None yet
3 participants
@jdemeyer
Contributor

jdemeyer commented Jun 7, 2017

This code

static CYTHON_INLINE PyObject* __Pyx_PyObject_CallOneArg(PyObject *func, PyObject *arg) {
#if CYTHON_FAST_PYCALL
    if (PyFunction_Check(func)) {
        return __Pyx_PyFunction_FastCall(func, &arg, 1);
    }
#endif
#ifdef __Pyx_CyFunction_USED
    if (likely(PyCFunction_Check(func) || PyObject_TypeCheck(func, __pyx_CyFunctionType))) {
#else
    if (likely(PyCFunction_Check(func))) {
#endif
        if (likely(PyCFunction_GET_FLAGS(func) & METH_O)) {
            // fast and simple case that we are optimising for
            return __Pyx_PyObject_CallMethO(func, arg);
#if CYTHON_FAST_PYCCALL
        } else if (PyCFunction_GET_FLAGS(func) & METH_FASTCALL) {
            return __Pyx_PyCFunction_FastCall(func, &arg, 1);
#endif
        }
    }
    return __Pyx__PyObject_CallOneArg(func, arg);
}

is confusing bound and unbound methods. In particular, it can happen that func is a cyfunction (which is like an unbound method) but that __Pyx_PyObject_CallMethO(func, arg); is called which assumes that func is a bound method.

In particular, the following Cython code is buggy when compiled with binding=True:

cdef class TestMethodOneArg:
    def meth(self, arg):
        pass

def call_meth(x):
    return x.meth()

The expected result of call_meth(TestMethodOneArg()) is a TypeError because meth() is called without arg. The actual result is that meth() is called with self=TestMethodOneArg.meth.__func__ and arg=x.

It's not entirely clear what should be fixed (maybe it's already a bug that the unbound method func has the METH_O flag set?), but simply dropping the check for cyfunctions in the C code above fixes the problem for me.

@jdemeyer jdemeyer changed the title from [WIP] Don't use __Pyx_PyObject_CallMethO on cyfunctions to Don't use __Pyx_PyObject_CallMethO on cyfunctions Jun 7, 2017

@robertwb robertwb added this to the 0.26 milestone Jul 9, 2017

@robertwb

This comment has been minimized.

Show comment
Hide comment
@robertwb

robertwb Jul 9, 2017

Contributor

@scoder Do you want to try and fix this, or should we disable this for now?

Contributor

robertwb commented Jul 9, 2017

@scoder Do you want to try and fix this, or should we disable this for now?

@scoder

This comment has been minimized.

Show comment
Hide comment
@scoder

scoder Jul 9, 2017

Contributor

It's currently also unclear to me what exactly needs fixing. I'll accept this change and think about it later.

Contributor

scoder commented Jul 9, 2017

It's currently also unclear to me what exactly needs fixing. I'll accept this change and think about it later.

@scoder scoder merged commit e8a6ad7 into cython:master Jul 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jdemeyer

This comment has been minimized.

Show comment
Hide comment
@jdemeyer

jdemeyer Jul 10, 2017

Contributor

As I said in the ticket, I haven't investigated this fully. I'm not sure whether this is the right fix, but at least it fixes the testcase.

Contributor

jdemeyer commented Jul 10, 2017

As I said in the ticket, I haven't investigated this fully. I'm not sure whether this is the right fix, but at least it fixes the testcase.

@robertwb

This comment has been minimized.

Show comment
Hide comment
@robertwb

robertwb Jul 12, 2017

Contributor

This is certainly a safe fix.

Contributor

robertwb commented Jul 12, 2017

This is certainly a safe fix.

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