[Python-checkins] [3.7] bpo-38395: Fix ownership in weakref.proxy methods (GH-16632) (GH-16663)
Miss Islington (bot)
webhook-mailer at python.org
Fri Oct 11 16:29:09 EDT 2019
https://github.com/python/cpython/commit/193366e25c4f84a58d2f6c6c577fd9f0143dc6e1
commit: 193366e25c4f84a58d2f6c6c577fd9f0143dc6e1
branch: 3.7
author: Pablo Galindo <Pablogsal at gmail.com>
committer: Miss Islington (bot) <31488909+miss-islington at users.noreply.github.com>
date: 2019-10-11T13:28:49-07:00
summary:
[3.7] bpo-38395: Fix ownership in weakref.proxy methods (GH-16632) (GH-16663)
The implementation of weakref.proxy's methods call back into the Python
API using a borrowed references of the weakly referenced object
(acquired via PyWeakref_GET_OBJECT). This API call may delete the last
reference to the object (either directly or via GC), leaving a dangling
pointer, which can be subsequently dereferenced.
To fix this, claim a temporary ownership of the referenced object when
calling the appropriate method. Some functions because at the moment they
do not need to access the borrowed referent, but to protect against
future changes to these functions, ownership need to be fixed in
all potentially affected methods..
(cherry picked from commit 10cd00a9e3c22af37c748ea5a417f6fb66601e21)
Co-authored-by: Pablo Galindo <Pablogsal at gmail.com>
https://bugs.python.org/issue38395
files:
A Misc/NEWS.d/next/C API/2019-10-08-01-23-24.bpo-38395.MJ6Ey9.rst
M Lib/test/test_weakref.py
M Objects/weakrefobject.c
diff --git a/Lib/test/test_weakref.py b/Lib/test/test_weakref.py
index d3396fc9cf90c..14ec8efdc638e 100644
--- a/Lib/test/test_weakref.py
+++ b/Lib/test/test_weakref.py
@@ -376,6 +376,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)
diff --git a/Misc/NEWS.d/next/C API/2019-10-08-01-23-24.bpo-38395.MJ6Ey9.rst b/Misc/NEWS.d/next/C API/2019-10-08-01-23-24.bpo-38395.MJ6Ey9.rst
new file mode 100644
index 0000000000000..4bc30600ca758
--- /dev/null
+++ b/Misc/NEWS.d/next/C API/2019-10-08-01-23-24.bpo-38395.MJ6Ey9.rst
@@ -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.
diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c
index f600179d0e9ba..4bf662ced1de1 100644
--- a/Objects/weakrefobject.c
+++ b/Objects/weakrefobject.c
@@ -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;
}
@@ -159,11 +162,18 @@ 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);
+ }
+
+ Py_INCREF(obj);
+ if (_PyObject_LookupAttrId(obj, &PyId___name__, &name) < 0) {
+ Py_DECREF(obj);
+ return NULL;
+ }
- name = _PyObject_GetAttrId(PyWeakref_GET_OBJECT(self), &PyId___name__);
if (name == NULL || !PyUnicode_Check(name)) {
if (name == NULL)
PyErr_Clear();
@@ -171,16 +181,17 @@ weakref_repr(PyWeakReference *self)
"<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;
}
@@ -207,8 +218,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
@@ -415,18 +432,14 @@ 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) \
@@ -434,7 +447,12 @@ proxy_checkref(PyWeakReference *proxy)
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
@@ -447,7 +465,14 @@ 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) \
@@ -455,7 +480,10 @@ proxy_checkref(PyWeakReference *proxy)
method(PyObject *proxy) { \
_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; \
}
@@ -481,7 +509,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 *
@@ -530,9 +562,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
@@ -551,9 +587,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 */
@@ -562,7 +602,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)
@@ -573,10 +618,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 */
@@ -586,7 +637,11 @@ 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 *
@@ -594,7 +649,12 @@ 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;
}
More information about the Python-checkins
mailing list