Fix comparison of KeyReferenceToPersistent with a proxied one.

[icemac]

This got broken somewhere between 3.6.2 an 4.x: KeyReferenceToPersistent.object
is not part of the interface so it cannot be accessed on a security proxied
object. The interface suggests to use __call__() to get the object.

[jamadden]

I have so many questions 😄

First, I can't find any evidence that security-proxied objects were ever supported in comparisons. The very first revision of persistent.py in this repository (2009's 3.5.0 of zope.app.keyreference) doesn't use object() or import zope.security. Nor does the corresponding version of interfaces.py make any mention of object at that time, and the configure.zcml doesn't make any special dispensation for that attribute either. Going further back to the earliest version on PyPI, 3.4.0 it's the same thing. Checking 3.6.2 (after the rename to zope.keyreference) also doesn't show any special support, and I didn't find anything else later that did.

So when and how did this ever actually work? Was there some change in zope.security that "broke" it? I reviewed its changes since 2009 and didn't see anything obvious to me.

(My apologies if I'm missing something obvious here and those questions are gibberish. I rarely-to-never use security-proxied objects, so I'm going off the description in this PR of what the problem is.)

If we can figure out that it did work at one point, but has been not-working for the past ~8 years, is fixing it actually worthwhile at this point? (I assume it's causing you problems or else you wouldn't have this PR 😄 ) Maybe people have come to depend (unknowingly?) on the current behaviour?

If so, is it worth adding a hard dependency on zope.security? That's a fairly large one, being as it involves C extensions. That could be a breaking change for some buildouts that don't allow non-picked versions that didn't have a zope.security dependency already. Can it be made soft?

Last (finally!), can there be a test case that shows it was broken and is now fixed?

Some of these seem like important enough questions that it might be good to get some more eyeballs from other contributors ( @tseaver ?). Not that I'm trying to pass the buck 😉

[icemac]

@jamadden Thank you for looking so deep into this PR.

A bit about what I do and where the code breaks so I wrote this PR.

I call zope.intid.IntIds.getId() using a security proxied object. Inside it adapts the object to IKeyReference (which is still security proxied) and calls __getitem__ on its ids which is a BTree. This invokes the comparison method.

Until zope.keyreference 3.6.2 this method is __cmp__. Surprisingly this method gets an unproxied other argument. So everything is fine.

Since zope.keyreference 4.0.0a1 there is a __eq__ method which is preferred over __cmp__. But __eq__ gets a security proxied other argument. So I wrote this PR to unwrap it.

When I remove __eq__ in 4.0.0a1 __cmp__ is used again and it does not break while accessing other.object.

Conclusion: There seems to be a special handling of __cmp__ in the security proxies which does not happen on the other comparison methods.

Unfortunately I cannot debug this deeper because the pure python variant of the security proxy does not run with my code. It breaks somewhere in zope.publisher way before hitting zope.intid.

@tseaver Do you see a chance to "fix" the proxy of zope.security to behave equally for __cmp__ and the other comparison methods? Or is the unwrapping on __cmp__ to be seen as a security hole which should be fixed there?

[jamadden]

Unfortunately I cannot debug this deeper because the pure python variant of the security proxy does not run with my code. It breaks somewhere in zope.publisher way before hitting zope.intid.

If you were running into zopefoundation/zope.proxy#16 that's now fixed in master.

CPython's comparison logic is complex to follow. Internally it basically still wants to use the __cmp__ version of things (because that's easiest in C) but it wants to map that to the fancy "rich comparison" functions for Python objects. So there's a lot of flags (the important one being HAVE_RICHCOMPARE) and indirection.

Objects implemented in Python automatically have the HAVE_RICHCOMPARE flag set, and the tp_richcompare C slot filled in with slot_tp_richcompare which simply delegates the operation to the appropriate dunder method like __eq__, either on self (if self implements it) or on other (if other implements it).

A C proxy manually fills in the C tp_richcompare slot and sets the HAVE_RICHCOMPARE flag. That means that any comparison where the LHS is a security proxy will call wrap_richcompare. That immediately unwraps self and returns PyObject_RichCompare. Let's say we have Proxy(KeyReference(A) == Proxy(KeyReference(B). That becomes PyObject_RichCompare(keyreference, proxy, '__eq__').

The guts of PyObject_RichCompare have a fast-path for when both objects are the exact same type. That's not true, so it falls through to do_richcmp which simply calls try_rich_compare. I'll produce that code here (as implemented in 2.7), with types filled in and results annotated for the path that gets taken when the keyreference has __eq__:

static PyObject *
try_rich_compare(KeyReference *self, Proxy *other, int op)
{
    richcmpfunc f;
    PyObject *res;

    if (self->ob_type != other->ob_type && /* True */
        PyType_IsSubtype(other->ob_type, self->ob_type) /* False */ &&
        ...) {
        /* Doesn't matter, not taken */
    }
    if ((f = RICHCOMPARE(self->ob_type)) != NULL) { /* True */
        /* f will be slot_tp_richcompare, which will delegate to the 
            Python method __eq__ KeyReference has __eq__, so we call it. */ 
        res = (*f)(v, w, op); /* BOOM: Raise exception, return NULL, propagate exception */
        if (res != Py_NotImplemented)
            return res; /* And we're out */
        Py_DECREF(res);
    }
    if (...) /* not important, not taken */
}

Ok, so what about the case where we don't have __eq__? Let's look at try_rich_compare in that case:

static PyObject *
try_rich_compare(KeyReference *self, Proxy *other, int op)
{
    richcmpfunc f;
    PyObject *res;

    if (self->ob_type != other->ob_type && /* True */
        PyType_IsSubtype(other->ob_type, self->ob_type) /* False */ &&
        ...) {
        /* Doesn't matter, not taken */
    }
    if ((f = RICHCOMPARE(self->ob_type)) != NULL) { /* True */
        /* f will be slot_tp_richcompare, which will delegate to the 
            Python method __eq__. We DONT have __eq__, 
            and neither does the C proxy (because __eq__ is only 
            looked up on the type, bypassing __getattribute__), 
            so it returns Py_NotImplemented */
        res = (*f)(v, w, op); /* Py_NotImplemented */
        Py_DECREF(res);
    }
    if ((f = RICHCOMPARE(other->ob_type)) != NULL) {
        /* Proxy has rich compare so we call it with swapped arguments: wrap_richcompare */
        return (*f)(w, v, _Py_SwappedOp[op]);
    }
}

We can see that if we (self) don't have __eq__ we wind up looking at other. In this case, other is a Proxy, so we call wrap_richcompare, but with the arguments reversed (and the opposite operation). So it winds up unwrapping other and the whole thing starts over. So in this way, when there is no __eq__, the operation Proxy(KeyReference(A)) == Proxy(KeyReference(B)) gets transformed to KeyReference(B) != KeyReference(A)!

An important side note would be what happens if the Python versions of the proxies are in use: They do provide an __eq__ and __ne__ method, so that second branch in slot_tp_richcompare will get taken, we'll end up invoking ProxyPy.__ne__(other, self) which unwraps other (because other comes in as self to the reversed operation) and things should just work.

Ok, that said, I think the fact that adding __eq__ and the like to an object changes the semantics of a C implemented proxy to start calling them without unrwapping both arguments could be considered a bug in the C implementation of zope.security. I think it can be fixed by having the C implementation manually implement __eq__, etc, by unwrapping self, just like the Python implementation does.

[jamadden]

The test cases for zope.app.catalog produce this now 😦

[icemac]

@jamadden Thank you for your excellent in depth analysis. Sorry for picking up this after so much time, but this problem occurred in a hobby project which I eventually want to migrate to Python 3.

Did you find a solution for tests in zope.app.catalog?

[sallner]

I have released a package, which can be installed to apply the changes of this PR in an actual project, if really needed.
https://pypi.org/project/gocept.patch-keyreferences/