Repository: cython/cython
Pull request: Fix bound class methods when unpacking builtin method calls
Enhance #5075 - use c api insted of evaluating python code

From #5075
> The general optimization for unpacking builtin method calls was breaking bound classmethods (with crashes on Py2 and wrong answers on Py3).
> 
> This PR adds code to handle it. The code is unlikely to be very fast, but my judgement is that this is a rare case and thus it's more important just to get it to work rather than to optimize this case.
> 
> Fixes #5051




Problem statement:
[BUG] When using the dictionary fromkeys method in a class, the converted C source code crashes on execution
### Describe the bug

When using the dictionary fromkeys method in a class, the converted C source code crashes on execution

### Code to reproduce the behaviour:

```cython
# example code
class LsaLsdb(object):
    def __init__(self):
        pass

    def test(self):
        new_node = {}
        new_node['area-id'] = ['0.0.0.0', '0.0.0.0']
        test_dict = {}
        new_node['area-id'] = test_dict.fromkeys(new_node['area-id']).keys()
        print(new_node['area-id'])


lsa = LsaLsdb()

lsa.test()
```
```bash
cython -2  --embed -o test.c test.py
gcc -g -I /usr/include/python2.7 test.c -lpython2.7 -o test

run test, and Segmentation fault
```


### Expected behaviour

['0.0.0.0']

### Environment

OS: Linux
Python version 2.7.17
Cython version 3.0.0a11 and 0.29.32


### Additional context

_No response_

Hints from issue discussion:
use dict.fromkeys()  or {}.from, not use test_dict.fromkeys can avoid this problem
This appears to be Python 2 only.

The crash is at
https://github.com/cython/cython/blob/169876872f3cb6198971a1db07e5b8a9d12b3dac/Cython/Utility/ObjectHandling.c#L2008
(`d_method` is NULL).

```
(gdb) print(*method)
$4 = {ob_refcnt = 1, ob_type = 0x7ffff7f5a500 <PyCFunction_Type>}
```

That suggests to me that `method` may not actually be a `PyMethodDescrObject` (and the check is Py3 only). But I don't really know what's gone wrong.
I'm assuming the issue is to with with it being a class method. In which case the options are either:
* reverse the `    if (likely(__Pyx_TypeCheck(method, &PyMethodDescr_Type)))` check in Python 2 - check for "not a C function" instead of "is a method descr"
* Disable the optimization for `fromkeys`. As far as I can tell there's no other builtin types with class methods in Python 2 (except for `float` which is essentially impossible to type in Python 2)
> 

py3 doesn't crash, but it runs incorrectly
The bug applies to any builtin-type classmethod (although these examples only exist on Py3)

```
def bytesfromhex(bytes b):
    return b.fromhex('2Ef0 F1f2  ')

def bytearrayfromhex(bytearray ba):
    return ba.fromhex('2Ef0 F1f2  ')

def intfrombytes(i: int):  # annotation - py int
    i.from_bytes([255, 0, 0], 'big')
```

```
>>> bytesfromhex(b'a')
...
TypeError: bytes.fromhex() takes exactly one argument (2 given)
>>> bytearrayfromhex(bytearray(b'a'))
...
TypeError: bytearray.fromhex() takes exactly one argument (2 given)
>>> intfrombytes(2)
...
TypeError: from_bytes() takes exactly 2 positional arguments (3 given)
```

Essentially the `__Pyx_CallUnboundCMethod1/2` optimization doesn't work for `classmethods` (crashes in Python 2, mixes up the arguments on Python 3)

Commit message: ObjectHandling.c: fix comment alignment

[skip ci]

Patch to review:
diff --git a/Cython/Utility/ObjectHandling.c b/Cython/Utility/ObjectHandling.c
index f0b5c13720e..f82545b9de4 100644
--- a/Cython/Utility/ObjectHandling.c
+++ b/Cython/Utility/ObjectHandling.c
@@ -1992,6 +1992,25 @@ typedef struct {
 /////////////// UnpackUnboundCMethod ///////////////
 //@requires: PyObjectGetAttrStr
 
+#define CONST_REINTERPRET_CAST(pointer_type, pointer) ((pointer_type)(void *)(pointer))
+#define REINTERPRET_CAST(type, var) (*(type *)(&var))
+
+static PyObject *__Pyx_SelflessCall(PyObject *method, PyObject *args, PyObject *kwargs) {
+    PyObject *selfless_args = PyTuple_GetSlice(args, 1, PyTuple_Size(args));
+    if (unlikely(!selfless_args)) return NULL;
+
+    PyObject *result = PyObject_Call(method, selfless_args, kwargs);
+    Py_DECREF(selfless_args);
+    return result;
+}
+
+static PyMethodDef __Pyx_UnboundCMethod_Def = {
+    /* .ml_name  = */ "UnboundCMethod",
+    /* .ml_meth  = */ CONST_REINTERPRET_CAST(PyCFunction, __Pyx_SelflessCall),
+    /* .ml_flags = */ METH_VARARGS | METH_KEYWORDS,
+    /* .ml_doc   = */ NULL
+};
+
 static int __Pyx_TryUnpackUnboundCMethod(__Pyx_CachedCFunction* target) {
     PyObject *method;
     method = __Pyx_PyObject_GetAttrStr(target->type, *target->method_name);
@@ -2000,15 +2019,53 @@ static int __Pyx_TryUnpackUnboundCMethod(__Pyx_CachedCFunction* target) {
     target->method = method;
 #if CYTHON_COMPILING_IN_CPYTHON
     #if PY_MAJOR_VERSION >= 3
-    // method descriptor type isn't exported in Py2.x, cannot easily check the type there
     if (likely(__Pyx_TypeCheck(method, &PyMethodDescr_Type)))
+    #else
+    // method descriptor type isn't exported in Py2.x, cannot easily check the type there.
+    // Therefore, reverse the check to the most likely alternative
+    // (which is returned for class methods)
+    if (likely(!PyCFunction_Check(method)))
     #endif
     {
         PyMethodDescrObject *descr = (PyMethodDescrObject*) method;
         target->func = descr->d_method->ml_meth;
         target->flag = descr->d_method->ml_flags & ~(METH_CLASS | METH_STATIC | METH_COEXIST | METH_STACKLESS);
-    }
+    } else
+#endif
+    // bound classmethods need special treatment
+#if defined(CYTHON_COMPILING_IN_PYPY)
+    // In these functions are regular methods, so just do
+    // the self check
+#elif PY_VERSION_HEX >= 0x03090000
+    if (PyCFunction_CheckExact(method))
+#else
+    if (PyCFunction_Check(method))
 #endif
+    {
+        PyObject *self;
+        int self_found;
+#if CYTHON_COMPILING_IN_LIMITED_API || CYTHON_COMPILING_IN_PYPY
+        self = PyObject_GetAttrString(method, "__self__");
+        if (!self) {
+            PyErr_Clear();
+        }
+#else
+        self = PyCFunction_GET_SELF(method);
+#endif
+        self_found = (self && self != Py_None);
+#if CYTHON_COMPILING_IN_LIMITED_API || CYTHON_COMPILING_IN_PYPY
+        Py_XDECREF(self);
+#endif
+        if (self_found) {
+            PyObject *unbound_method = PyCFunction_New(&__Pyx_UnboundCMethod_Def, method);
+            if (unlikely(!unbound_method)) {
+                Py_DECREF(method);
+                return -1;
+            }
+            Py_INCREF(method);  // PyCFunction will Py_XDECREF method on destruction
+            target->method = unbound_method;
+        }
+    }
     return 0;
 }
 
diff --git a/tests/run/bytearraymethods.pyx b/tests/run/bytearraymethods.pyx
index 76743a71fbb..bf0e0d633d8 100644
--- a/tests/run/bytearraymethods.pyx
+++ b/tests/run/bytearraymethods.pyx
@@ -287,3 +287,13 @@ cdef class BytearraySubtype(bytearray):
     """
     def _append(self, x):
         self.append(x)
+
+def fromhex(bytearray b):
+    """
+    https://github.com/cython/cython/issues/5051
+    Optimization of bound method calls was breaking classmethods
+    >>> fromhex(bytearray(b""))
+    """
+    if IS_PY3:
+        assert b.fromhex('2Ef0 F1f2  ') == b'.\xf0\xf1\xf2'
+    # method doesn't exist on Py2!
diff --git a/tests/run/bytesmethods.pyx b/tests/run/bytesmethods.pyx
index f2a10e1d24d..fecdcc66824 100644
--- a/tests/run/bytesmethods.pyx
+++ b/tests/run/bytesmethods.pyx
@@ -11,6 +11,8 @@ SSIZE_T_MIN = PY_SSIZE_T_MIN
 b_a = b'a'
 b_b = b'b'
 
+import sys
+
 
 @cython.test_assert_path_exists(
     "//PythonCapiCallNode")
@@ -277,3 +279,13 @@ def literal_join(*args):
     result = b'|'.join(args)
     assert cython.typeof(result) == 'Python object', cython.typeof(result)
     return result
+
+def fromhex(bytes b):
+    """
+    https://github.com/cython/cython/issues/5051
+    Optimization of bound method calls was breaking classmethods
+    >>> fromhex(b"")
+    """
+    if sys.version_info[0] > 2:
+        assert b.fromhex('2Ef0 F1f2  ') == b'.\xf0\xf1\xf2'
+    # method doesn't exist on Py2!
diff --git a/tests/run/dict.pyx b/tests/run/dict.pyx
index 691b7863599..1b8f2ecfcbd 100644
--- a/tests/run/dict.pyx
+++ b/tests/run/dict.pyx
@@ -136,3 +136,17 @@ def dict_unpacking_not_for_arg_create_a_copy():
 
     print(sorted(call_once.items()))
     print(sorted(call_twice.items()))
+
+def from_keys_bound(dict d, val):
+    """
+    https://github.com/cython/cython/issues/5051
+    Optimization of bound method calls was breaking classmethods
+    >>> sorted(from_keys_bound({}, 100).items())
+    [('a', 100), ('b', 100)]
+    >>> sorted(from_keys_bound({}, None).items())
+    [('a', None), ('b', None)]
+    """
+    if val is not None:
+        return d.fromkeys(("a", "b"), val)
+    else:
+        return d.fromkeys(("a", "b"))


Code review comments:
diff_hunk: @@ -1992,6 +1992,25 @@ typedef struct {
 /////////////// UnpackUnboundCMethod ///////////////
 //@requires: PyObjectGetAttrStr
 
+#define CONST_REINTERPRET_CAST(pointer_type, pointer) ((pointer_type)(void *)(pointer))
path: Cython/Utility/ObjectHandling.c
text: @user1:
I don't think function pointer->void* is actually portable. I believe function pointer-> other function pointer is allowed though.

Also, if we want to keep these they should probably have `__PYX_` names

@author:
> I don't think function pointer->void* is actually portable. I believe function pointer-> other function pointer is allowed though.

Yes, it seems that C11 standard explicitly allows only casts from function pointers to other function pointers
When I cast `PyCFunctionWithKeywords` to `PyCFunction` compiler generates a warning, I wanted to escape it

It seems like cpython uses a smarter approach of casting to void function for `_PyCFunction_CAST`
```c
// Cast an function to the PyCFunction type to use it with PyMethodDef.
//
// This macro can be used to prevent compiler warnings
// ...
#define _PyCFunction_CAST(func) \
    _Py_CAST(PyCFunction, _Py_CAST(void(*)(void), (func)))
```

> Also, if we want to keep these they should probably have `__PYX_` names

Sure! I can see some areas that can benifit from clean up with those macros, if I want to refactor them it should probably be done in another pr


@user1:
> > Also, if we want to keep these they should probably have __PYX_ names

> Sure! I can see some areas that can benifit from clean up with those macros, if I want to refactor them it should probably be done in another pr

For now I'm just suggesting changing changing the name to not clash with anything a user/library might have defined. I agree a refactor of elsewhere shouldn't be in this PR

diff_hunk: @@ -2000,15 +2019,53 @@ static int __Pyx_TryUnpackUnboundCMethod(__Pyx_CachedCFunction* target) {
     target->method = method;
 #if CYTHON_COMPILING_IN_CPYTHON
     #if PY_MAJOR_VERSION >= 3
-    // method descriptor type isn't exported in Py2.x, cannot easily check the type there
     if (likely(__Pyx_TypeCheck(method, &PyMethodDescr_Type)))
+    #else
+    // method descriptor type isn't exported in Py2.x, cannot easily check the type there.
+    // Therefore, reverse the check to the most likely alternative
+    // (which is returned for class methods)
+    if (likely(!PyCFunction_Check(method)))
     #endif
     {
         PyMethodDescrObject *descr = (PyMethodDescrObject*) method;
         target->func = descr->d_method->ml_meth;
         target->flag = descr->d_method->ml_flags & ~(METH_CLASS | METH_STATIC | METH_COEXIST | METH_STACKLESS);
-    }
+    } else
+#endif
+    // bound classmethods need special treatment
+#if defined(CYTHON_COMPILING_IN_PYPY)
+    // In these functions are regular methods, so just do
+    // the self check
+#elif PY_VERSION_HEX >= 0x03090000
+    if (PyCFunction_CheckExact(method))
+#else
+    if (PyCFunction_Check(method))
 #endif
+    {
+        PyObject *self;
+        int self_found;
+#if CYTHON_COMPILING_IN_LIMITED_API || CYTHON_COMPILING_IN_PYPY
+        self = PyObject_GetAttrString(method, "__self__");
+        if (!self) {
+            PyErr_Clear();
+        }
+#else
+        self = PyCFunction_GET_SELF(method);
+#endif
+        self_found = (self && self != Py_None);
+#if CYTHON_COMPILING_IN_LIMITED_API || CYTHON_COMPILING_IN_PYPY
+        Py_XDECREF(self);
+#endif
+        if (self_found) {
+            PyObject *unbound_method = PyCFunction_New(&__Pyx_UnboundCMethod_Def, method);
+            if (unlikely(!unbound_method)) {
+                Py_DECREF(method);
+                return -1;
+            }
+            Py_INCREF(method);  // PyCFunction will Py_XDECREF method on destruction
path: Cython/Utility/ObjectHandling.c
text: @author:
If I think about it it seems wrong, `__Pyx_PyObject_GetAttrStr` should have already incremented the refcount so we should be fine then?

@user1:
`PyCFunction_New` increfs self internally, so it `XDECREF`s its own reference. We want to `Py_DECREF` method on both the error and non-error path after `PyCFunction_New` I think

@author:
Oh yeah, I missed that `Py_XNewRef` for self.
I think you are correct, previosly `method` was not cleaned up on a non-error path

This maked me think if we can have some test on how much memory is still occupied before python terminates, it would be nice to catch those kinds of errors, this is likely to be a systematic problem

Also I wonder if there is a valgrind test, it still seems like a good idea to have it at least for one platform

@user1:
I don't think Python cleans up after itself perfectly on shutdown anyway so I believe Valgrind is quite hard to interpret.

We have refnanny for references in generated code, but not utility code.

@author:
I tried to experiment with [cpython/Misc/Readme.valgrind](https://github.com/python/cpython/blob/main/Misc/README.valgrind) and [cpython/Misc/valgrind-python.supp](https://github.com/python/cpython/blob/main/Misc/valgrind-python.supp), but it's not very fruitful 
I tried to generate a suppression from the scratch, and I'm getting mixed results

@author:
Actually this is a separate issue, and should probably be discussed elsewhere 

@user2:
> We have refnanny for references in generated code, but not utility code.

Note that you can use it also in utility code, if you put in the required declarations and use the refnanny incref/decref/gotref/giveref macros. We just don't normally do it because it's cumbersome to copy all the boiler plate code together and write refnanny code by hand. If it helps here, it might be worth using. (My guess is that there are quite a number of utility code functions that might benefit from this, especially in `ObjectHandling.c` and `Builtins.c`, which deal a lot with object references.

@author:
resolving in favor of #5224