Skip to content

Comments

includes/cpython: Fix newfunc to use PyObject* for args/kwargs instead of object#4823

Merged
da-woods merged 1 commit intocython:0.29.xfrom
navytux:y/newfunc
Jul 3, 2022
Merged

includes/cpython: Fix newfunc to use PyObject* for args/kwargs instead of object#4823
da-woods merged 1 commit intocython:0.29.xfrom
navytux:y/newfunc

Conversation

@navytux
Copy link
Contributor

@navytux navytux commented Jun 5, 2022

object means the argument is always non-NULL valid Python object, while
PyObject* argument can be generally NULL. If the argument is indeed
passed as NULL, and we declare it as object, generated code will crash
while trying to incref it.

Quoting #4822:

object.pxd currently declares `newfunc` as follows:

```pyx
ctypedef object (*newfunc)(cpython.type.type, object, object)  # (type, args, kwargs)
```

which implies that `args` and `kwargs` are always live objects and cannot be NULL.

However Python can, and does, call tp_new with either args=NULL, or kwargs=NULL or both. And in such cases this leads to segfault in automatically-generated __Pyx_INCREF for args or kw.

The fix is to change `object` to `PyObject*` for both args and kwargs.

Please see below for details:

```cython
# cython: language_level=3
from cpython cimport newfunc, type as cpytype, Py_TYPE

cdef class X:
    cdef int i
    def __init__(self, i):
        self.i = i
    def __repr__(self):
        return 'X(%d)' % self.i

cdef newfunc _orig_tp_new = Py_TYPE(X(0)).tp_new

cdef object _trace_tp_new(cpytype cls, object args, object kw):
    print('_trace_tp_new', cls, args, kw)
    return _orig_tp_new(cls, args, kw)

Py_TYPE(X(0)).tp_new = _trace_tp_new

x = X(123)
print(x)
```

```console
(neo) (py3.venv) (g.env) kirr@deca:~/src/tools/go/pygolang$ cythonize -i x.pyx
Compiling /home/kirr/src/tools/go/pygolang/x.pyx because it changed.
[1/1] Cythonizing /home/kirr/src/tools/go/pygolang/x.pyx
running build_ext
building 'x' extension
...
x86_64-linux-gnu-gcc -pthread -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O2 -Wall -g -ffile-prefix-map=/build/python3.9-RNBry6/python3.9-3.9.2=. -fstack-protector-strong -Wformat -Werror=format-security -g -fwrapv -O2 -g -ffile-prefix-map=/build/python3.9-RNBry6/python3.9-3.9.2=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -I/home/kirr/src/wendelin/venv/py3.venv/include -I/usr/include/python3.9 -c /home/kirr/src/tools/go/pygolang/x.c -o /home/kirr/src/tools/go/pygolang/tmpqkz1r96s/home/kirr/src/tools/go/pygolang/x.o
x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-z,relro -g -fwrapv -O2 -Wl,-z,relro -g -fwrapv -O2 -g -ffile-prefix-map=/build/python3.9-RNBry6/python3.9-3.9.2=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 /home/kirr/src/tools/go/pygolang/tmpqkz1r96s/home/kirr/src/tools/go/pygolang/x.o -o /home/kirr/src/tools/go/pygolang/x.cpython-39-x86_64-linux-gnu.so
```

```console
(neo) (py3.venv) (g.env) kirr@deca:~/src/tools/go/pygolang$ python -c 'import x'
Ошибка сегментирования (стек памяти сброшен на диск)
```

```console
(neo) (py3.venv) (g.env) kirr@deca:~/src/tools/go/pygolang$ gdb python core
...
Reading symbols from python...
Reading symbols from /usr/lib/debug/.build-id/f9/02f8a561c3abdb9c8d8c859d4243bd8c3f928f.debug...
[New LWP 218557]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Core was generated by `python -c import x'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  _Py_INCREF (op=0x0) at /usr/include/python3.9/object.h:408
408         op->ob_refcnt++;

(gdb) bt 5
#0  _Py_INCREF (op=0x0) at /usr/include/python3.9/object.h:408
#1  __pyx_f_1x__trace_tp_new (__pyx_v_cls=0x7f5ce75e6880 <__pyx_type_1x_X>, __pyx_v_args=(123,), __pyx_v_kw=0x0) at /home/kirr/src/tools/go/pygolang/x.c:1986
#2  0x000000000051dd7e in type_call (type=type@entry=0x7f5ce75e6880 <__pyx_type_1x_X>, args=args@entry=(123,), kwds=kwds@entry=0x0)
    at ../Objects/typeobject.c:1014
#3  0x00007f5ce75df8d4 in __Pyx_PyObject_Call (func=<type at remote 0x7f5ce75e6880>, arg=(123,), kw=0x0) at /home/kirr/src/tools/go/pygolang/x.c:3414
#4  0x00007f5ce75df276 in __pyx_pymod_exec_x (__pyx_pyinit_module=<optimized out>) at /home/kirr/src/tools/go/pygolang/x.c:3017
(More stack frames follow...)

(gdb) f 1
#1  __pyx_f_1x__trace_tp_new (__pyx_v_cls=0x7f5ce75e6880 <__pyx_type_1x_X>, __pyx_v_args=(123,), __pyx_v_kw=0x0) at /home/kirr/src/tools/go/pygolang/x.c:1986
1986      __Pyx_INCREF(__pyx_v_kw);
```

-> Change newfunc signature to use PyObject* instead of object to fix it.

With this fix, and test example updates to account for object -> PyObject* change as follows ...

--- a/x.pyx.kirr
+++ b/x.pyx
@@ -1,5 +1,5 @@
 # cython: language_level=3
-from cpython cimport newfunc, type as cpytype, Py_TYPE
+from cpython cimport newfunc, type as cpytype, Py_TYPE, PyObject

 cdef class X:
     cdef int i
@@ -10,8 +10,12 @@ cdef class X:

 cdef newfunc _orig_tp_new = Py_TYPE(X(0)).tp_new

-cdef object _trace_tp_new(cpytype cls, object args, object kw):
-    print('_trace_tp_new', cls, args, kw)
+cdef object xobject(PyObject* x):
+    return "null"  if x == NULL  else \
+           <object>x
+
+cdef object _trace_tp_new(cpytype cls, PyObject* args, PyObject* kw):
+    print('_trace_tp_new', cls, xobject(args), xobject(kw))
     return _orig_tp_new(cls, args, kw)

 Py_TYPE(X(0)).tp_new = _trace_tp_new

... it works as expected without crashing:

$ python -c 'import x'
_trace_tp_new <type 'x.X'> (123,) null
X(123)

Fixes: #4822

…d of object

object means the argument is always non-NULL valid Python object, while
PyObject* argument can be generally NULL. If the argument is indeed
passed as NULL, and we declare it as object, generated code will crash
while trying to incref it.

Quoting cython#4822:

    object.pxd currently declares `newfunc` as follows:

    ```pyx
    ctypedef object (*newfunc)(cpython.type.type, object, object)  # (type, args, kwargs)
    ```

    which implies that `args` and `kwargs` are always live objects and cannot be NULL.

    However Python can, and does, call tp_new with either args=NULL, or kwargs=NULL or both. And in such cases this leads to segfault in automatically-generated __Pyx_INCREF for args or kw.

    The fix is to change `object` to `PyObject*` for both args and kwargs.

    Please see below for details:

    ```cython
    # cython: language_level=3
    from cpython cimport newfunc, type as cpytype, Py_TYPE

    cdef class X:
        cdef int i
        def __init__(self, i):
            self.i = i
        def __repr__(self):
            return 'X(%d)' % self.i

    cdef newfunc _orig_tp_new = Py_TYPE(X(0)).tp_new

    cdef object _trace_tp_new(cpytype cls, object args, object kw):
        print('_trace_tp_new', cls, args, kw)
        return _orig_tp_new(cls, args, kw)

    Py_TYPE(X(0)).tp_new = _trace_tp_new

    x = X(123)
    print(x)
    ```

    ```console
    (neo) (py3.venv) (g.env) kirr@deca:~/src/tools/go/pygolang$ cythonize -i x.pyx
    Compiling /home/kirr/src/tools/go/pygolang/x.pyx because it changed.
    [1/1] Cythonizing /home/kirr/src/tools/go/pygolang/x.pyx
    running build_ext
    building 'x' extension
    ...
    x86_64-linux-gnu-gcc -pthread -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O2 -Wall -g -ffile-prefix-map=/build/python3.9-RNBry6/python3.9-3.9.2=. -fstack-protector-strong -Wformat -Werror=format-security -g -fwrapv -O2 -g -ffile-prefix-map=/build/python3.9-RNBry6/python3.9-3.9.2=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -I/home/kirr/src/wendelin/venv/py3.venv/include -I/usr/include/python3.9 -c /home/kirr/src/tools/go/pygolang/x.c -o /home/kirr/src/tools/go/pygolang/tmpqkz1r96s/home/kirr/src/tools/go/pygolang/x.o
    x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-z,relro -g -fwrapv -O2 -Wl,-z,relro -g -fwrapv -O2 -g -ffile-prefix-map=/build/python3.9-RNBry6/python3.9-3.9.2=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 /home/kirr/src/tools/go/pygolang/tmpqkz1r96s/home/kirr/src/tools/go/pygolang/x.o -o /home/kirr/src/tools/go/pygolang/x.cpython-39-x86_64-linux-gnu.so
    ```

    ```console
    (neo) (py3.venv) (g.env) kirr@deca:~/src/tools/go/pygolang$ python -c 'import x'
    Ошибка сегментирования (стек памяти сброшен на диск)
    ```

    ```console
    (neo) (py3.venv) (g.env) kirr@deca:~/src/tools/go/pygolang$ gdb python core
    ...
    Reading symbols from python...
    Reading symbols from /usr/lib/debug/.build-id/f9/02f8a561c3abdb9c8d8c859d4243bd8c3f928f.debug...
    [New LWP 218557]
    [Thread debugging using libthread_db enabled]
    Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
    Core was generated by `python -c import x'.
    Program terminated with signal SIGSEGV, Segmentation fault.
    #0  _Py_INCREF (op=0x0) at /usr/include/python3.9/object.h:408
    408         op->ob_refcnt++;

    (gdb) bt 5
    #0  _Py_INCREF (op=0x0) at /usr/include/python3.9/object.h:408
    cython#1  __pyx_f_1x__trace_tp_new (__pyx_v_cls=0x7f5ce75e6880 <__pyx_type_1x_X>, __pyx_v_args=(123,), __pyx_v_kw=0x0) at /home/kirr/src/tools/go/pygolang/x.c:1986
    cython#2  0x000000000051dd7e in type_call (type=type@entry=0x7f5ce75e6880 <__pyx_type_1x_X>, args=args@entry=(123,), kwds=kwds@entry=0x0)
        at ../Objects/typeobject.c:1014
    cython#3  0x00007f5ce75df8d4 in __Pyx_PyObject_Call (func=<type at remote 0x7f5ce75e6880>, arg=(123,), kw=0x0) at /home/kirr/src/tools/go/pygolang/x.c:3414
    cython#4  0x00007f5ce75df276 in __pyx_pymod_exec_x (__pyx_pyinit_module=<optimized out>) at /home/kirr/src/tools/go/pygolang/x.c:3017
    (More stack frames follow...)

    (gdb) f 1
    cython#1  __pyx_f_1x__trace_tp_new (__pyx_v_cls=0x7f5ce75e6880 <__pyx_type_1x_X>, __pyx_v_args=(123,), __pyx_v_kw=0x0) at /home/kirr/src/tools/go/pygolang/x.c:1986
    1986      __Pyx_INCREF(__pyx_v_kw);
    ```

-> Change newfunc signature to use PyObject* instead of object to fix it.

With this fix, and test example updates to account for object -> PyObject* change as follows ...

    --- a/x.pyx.kirr
    +++ b/x.pyx
    @@ -1,5 +1,5 @@
     # cython: language_level=3
    -from cpython cimport newfunc, type as cpytype, Py_TYPE
    +from cpython cimport newfunc, type as cpytype, Py_TYPE, PyObject

     cdef class X:
         cdef int i
    @@ -10,8 +10,12 @@ cdef class X:

     cdef newfunc _orig_tp_new = Py_TYPE(X(0)).tp_new

    -cdef object _trace_tp_new(cpytype cls, object args, object kw):
    -    print('_trace_tp_new', cls, args, kw)
    +cdef object xobject(PyObject* x):
    +    return "null"  if x == NULL  else \
    +           <object>x
    +
    +cdef object _trace_tp_new(cpytype cls, PyObject* args, PyObject* kw):
    +    print('_trace_tp_new', cls, xobject(args), xobject(kw))
         return _orig_tp_new(cls, args, kw)

     Py_TYPE(X(0)).tp_new = _trace_tp_new

... it works as expected without crashing:

    $ python -c 'import x'
    _trace_tp_new <type 'x.X'> (123,) null
    X(123)

Fixes: cython#4822
@navytux navytux changed the base branch from master to 0.29.x June 5, 2022 13:14
@da-woods
Copy link
Contributor

da-woods commented Jun 7, 2022

This looks good to me (although I'll wait for other comments).

My feeling is that your "reassign tp_new use-case is maybe too hacky to go in our test-suite so this is possibly best left untested

@navytux
Copy link
Contributor Author

navytux commented Jun 7, 2022

@da-woods, thanks for feedback. I share your feelings.

And btw, the tp_new hack won't work on PyPy, where patching PyTypeObject at runtime does not work (see e.g. https://foss.heptapod.net/pypy/pypy/-/issues/3588).

@da-woods da-woods added this to the 0.29.31 milestone Jul 3, 2022
@da-woods da-woods merged commit 7c78903 into cython:0.29.x Jul 3, 2022
@da-woods
Copy link
Contributor

da-woods commented Jul 3, 2022

Thanks @navytux

@navytux
Copy link
Contributor Author

navytux commented Jul 3, 2022

@da-woods, thanks for merging.

@navytux navytux deleted the y/newfunc branch July 3, 2022 13:57
@scoder scoder modified the milestones: 0.29.31, 3.0 Jul 4, 2022
scoder added a commit that referenced this pull request Jul 4, 2022
scoder pushed a commit that referenced this pull request Jul 4, 2022
…d of object (#4823)

object means the argument is always non-NULL valid Python object, while
PyObject* argument can be generally NULL. If the argument is indeed
passed as NULL, and we declare it as object, generated code will crash
while trying to incref it.

Quoting #4822:

    object.pxd currently declares `newfunc` as follows:

    ```pyx
    ctypedef object (*newfunc)(cpython.type.type, object, object)  # (type, args, kwargs)
    ```

    which implies that `args` and `kwargs` are always live objects and cannot be NULL.

    However Python can, and does, call tp_new with either args=NULL, or kwargs=NULL or both. And in such cases this leads to segfault in automatically-generated __Pyx_INCREF for args or kw.

    The fix is to change `object` to `PyObject*` for both args and kwargs.

    Please see below for details:

    ```cython
    # cython: language_level=3
    from cpython cimport newfunc, type as cpytype, Py_TYPE

    cdef class X:
        cdef int i
        def __init__(self, i):
            self.i = i
        def __repr__(self):
            return 'X(%d)' % self.i

    cdef newfunc _orig_tp_new = Py_TYPE(X(0)).tp_new

    cdef object _trace_tp_new(cpytype cls, object args, object kw):
        print('_trace_tp_new', cls, args, kw)
        return _orig_tp_new(cls, args, kw)

    Py_TYPE(X(0)).tp_new = _trace_tp_new

    x = X(123)
    print(x)
    ```

    ```console
    (neo) (py3.venv) (g.env) kirr@deca:~/src/tools/go/pygolang$ cythonize -i x.pyx
    Compiling /home/kirr/src/tools/go/pygolang/x.pyx because it changed.
    [1/1] Cythonizing /home/kirr/src/tools/go/pygolang/x.pyx
    running build_ext
    building 'x' extension
    ...
    x86_64-linux-gnu-gcc -pthread -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O2 -Wall -g -ffile-prefix-map=/build/python3.9-RNBry6/python3.9-3.9.2=. -fstack-protector-strong -Wformat -Werror=format-security -g -fwrapv -O2 -g -ffile-prefix-map=/build/python3.9-RNBry6/python3.9-3.9.2=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -I/home/kirr/src/wendelin/venv/py3.venv/include -I/usr/include/python3.9 -c /home/kirr/src/tools/go/pygolang/x.c -o /home/kirr/src/tools/go/pygolang/tmpqkz1r96s/home/kirr/src/tools/go/pygolang/x.o
    x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-z,relro -g -fwrapv -O2 -Wl,-z,relro -g -fwrapv -O2 -g -ffile-prefix-map=/build/python3.9-RNBry6/python3.9-3.9.2=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 /home/kirr/src/tools/go/pygolang/tmpqkz1r96s/home/kirr/src/tools/go/pygolang/x.o -o /home/kirr/src/tools/go/pygolang/x.cpython-39-x86_64-linux-gnu.so
    ```

    ```console
    (neo) (py3.venv) (g.env) kirr@deca:~/src/tools/go/pygolang$ python -c 'import x'
    Ошибка сегментирования (стек памяти сброшен на диск)
    ```

    ```console
    (neo) (py3.venv) (g.env) kirr@deca:~/src/tools/go/pygolang$ gdb python core
    ...
    Reading symbols from python...
    Reading symbols from /usr/lib/debug/.build-id/f9/02f8a561c3abdb9c8d8c859d4243bd8c3f928f.debug...
    [New LWP 218557]
    [Thread debugging using libthread_db enabled]
    Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
    Core was generated by `python -c import x'.
    Program terminated with signal SIGSEGV, Segmentation fault.
    #0  _Py_INCREF (op=0x0) at /usr/include/python3.9/object.h:408
    408         op->ob_refcnt++;

    (gdb) bt 5
    #0  _Py_INCREF (op=0x0) at /usr/include/python3.9/object.h:408
    #1  __pyx_f_1x__trace_tp_new (__pyx_v_cls=0x7f5ce75e6880 <__pyx_type_1x_X>, __pyx_v_args=(123,), __pyx_v_kw=0x0) at /home/kirr/src/tools/go/pygolang/x.c:1986
    #2  0x000000000051dd7e in type_call (type=type@entry=0x7f5ce75e6880 <__pyx_type_1x_X>, args=args@entry=(123,), kwds=kwds@entry=0x0)
        at ../Objects/typeobject.c:1014
    #3  0x00007f5ce75df8d4 in __Pyx_PyObject_Call (func=<type at remote 0x7f5ce75e6880>, arg=(123,), kw=0x0) at /home/kirr/src/tools/go/pygolang/x.c:3414
    #4  0x00007f5ce75df276 in __pyx_pymod_exec_x (__pyx_pyinit_module=<optimized out>) at /home/kirr/src/tools/go/pygolang/x.c:3017
    (More stack frames follow...)

    (gdb) f 1
    #1  __pyx_f_1x__trace_tp_new (__pyx_v_cls=0x7f5ce75e6880 <__pyx_type_1x_X>, __pyx_v_args=(123,), __pyx_v_kw=0x0) at /home/kirr/src/tools/go/pygolang/x.c:1986
    1986      __Pyx_INCREF(__pyx_v_kw);
    ```

-> Change newfunc signature to use PyObject* instead of object to fix it.

With this fix, and test example updates to account for object -> PyObject* change as follows ...

    --- a/x.pyx.kirr
    +++ b/x.pyx
    @@ -1,5 +1,5 @@
     # cython: language_level=3
    -from cpython cimport newfunc, type as cpytype, Py_TYPE
    +from cpython cimport newfunc, type as cpytype, Py_TYPE, PyObject

     cdef class X:
         cdef int i
    @@ -10,8 +10,12 @@ cdef class X:

     cdef newfunc _orig_tp_new = Py_TYPE(X(0)).tp_new

    -cdef object _trace_tp_new(cpytype cls, object args, object kw):
    -    print('_trace_tp_new', cls, args, kw)
    +cdef object xobject(PyObject* x):
    +    return "null"  if x == NULL  else \
    +           <object>x
    +
    +cdef object _trace_tp_new(cpytype cls, PyObject* args, PyObject* kw):
    +    print('_trace_tp_new', cls, xobject(args), xobject(kw))
         return _orig_tp_new(cls, args, kw)

     Py_TYPE(X(0)).tp_new = _trace_tp_new

... it works as expected without crashing:

    $ python -c 'import x'
    _trace_tp_new <type 'x.X'> (123,) null
    X(123)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Includes: declaration for newfunc is wrong

3 participants