From e8b9c8e9d886e8908f31dc7aa26333311ff8b614 Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Mon, 20 Mar 2017 07:06:31 -0500 Subject: [PATCH] Avoid raising a SystemError when clearing slots if setstate() failed. PR #52 introduced a code path to `ghostify` that calls PyErr_Clear() with the intent to avoid propagating AttributeErrors for slots. However, if there is an error (like a POSKeyError) raised by jar.setstate(), then `unghostify` will call ghostify with an error pending. If the object had slots that weren't set and the AttributeError was cleared, so was the pending error from setstate. So when `ghostify` returned NULL that got propagated up to the interpreter which finds no exception and so raises `SystemError: error return without exception set`. This commit makes `unghostify` save and restore the exception state around the call to PyErr_Clear. --- CHANGES.rst | 4 +++- persistent/cPersistence.c | 20 ++++++++++++------ persistent/tests/test_persistence.py | 31 ++++++++++++++++++++++++---- 3 files changed, 44 insertions(+), 11 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 23ce3ac..7bc6c73 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -4,7 +4,9 @@ 4.2.4 (unreleased) ------------------ -- Nothing changed yet. +- Avoid raising a ``SystemError: error return without exception set`` + when loading an object with slots whose jar generates an exception + (such as a ZODB ``POSKeyError``) in ``setstate``. 4.2.3 (2017-03-08) diff --git a/persistent/cPersistence.c b/persistent/cPersistence.c index 7238562..dc02379 100644 --- a/persistent/cPersistence.c +++ b/persistent/cPersistence.c @@ -101,7 +101,7 @@ unghostify(cPersistentObject *self) { /* Create a node in the ring for this unghostified object. */ self->cache->non_ghost_count++; - self->cache->total_estimated_size += + self->cache->total_estimated_size += _estimated_size_in_bytes(self->estimated_size); ring_add(&self->cache->ring_home, &self->ring); Py_INCREF(self); @@ -152,13 +152,14 @@ static void ghostify(cPersistentObject *self) { PyObject **dictptr, *slotnames; + PyObject *errtype, *errvalue, *errtb; /* are we already a ghost? */ if (self->state == cPersistent_GHOST_STATE) return; /* Is it ever possible to not have a cache? */ - if (self->cache == NULL) + if (self->cache == NULL) { self->state = cPersistent_GHOST_STATE; return; @@ -177,7 +178,7 @@ ghostify(cPersistentObject *self) /* If we're ghostifying an object, we better have some non-ghosts. */ assert(self->cache->non_ghost_count > 0); self->cache->non_ghost_count--; - self->cache->total_estimated_size -= + self->cache->total_estimated_size -= _estimated_size_in_bytes(self->estimated_size); ring_del(&self->ring); self->state = cPersistent_GHOST_STATE; @@ -195,6 +196,12 @@ ghostify(cPersistentObject *self) * override __new__ ) */ if (Py_TYPE(self)->tp_new == Pertype.tp_new) { + /* later we might clear an AttributeError but + * if we have a pending exception that still needs to be + * raised so that we don't generate a SystemError. + */ + PyErr_Fetch(&errtype, &errvalue, &errtb); + slotnames = pickle_slotnames(Py_TYPE(self)); if (slotnames && slotnames != Py_None) { @@ -235,6 +242,7 @@ ghostify(cPersistentObject *self) } } Py_XDECREF(slotnames); + PyErr_Restore(errtype, errvalue, errtb); } /* We remove the reference to the just ghosted object that the ring @@ -262,7 +270,7 @@ changed(cPersistentObject *self) if (meth == NULL) return -1; arg = PyTuple_New(1); - if (arg == NULL) + if (arg == NULL) { Py_DECREF(meth); return -1; @@ -371,7 +379,7 @@ pickle_slotnames(PyTypeObject *cls) return NULL; if (n) slotnames = Py_None; - + Py_INCREF(slotnames); return slotnames; } @@ -594,7 +602,7 @@ pickle___setstate__(PyObject *self, PyObject *state) int len; dict = _PyObject_GetDictPtr(self); - + if (!dict) { PyErr_SetString(PyExc_TypeError, diff --git a/persistent/tests/test_persistence.py b/persistent/tests/test_persistence.py index e62820c..055077b 100644 --- a/persistent/tests/test_persistence.py +++ b/persistent/tests/test_persistence.py @@ -16,6 +16,9 @@ import platform import sys + +from persistent._compat import copy_reg + py_impl = getattr(platform, 'python_implementation', lambda: None) _is_pypy3 = py_impl() == 'PyPy' and sys.version_info[0] > 2 _is_jython = py_impl() == 'Jython' @@ -78,22 +81,22 @@ def __init__(self): self.called = 0 def register(self,ob): self.called += 1 - raise NotImplementedError + raise NotImplementedError() def setstate(self,ob): - raise NotImplementedError + raise NotImplementedError() jar = _BrokenJar() jar._cache = self._makeCache(jar) return jar - def _makeOneWithJar(self, klass=None): + def _makeOneWithJar(self, klass=None, broken_jar=False): from persistent.timestamp import _makeOctets OID = _makeOctets('\x01' * 8) if klass is not None: inst = klass() else: inst = self._makeOne() - jar = self._makeJar() + jar = self._makeJar() if not broken_jar else self._makeBrokenJar() jar._cache.new_ghost(OID, inst) # assigns _p_jar, _p_oid return inst, jar, OID @@ -1401,6 +1404,26 @@ def __new__(cls): self.assertEqual(list(jar._loaded), []) self.assertEqual(list(jar._registered), []) + def test_p_invalidate_with_slots_broken_jar(self): + # If jar.setstate() raises a POSKeyError (or any error) + # clearing an object with unset slots doesn't result in a + # SystemError, the original error is propagated + + class Derived(self._getTargetClass()): + __slots__ = ('slot1',) + + # Pre-cache in __slotnames__; cpersistent goes directly for this + # and avoids a call to copy_reg. (If it calls the python code in + # copy_reg, the pending exception will be immediately propagated by + # copy_reg, not by us.) + copy_reg._slotnames(Derived) + + inst, jar, OID = self._makeOneWithJar(Derived, broken_jar=True) + inst._p_invalidate() + self.assertEqual(inst._p_status, 'ghost') + self.assertRaises(NotImplementedError, inst._p_activate) + + def test__p_invalidate_from_sticky(self): inst, jar, OID = self._makeOneWithJar() inst._p_activate() # XXX