From: INADA Naoki Date: Mon, 4 Sep 2017 03:31:41 +0000 (+0900) Subject: bpo-31095: Fix potential crash during GC (GH-3197) X-Git-Tag: v2.7.15rc1~234 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=4cde4bdcc86cb08ee3847500a172cc24eba37ffe;p=python bpo-31095: Fix potential crash during GC (GH-3197) (cherry picked from commit a6296d34a478b4f697ea9db798146195075d496c) --- diff --git a/Doc/extending/newtypes.rst b/Doc/extending/newtypes.rst index 5959e4f2b1..7eadcae5e6 100644 --- a/Doc/extending/newtypes.rst +++ b/Doc/extending/newtypes.rst @@ -748,8 +748,9 @@ simplified:: uniformity across these boring implementations. We also need to provide a method for clearing any subobjects that can -participate in cycles. We implement the method and reimplement the deallocator -to use it:: +participate in cycles. + +:: static int Noddy_clear(Noddy *self) @@ -767,13 +768,6 @@ to use it:: return 0; } - static void - Noddy_dealloc(Noddy* self) - { - Noddy_clear(self); - self->ob_type->tp_free((PyObject*)self); - } - Notice the use of a temporary variable in :c:func:`Noddy_clear`. We use the temporary variable so that we can set each member to *NULL* before decrementing its reference count. We do this because, as was discussed earlier, if the @@ -796,6 +790,23 @@ decrementing of reference counts. With :c:func:`Py_CLEAR`, the return 0; } +Note that :c:func:`Noddy_dealloc` may call arbitrary functions through +``__del__`` method or weakref callback. It means circular GC can be +triggered inside the function. Since GC assumes reference count is not zero, +we need to untrack the object from GC by calling :c:func:`PyObject_GC_UnTrack` +before clearing members. Here is reimplemented deallocator which uses +:c:func:`PyObject_GC_UnTrack` and :c:func:`Noddy_clear`. + +:: + + static void + Noddy_dealloc(Noddy* self) + { + PyObject_GC_UnTrack(self); + Noddy_clear(self); + Py_TYPE(self)->tp_free((PyObject*)self); + } + Finally, we add the :const:`Py_TPFLAGS_HAVE_GC` flag to the class flags:: Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE | Py_TPFLAGS_HAVE_GC, /* tp_flags */ diff --git a/Doc/includes/noddy4.c b/Doc/includes/noddy4.c index 9feb71aae3..0fd4dc3788 100644 --- a/Doc/includes/noddy4.c +++ b/Doc/includes/noddy4.c @@ -46,6 +46,7 @@ Noddy_clear(Noddy *self) static void Noddy_dealloc(Noddy* self) { + PyObject_GC_UnTrack(self); Noddy_clear(self); Py_TYPE(self)->tp_free((PyObject*)self); } diff --git a/Misc/NEWS.d/next/Core and Builtins/2017-08-01-18-48-30.bpo-31095.bXWZDb.rst b/Misc/NEWS.d/next/Core and Builtins/2017-08-01-18-48-30.bpo-31095.bXWZDb.rst new file mode 100644 index 0000000000..ca1f8bafba --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2017-08-01-18-48-30.bpo-31095.bXWZDb.rst @@ -0,0 +1,2 @@ +Fix potential crash during GC caused by ``tp_dealloc`` which doesn't call +``PyObject_GC_UnTrack()``. diff --git a/Modules/_collectionsmodule.c b/Modules/_collectionsmodule.c index 1dd4b99810..2e4da4c211 100644 --- a/Modules/_collectionsmodule.c +++ b/Modules/_collectionsmodule.c @@ -1271,6 +1271,8 @@ dequeiter_traverse(dequeiterobject *dio, visitproc visit, void *arg) static void dequeiter_dealloc(dequeiterobject *dio) { + /* bpo-31095: UnTrack is needed before calling any callbacks */ + PyObject_GC_UnTrack(dio); Py_XDECREF(dio->deque); PyObject_GC_Del(dio); } @@ -1556,6 +1558,8 @@ static PyMemberDef defdict_members[] = { static void defdict_dealloc(defdictobject *dd) { + /* bpo-31095: UnTrack is needed before calling any callbacks */ + PyObject_GC_UnTrack(dd); Py_CLEAR(dd->default_factory); PyDict_Type.tp_dealloc((PyObject *)dd); } diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c index e0781a98d5..e9e89331b9 100644 --- a/Modules/_functoolsmodule.c +++ b/Modules/_functoolsmodule.c @@ -143,6 +143,7 @@ partial_new(PyTypeObject *type, PyObject *args, PyObject *kw) static void partial_dealloc(partialobject *pto) { + /* bpo-31095: UnTrack is needed before calling any callbacks */ PyObject_GC_UnTrack(pto); if (pto->weakreflist != NULL) PyObject_ClearWeakRefs((PyObject *) pto); diff --git a/Modules/_io/bytesio.c b/Modules/_io/bytesio.c index 1335ae8903..0ee4b80434 100644 --- a/Modules/_io/bytesio.c +++ b/Modules/_io/bytesio.c @@ -745,6 +745,7 @@ bytesio_setstate(bytesio *self, PyObject *state) static void bytesio_dealloc(bytesio *self) { + /* bpo-31095: UnTrack is needed before calling any callbacks */ _PyObject_GC_UNTRACK(self); if (self->buf != NULL) { PyMem_Free(self->buf); diff --git a/Modules/_json.c b/Modules/_json.c index 42c93aba1e..be1e079696 100644 --- a/Modules/_json.c +++ b/Modules/_json.c @@ -840,7 +840,8 @@ py_encode_basestring_ascii(PyObject* self UNUSED, PyObject *pystr) static void scanner_dealloc(PyObject *self) { - /* Deallocate scanner object */ + /* bpo-31095: UnTrack is needed before calling any callbacks */ + PyObject_GC_UnTrack(self); scanner_clear(self); Py_TYPE(self)->tp_free(self); } @@ -2298,7 +2299,8 @@ bail: static void encoder_dealloc(PyObject *self) { - /* Deallocate Encoder */ + /* bpo-31095: UnTrack is needed before calling any callbacks */ + PyObject_GC_UnTrack(self); encoder_clear(self); Py_TYPE(self)->tp_free(self); } diff --git a/Modules/_ssl.c b/Modules/_ssl.c index 45a1d01231..213c7d2151 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -2214,6 +2214,8 @@ context_clear(PySSLContext *self) static void context_dealloc(PySSLContext *self) { + /* bpo-31095: UnTrack is needed before calling any callbacks */ + PyObject_GC_UnTrack(self); context_clear(self); SSL_CTX_free(self->ctx); #ifdef OPENSSL_NPN_NEGOTIATED diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 9506067c7d..a792b2dfa2 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1076,6 +1076,7 @@ dict_dealloc(register PyDictObject *mp) { register PyDictEntry *ep; Py_ssize_t fill = mp->ma_fill; + /* bpo-31095: UnTrack is needed before calling any callbacks */ PyObject_GC_UnTrack(mp); Py_TRASHCAN_SAFE_BEGIN(mp) for (ep = mp->ma_table; fill > 0; ep++) { @@ -2576,6 +2577,8 @@ dictiter_new(PyDictObject *dict, PyTypeObject *itertype) static void dictiter_dealloc(dictiterobject *di) { + /* bpo-31095: UnTrack is needed before calling any callbacks */ + _PyObject_GC_UNTRACK(di); Py_XDECREF(di->di_dict); Py_XDECREF(di->di_result); PyObject_GC_Del(di); @@ -2855,6 +2858,8 @@ typedef struct { static void dictview_dealloc(dictviewobject *dv) { + /* bpo-31095: UnTrack is needed before calling any callbacks */ + _PyObject_GC_UNTRACK(dv); Py_XDECREF(dv->dv_dict); PyObject_GC_Del(dv); } diff --git a/Objects/setobject.c b/Objects/setobject.c index b3ca643c4f..0c69fac8e6 100644 --- a/Objects/setobject.c +++ b/Objects/setobject.c @@ -549,6 +549,7 @@ set_dealloc(PySetObject *so) { register setentry *entry; Py_ssize_t fill = so->fill; + /* bpo-31095: UnTrack is needed before calling any callbacks */ PyObject_GC_UnTrack(so); Py_TRASHCAN_SAFE_BEGIN(so) if (so->weakreflist != NULL) @@ -811,6 +812,8 @@ typedef struct { static void setiter_dealloc(setiterobject *si) { + /* bpo-31095: UnTrack is needed before calling any callbacks */ + _PyObject_GC_UNTRACK(si); Py_XDECREF(si->si_set); PyObject_GC_Del(si); }