Skip to content

[3.8] bpo-38395: Fix ownership in weakref.proxy methods (GH-16632) #16662

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 8, 2019
Merged

[3.8] bpo-38395: Fix ownership in weakref.proxy methods (GH-16632) #16662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions Lib/test/test_weakref.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,26 @@ class List(list): pass
lyst = List()
self.assertEqual(bool(weakref.proxy(lyst)), bool(lyst))

def test_proxy_iter(self):
# Test fails with a debug build of the interpreter
# (see bpo-38395).

obj = None

class MyObj:
def __iter__(self):
nonlocal obj
del obj
return NotImplemented

obj = MyObj()
p = weakref.proxy(obj)
with self.assertRaises(TypeError):
# "blech" in p calls MyObj.__iter__ through the proxy,
# without keeping a reference to the real object, so it
# can be killed in the middle of the call
"blech" in p

def test_getweakrefcount(self):
o = C()
ref1 = weakref.ref(o)
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/C API/2019-10-08-01-23-24.bpo-38395.MJ6Ey9.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fix a crash in :class:`weakref.proxy` objects due to incorrect lifetime
management when calling some associated methods that may delete the last
reference to object being referenced by the proxy. Patch by Pablo Galindo.
119 changes: 88 additions & 31 deletions Objects/weakrefobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,11 +145,14 @@ weakref_hash(PyWeakReference *self)
{
if (self->hash != -1)
return self->hash;
if (PyWeakref_GET_OBJECT(self) == Py_None) {
PyObject* obj = PyWeakref_GET_OBJECT(self);
if (obj == Py_None) {
PyErr_SetString(PyExc_TypeError, "weak object has gone away");
return -1;
}
self->hash = PyObject_Hash(PyWeakref_GET_OBJECT(self));
Py_INCREF(obj);
self->hash = PyObject_Hash(obj);
Py_DECREF(obj);
return self->hash;
}

Expand All @@ -159,28 +162,33 @@ weakref_repr(PyWeakReference *self)
{
PyObject *name, *repr;
_Py_IDENTIFIER(__name__);
PyObject* obj = PyWeakref_GET_OBJECT(self);

if (PyWeakref_GET_OBJECT(self) == Py_None)
if (obj == Py_None) {
return PyUnicode_FromFormat("<weakref at %p; dead>", self);
}

if (_PyObject_LookupAttrId(PyWeakref_GET_OBJECT(self), &PyId___name__, &name) < 0) {
Py_INCREF(obj);
if (_PyObject_LookupAttrId(obj, &PyId___name__, &name) < 0) {
Py_DECREF(obj);
return NULL;
}
if (name == NULL || !PyUnicode_Check(name)) {
repr = PyUnicode_FromFormat(
"<weakref at %p; to '%s' at %p>",
self,
Py_TYPE(PyWeakref_GET_OBJECT(self))->tp_name,
PyWeakref_GET_OBJECT(self));
obj);
}
else {
repr = PyUnicode_FromFormat(
"<weakref at %p; to '%s' at %p (%U)>",
self,
Py_TYPE(PyWeakref_GET_OBJECT(self))->tp_name,
PyWeakref_GET_OBJECT(self),
obj,
name);
}
Py_DECREF(obj);
Py_XDECREF(name);
return repr;
}
Expand All @@ -207,8 +215,14 @@ weakref_richcompare(PyWeakReference* self, PyWeakReference* other, int op)
else
Py_RETURN_FALSE;
}
return PyObject_RichCompare(PyWeakref_GET_OBJECT(self),
PyWeakref_GET_OBJECT(other), op);
PyObject* obj = PyWeakref_GET_OBJECT(self);
PyObject* other_obj = PyWeakref_GET_OBJECT(other);
Py_INCREF(obj);
Py_INCREF(other_obj);
PyObject* res = PyObject_RichCompare(obj, other_obj, op);
Py_DECREF(obj);
Py_DECREF(other_obj);
return res;
}

/* Given the head of an object's list of weak references, extract the
Expand Down Expand Up @@ -415,26 +429,27 @@ proxy_checkref(PyWeakReference *proxy)
o = PyWeakref_GET_OBJECT(o); \
}

#define UNWRAP_I(o) \
if (PyWeakref_CheckProxy(o)) { \
if (!proxy_checkref((PyWeakReference *)o)) \
return -1; \
o = PyWeakref_GET_OBJECT(o); \
}

#define WRAP_UNARY(method, generic) \
static PyObject * \
method(PyObject *proxy) { \
UNWRAP(proxy); \
return generic(proxy); \
Py_INCREF(proxy); \
PyObject* res = generic(proxy); \
Py_DECREF(proxy); \
return res; \
}

#define WRAP_BINARY(method, generic) \
static PyObject * \
method(PyObject *x, PyObject *y) { \
UNWRAP(x); \
UNWRAP(y); \
return generic(x, y); \
Py_INCREF(x); \
Py_INCREF(y); \
PyObject* res = generic(x, y); \
Py_DECREF(x); \
Py_DECREF(y); \
return res; \
}

/* Note that the third arg needs to be checked for NULL since the tp_call
Expand All @@ -447,15 +462,25 @@ proxy_checkref(PyWeakReference *proxy)
UNWRAP(v); \
if (w != NULL) \
UNWRAP(w); \
return generic(proxy, v, w); \
Py_INCREF(proxy); \
Py_INCREF(v); \
Py_XINCREF(w); \
PyObject* res = generic(proxy, v, w); \
Py_DECREF(proxy); \
Py_DECREF(v); \
Py_XDECREF(w); \
return res; \
}

#define WRAP_METHOD(method, special) \
static PyObject * \
method(PyObject *proxy, PyObject *Py_UNUSED(ignored)) { \
_Py_IDENTIFIER(special); \
UNWRAP(proxy); \
return _PyObject_CallMethodId(proxy, &PyId_##special, NULL); \
Py_INCREF(proxy); \
PyObject* res = _PyObject_CallMethodId(proxy, &PyId_##special, NULL); \
Py_DECREF(proxy); \
return res; \
}


Expand All @@ -481,7 +506,11 @@ proxy_setattr(PyWeakReference *proxy, PyObject *name, PyObject *value)
{
if (!proxy_checkref(proxy))
return -1;
return PyObject_SetAttr(PyWeakref_GET_OBJECT(proxy), name, value);
PyObject *obj = PyWeakref_GET_OBJECT(proxy);
Py_INCREF(obj);
int res = PyObject_SetAttr(obj, name, value);
Py_DECREF(obj);
return res;
}

static PyObject *
Expand Down Expand Up @@ -532,9 +561,13 @@ static int
proxy_bool(PyWeakReference *proxy)
{
PyObject *o = PyWeakref_GET_OBJECT(proxy);
if (!proxy_checkref(proxy))
if (!proxy_checkref(proxy)) {
return -1;
return PyObject_IsTrue(o);
}
Py_INCREF(o);
int res = PyObject_IsTrue(o);
Py_DECREF(o);
return res;
}

static void
Expand All @@ -553,9 +586,13 @@ proxy_contains(PyWeakReference *proxy, PyObject *value)
{
if (!proxy_checkref(proxy))
return -1;
return PySequence_Contains(PyWeakref_GET_OBJECT(proxy), value);
}

PyObject *obj = PyWeakref_GET_OBJECT(proxy);
Py_INCREF(obj);
int res = PySequence_Contains(obj, value);
Py_DECREF(obj);
return res;
}

/* mapping slots */

Expand All @@ -564,7 +601,12 @@ proxy_length(PyWeakReference *proxy)
{
if (!proxy_checkref(proxy))
return -1;
return PyObject_Length(PyWeakref_GET_OBJECT(proxy));

PyObject *obj = PyWeakref_GET_OBJECT(proxy);
Py_INCREF(obj);
Py_ssize_t res = PyObject_Length(obj);
Py_DECREF(obj);
return res;
}

WRAP_BINARY(proxy_getitem, PyObject_GetItem)
Expand All @@ -575,10 +617,16 @@ proxy_setitem(PyWeakReference *proxy, PyObject *key, PyObject *value)
if (!proxy_checkref(proxy))
return -1;

if (value == NULL)
return PyObject_DelItem(PyWeakref_GET_OBJECT(proxy), key);
else
return PyObject_SetItem(PyWeakref_GET_OBJECT(proxy), key, value);
PyObject *obj = PyWeakref_GET_OBJECT(proxy);
Py_INCREF(obj);
int res;
if (value == NULL) {
res = PyObject_DelItem(obj, key);
} else {
res = PyObject_SetItem(obj, key, value);
}
Py_DECREF(obj);
return res;
}

/* iterator slots */
Expand All @@ -588,15 +636,24 @@ proxy_iter(PyWeakReference *proxy)
{
if (!proxy_checkref(proxy))
return NULL;
return PyObject_GetIter(PyWeakref_GET_OBJECT(proxy));
PyObject *obj = PyWeakref_GET_OBJECT(proxy);
Py_INCREF(obj);
PyObject* res = PyObject_GetIter(obj);
Py_DECREF(obj);
return res;
}

static PyObject *
proxy_iternext(PyWeakReference *proxy)
{
if (!proxy_checkref(proxy))
return NULL;
return PyIter_Next(PyWeakref_GET_OBJECT(proxy));

PyObject *obj = PyWeakref_GET_OBJECT(proxy);
Py_INCREF(obj);
PyObject* res = PyIter_Next(obj);
Py_DECREF(obj);
return res;
}


Expand Down