8000 bpo-38395: Fix ownership in weakref.proxy methods (GH-16632) · python/cpython@10cd00a · GitHub
[go: up one dir, main page]
Skip to content

Commit 10cd00a

Browse files
pablogsalpablogsal
authored
bpo-38395: Fix ownership in weakref.proxy methods (GH-16632)
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.
1 parent 03ab6b4 commit 10cd00a

File tree

3 files changed

+111
-31
lines changed

3 files changed

+111
-31
lines changed

‎Lib/test/test_weakref.py

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -391,6 +391,26 @@ class List(list): pass
391391
lyst = List()
392392
self.assertEqual(bool(weakref.proxy(lyst)), bool(lyst))
393393

394+
def test_proxy_iter(self):
395+
# Test fails with a debug build of the interpreter
396+
# (see bpo-38395).
397+
398+
obj = None
399+
400+
class MyObj:
401+
def __iter__(self):
402+
nonlocal obj
403+
del obj
404+
return NotImplemented
405+
406+
obj = MyObj()
407+
p = weakref.proxy(obj)
408+
with self.assertRaises(TypeError):
409+
# "blech" in p calls MyObj.__iter__ through the proxy,
410+
# without keeping a reference to the real object, so it
411+
# can be killed in the middle of the call
412+
"blech" in p
413+
394414
def test_getweakrefcount(self):
395415
o = C()
396416
ref1 = weakref.ref(o)

‎Misc/NEWS.d/next/C API/2019-10-08-01-23-24.bpo-38395.MJ6Ey9.rst

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Fix a crash in :class:`weakref.proxy` objects due to incorrect lifetime
2+
management when calling some associated methods that may delete the last
3+
reference to object being referenced by the proxy. Patch by Pablo Galindo.

‎Objects/weakrefobject.c

Lines changed: 88 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -145,11 +145,14 @@ weakref_hash(PyWeakReference *self)
145145
{
146146
if (self->hash != -1)
147147
return self->hash;
148-
if (PyWeakref_GET_OBJECT(self) == Py_None) {
148+
PyObject* obj = PyWeakref_GET_OBJECT(self);
149+
if (obj == Py_None) {
149150
PyErr_SetString(PyExc_TypeError, "weak object has gone away");
150151
return -1;
151152
}
152-
self->hash = PyObject_Hash(PyWeakref_GET_OBJECT(self));
153+
Py_INCREF(obj);
154+
self->hash = PyObject_Hash(obj);
155+
Py_DECREF(obj);
153156
return self->hash;
154157
}
155158

@@ -159,28 +162,33 @@ weakref_repr(PyWeakReference *self)
159162
{
160163
PyObject *name, *repr;
161164
_Py_IDENTIFIER(__name__);
165+
PyObject* obj = PyWeakref_GET_OBJECT(self);
162166

163-
if (PyWeakref_GET_OBJECT(self) == Py_None)
167+
if (obj == Py_None) {
164168
return PyUnicode_FromFormat("<weakref at %p; dead>", self);
169+
}
165170

166-
if (_PyObject_LookupAttrId(PyWeakref_GET_OBJECT(self), &PyId___name__, &name) < 0) {
171+
Py_INCREF(obj);
172+
if (_PyObject_LookupAttrId(obj, &PyId___name__, &name) < 0) {
173+
Py_DECREF(obj);
167174
return NULL;
168175
}
169176
if (name == NULL || !PyUnicode_Check(name)) {
170177
repr = PyUnicode_FromFormat(
171178
"<weakref at %p; to '%s' at %p>",
172179
self,
173180
Py_TYPE(PyWeakref_GET_OBJECT(self))->tp_name,
174-
PyWeakref_GET_OBJECT(self));
181+
obj);
175182
}
176183
else {
177184
repr = PyUnicode_FromFormat(
178185
"<weakref at %p; to '%s' at %p (%U)>",
179186
self,
180187
Py_TYPE(PyWeakref_GET_OBJECT(self))->tp_name,
181-
PyWeakref_GET_OBJECT(self),
188+
obj,
182189
name);
183190
}
191+
Py_DECREF(obj);
184192
Py_XDECREF(name);
185193
return repr;
186194
}
@@ -207,8 +215,14 @@ weakref_richcompare(PyWeakReference* self, PyWeakReference* other, int op)
207215
else
208216
Py_RETURN_FALSE;
209217
}
210-
return PyObject_RichCompare(PyWeakref_GET_OBJECT(self),
211-
PyWeakref_GET_OBJECT(other), op);
218+
PyObject* obj = PyWeakref_GET_OBJECT(self);
219+
PyObject* other_obj = PyWeakref_GET_OBJECT(other);
220+
Py_INCREF(obj);
221+
Py_INCREF(other_obj);
222+
PyObject* res = PyObject_RichCompare(obj, other_obj, op);
223+
Py_DECREF(obj);
224+
Py_DECREF(other_obj);
225+
return res;
212226
}
213227

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

418-
#define UNWRAP_I(o) \
419-
if (PyWeakref_CheckProxy(o)) { \
420-
if (!proxy_checkref((PyWeakReference *)o)) \
421-
return -1; \
422-
o = PyWeakref_GET_OBJECT(o); \
423-
}
424-
425432
#define WRAP_UNARY(method, generic) \
426433
static PyObject * \
427434
method(PyObject *proxy) { \
428435
UNWRAP(proxy); \
429-
return generic(proxy); \
436+
Py_INCREF(proxy); \
437+
PyObject* res = generic(proxy); \
438+
Py_DECREF(proxy); \
439+
return res; \
430440
}
431441

432442
#define WRAP_BINARY(method, generic) \
433443
static PyObject * \
434444
method(PyObject *x, PyObject *y) { \
435445
UNWRAP(x); \
436446
UNWRAP(y); \
437-
return generic(x, y); \
447+
Py_INCREF(x); \
448+
Py_INCREF(y); \
449+
PyObject* res = generic(x, y); \
450+
Py_DECREF(x); \
451+
Py_DECREF(y); \
452+
return res; \
438453
}
439454

440455
/* Note that the third arg needs to be checked for NULL since the tp_call
@@ -447,15 +462,25 @@ proxy_checkref(PyWeakReference *proxy)
447462
UNWRAP(v); \
448463
if (w != NULL) \
449464
UNWRAP(w); \
450-
return generic(proxy, v, w); \
465+
Py_INCREF(proxy); \
466+
Py_INCREF(v); \
467+
Py_XINCREF(w); \
468+
PyObject* res = generic(proxy, v, w); \
469+
Py_DECREF(proxy); \
470+
Py_DECREF(v); \
471+
Py_XDECREF(w); \
472+
return res; \
451473
}
452474

453475
#define WRAP_METHOD(method, special) \
454476
static PyObject * \
455477
method(PyObject *proxy, PyObject *Py_UNUSED(ignored)) { \
456478
_Py_IDENTIFIER(special); \
457479
UNWRAP(proxy); \
458-
return _PyObject_CallMethodIdNoArgs(proxy, &PyId_##special); \
480+
Py_INCREF(proxy); \
481+
PyObject* res = _PyObject_CallMethodIdNoArgs(proxy, &PyId_##special); \
482+
Py_DECREF(proxy); \
483+
return res; \
459484
}
460485

461486

@@ -481,7 +506,11 @@ proxy_setattr(PyWeakReference *proxy, PyObject *name, PyObject *value)
481506
{
482507
if (!proxy_checkref(proxy))
483508
return -1;
484-
return PyObject_SetAttr(PyWeakref_GET_OBJECT(proxy), name, value);
509+
PyObject *obj = PyWeakref_GET_OBJECT(proxy);
510+
Py_INCREF(obj);
511+
int res = PyObject_SetAttr(obj, name, value);
512+
Py_DECREF(obj);
513+
return res;
485514
}
486515

487516
static PyObject *
@@ -532,9 +561,13 @@ static int
532561
proxy_bool(PyWeakReference *proxy)
533562
{
534563
PyObject *o = PyWeakref_GET_OBJECT(proxy);
535-
if (!proxy_checkref(proxy))
564+
if (!proxy_checkref(proxy)) {
536565
return -1;
537-
return PyObject_IsTrue(o);
566+
}
567+
Py_INCREF(o);
568+
int res = PyObject_IsTrue(o);
569+
Py_DECREF(o);
570+
return res;
538571
}
539572

540573
static void
@@ -553,9 +586,13 @@ proxy_contains(PyWeakReference *proxy, PyObject *value)
553586
{
554587
if (!proxy_checkref(proxy))
555588
return -1;
556-
return PySequence_Contains(PyWeakref_GET_OBJECT(proxy), value);
557-
}
558589

590+
PyObject *obj = PyWeakref_GET_OBJECT(proxy);
591+
Py_INCREF(obj);
592+
int res = PySequence_Contains(obj, value);
593+
Py_DECREF(obj);
594+
return res;
595+
}
559596

560597
/* mapping slots */
561598

@@ -564,7 +601,12 @@ proxy_length(PyWeakReference *proxy)
564601
{
565602
if (!proxy_checkref(proxy))
566603
return -1;
567-
return PyObject_Length(PyWeakref_GET_OBJECT(proxy));
604+
605+
PyObject *obj = PyWeakref_GET_OBJECT(proxy);
606+
Py_INCREF(obj);
607+
Py_ssize_t res = PyObject_Length(obj);
608+
Py_DECREF(obj);
609+
return res;
568610
}
569611

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

578-
if (value == NULL)
579-
return PyObject_DelItem(PyWeakref_GET_OBJECT(proxy), key);
580-
else
581-
return PyObject_SetItem(PyWeakref_GET_OBJECT(proxy), key, value);
620+
PyObject *obj = PyWeakref_GET_OBJECT(proxy);
621+
Py_INCREF(obj);
622+
int res;
623+
if (value == NULL) {
624+
res = PyObject_DelItem(obj, key);
625+
} else {
626+
res = PyObject_SetItem(obj, key, value);
627+
}
628+
Py_DECREF(obj);
629+
return res;
582630
}
583631

584632
/* iterator slots */
@@ -588,15 +636,24 @@ proxy_iter(PyWeakReference *proxy)
588636
{
589637
if (!proxy_checkref(proxy))
590638
return NULL;
591-
return PyObject_GetIter(PyWeakref_GET_OBJECT(proxy));
639+
PyObject *obj = PyWeakref_GET_OBJECT(proxy);
640+
Py_INCREF(obj);
641+
PyObject* res = PyObject_GetIter(obj);
642+
Py_DECREF(obj);
643+
return res;
592644
}
593645

594646
static PyObject *
595647
proxy_iternext(PyWeakReference *proxy)
596648
{
597649
if (!proxy_checkref(proxy))
598650
return NULL;
599-
return PyIter_Next(PyWeakref_GET_OBJECT(proxy));
651+
652+
PyObject *obj = PyWeakref_GET_OBJECT(proxy);
653+
Py_INCREF(obj);
654+
PyObject* res = PyIter_Next(obj);
655+
Py_DECREF(obj);
656+
return res;
600657
}
601658

602659

0 commit comments

Comments
 (0)
0