zopefoundation / persistent

automatic persistence for Python objects
https://pypi.org/project/persistent/
Other
46 stars 28 forks source link

cPickleCache uses sizeof(X) when it means isinstance(X,persistent) #69

Closed jamadden closed 6 years ago

jamadden commented 6 years ago

From cPickleCache.c's cc_add function:

    else if (v->ob_type->tp_basicsize < sizeof(cPersistentObject))
    {
        /* If it's not an instance of a persistent class, (ie Python
            classes that derive from persistent.Persistent, BTrees,
            etc), report an error.
            TODO:  checking sizeof() seems a poor test.
        */
        PyErr_SetString(PyExc_TypeError,
                        "Cache values must be persistent objects.");
        return -1;
    }

I think I agree with the code's own comment that 'checking sizeof() seems a poor test'. Why does it do that instead of using PyObject_IsInstance (which is what the Python implementation of PickleCache does)? Could that check be changed?

(Ref https://github.com/zopefoundation/Persistence/pull/9)

tseaver commented 6 years ago

That change seems "safe" (won't cause coredumps), because the PyObject_IsInstance test should be a strict subset of the size check. I don't know if there is any code in the wild which is using not-actually-instances-of-Persistent objects which are accidentally big enough (and have fakey slots to boot), but ISTM that such hypothetical code Deserves to Lose(TM).