Python/C discrepancy: Python can raise RecursionError if __setattr__ and __getattr__ use providedBy()

(Found while working on collective/contentratings, which has exactly this anti-pattern).

Consider this example code. It creates a class that implements an interface, and that class calls providedBy(self) in both __getattr__ (for unknown attributes) and __setattr__ (for setting attributes). It then instantiates a subclass of that class (which itself does not implement any interfaces):

from zope.interface import providedBy
from zope.interface import implementer
from zope.interface import Interface

class IFoo(Interface):
    pass

@implementer(IFoo)
class C(object):

    def __getattr__(self, name):
        providedBy(self)
        raise AttributeError

    def __setattr__(self, name, value):
        providedBy(self)
        super(C, self).__setattr__(name, value)

class D(C):
    pass

d = D()
d.foo = 42
print("Done")

If we run it with the C optimizations, everything seems fine (at first blush):

$ python foo.py
Done
$

If we use the Python implementation, however, it blows up:

$ PURE_PYTHON=1 python /tmp/foo.py
Traceback (most recent call last):
  File "/tmp/foo.py", line 24, in <module>
    d.foo = 42
  File "/tmp/foo.py", line 17, in __setattr__
    providedBy(self)
  File "///zope/interface/declarations.py", line 1218, in providedBy
    r = ob.__providedBy__
  File "///zope/interface/declarations.py", line 1275, in __get__
    provides = getattr(inst, '__provides__', None)
  File "/tmp/foo.py", line 13, in __getattr__
    providedBy(self)
  File "///zope/interface/declarations.py", line 1218, in providedBy
    r = ob.__providedBy__
  File "///zope/interface/declarations.py", line 1275, in __get__
    provides = getattr(inst, '__provides__', None)
  File "/tmp/foo.py", line 13, in __getattr__
    providedBy(self)
…
RecursionError: maximum recursion depth exceeded while calling a Python object

The providedBy function first asks for the __providedBy__ attribute of the object. For classes that implement an interface, this is a non-data descriptor. The __get__ method of this descriptor in turn asks for the instance's __provides__ attribute to see if the instance has any extra interfaces; if not, it returns what the class implements.

The Python version of this descriptor uses getattr(inst, '__provides__', None) to check for __provides__. In Python 3, this allows exceptions other than AttributeError to be raised:

https://github.com/zopefoundation/zope.interface/blob/4a686fc8d87d398045dc44c1b6a97a2940121800/src/zope/interface/declarations.py#L1258-L1260

The C code, on the other hand, disregards all exceptions that might happen checking for __provides__:

https://github.com/zopefoundation/zope.interface/blob/4a686fc8d87d398045dc44c1b6a97a2940121800/src/zope/interface/_zope_interface_coptimizations.c#L528-L531

Watching closely, we can see that the recursion is still happening with the C code, it just ultimately "bottoms out" and returns NULL, which is then ignored…and all the stack unwound, exceptions discarded, etc.

Most places in the C code that do PyObject_GetAttr follow a NULL result up with PyErr_ExceptionMatches(AttributeError) before deciding to ignore the error. This matches what getattr(…,…,None) does.

I argue that's what should happen here. It's bad to allow deep recursion in the event of errors which are then completely hidden. Indeed, with sys.recursionlimit set high enough, this code even crashes in the bowels of C, which is not good.

It was a bit tricky to simplify this from the failing test case in contentratings. The key is that there must be a subclass of a class that implements something (the super class has both __providedBy__ and __provides__; the subclass has neither). If you don't use a subclass, both C and Python implementations work fine (because the class automatically has __provides__); if the subclass also implements something, they work fine (same reason). The base class, but not the subclass, must also implement an interface; if it doesn't, both C and Python crash (because there is no __providedBy__ descriptor). And actually, the __setattr__ isn't even necessary: calling providedBy() in __getattr__ is enough to trigger this.

The fix for contentratings, by the way, was to check in __getattr__ to see if we're being asked for __provides__ before going through with calling providedBy().

zopefoundation / zope.interface

Python/C discrepancy: Python can raise RecursionError if setattr and getattr use providedBy() #239

zopefoundation / zope.interface

Python/C discrepancy: Python can raise RecursionError if __setattr__ and __getattr__ use providedBy() #239

Python/C discrepancy: Python can raise RecursionError if setattr and getattr use providedBy() #239