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

[BUG] Infinite loop/crash on __add__ #4172

Closed
da-woods opened this issue May 18, 2021 · 4 comments · Fixed by #4204
Closed

[BUG] Infinite loop/crash on __add__ #4172

da-woods opened this issue May 18, 2021 · 4 comments · Fixed by #4204

Comments

@da-woods
Copy link
Contributor

da-woods commented May 18, 2021

Describe the bug
It's possible to get an infinite loop for an add (and presumably other operators) operator that returns NotImplemented when used with a derived class.

To Reproduce

cdef class Base:
    def __add__(self, other):
        return NotImplemented

class Derived(Base):
    pass

and to test:

>>> import hmmm
>>> x = hmmm.Base()
>>> y = hmmm.Derived()
>>> y+x   # works OK as expected
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for +: 'Derived' and 'hmmm.Base'
>>> x+y

For x+y I get an infinite loop but Pandas reports segmentation fault - probably just to do with tail-call recursion optimization or not.

Expected behavior

Should just be NotImplementedError

Environment (please complete the following information):

  • OS: Linux
  • Python version 3.8.9
  • Cython version 0e80efb (close to current master)

Additional context

I think this is to do with the changes to support __radd__ etc.. I think it's somewhere in:

static PyObject *{{func_name}}(PyObject *left, PyObject *right {{extra_arg_decl}}) {

However, I haven't traced the error through in great detail so not 100% sure exactly what's gone wrong.

rainwoodman added a commit to rainwoodman/pandas that referenced this issue May 18, 2021
rainwoodman added a commit to rainwoodman/pandas that referenced this issue May 18, 2021
@matusvalo
Copy link
Contributor

I was playing with this issue recently. I did git bisect and the issue first occurred in following commit:

7ac25eea68798f46f79606cfa40a82fabd51534e is the first bad commit
commit 7ac25eea68798f46f79606cfa40a82fabd51534e
Author: Robert Bradshaw <robertwb@gmail.com>
Date:   Wed Jun 17 13:13:28 2020 -0700

    Change the default of the "c_api_binop_methods" directive to False. (GH-3644)

    This is a backwards incompatible change that enables Python semantics for special methods by default, while the directive allows users to opt out of it and go back to the previous C-ish Cython semantics.

    See https://github.com/cython/cython/issues/2056

 Cython/Compiler/Options.py       |  2 +-
 tests/run/unicode_formatting.pyx | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)
bisect run success

I have checked the commit and the change is only changing default value of c_api_binop_methods. I have double-checked and it seems that also in master setting c_api_binop_methods=True is fixing the issue:

Traceback (most recent call last):
  File "/home/matus/dev/cython/t.py", line 2, in <module>
    print(test.Derived() + test.Derived())
TypeError: unsupported operand type(s) for +: 'Derived' and 'Derived'

I have also tried to find commit before bisected commit but I was not able to find any usable. Not sure whether my findings are useful or not...

@da-woods
Copy link
Contributor Author

Thanks. I think the commit you identify is definitely the point where is changed. I'm not sure bisecting further back is too useful.

I am looking into it (slightly slowly...). You can get it not to crash by using specific_known_type->tp_base rather than Py_TYPE(left_or_right)->tp_base, which is an improvement but doesn't quite bring it in line with Python behaviour. I suspect we'll have to compromise a bit on how closely we can hit Python behaviour though.

@da-woods
Copy link
Contributor Author

So...

There's a quick-fix patch that avoids the recursion/crash but doesn't necessarily get the right behaviour for some corner cases. I can create a PR for that if needed. It's basically b66b0ac, but with the tests tweaked a little. I was going to suggest that as a short-term fix for the next release, but I think I missed the window to do that.

I think in the longer-term calling inherited nb_add (etc.) slots is doomed to failure. I think the best option is to call the specific __add__/__radd__ functions. This can be resolved at compile-time in a lot of cases, but in the general case it'll require the __add__/__radd__ functions to also be available via the class dict.

@scoder
Copy link
Contributor

scoder commented May 25, 2021

I missed the window to do that

There's always a next release. This bug is (annoying but) not critical and the next 3.0 release is hopefully not far away.

@scoder scoder added this to the 3.0 milestone Jul 2, 2021
scoder pushed a commit that referenced this issue Jul 2, 2021
da-woods added a commit to da-woods/cython that referenced this issue Jul 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants