From 00fea7c4df492b079c6d16484732263561322697 Mon Sep 17 00:00:00 2001 From: INADA Naoki Date: Thu, 24 Aug 2017 14:55:17 +0900 Subject: [PATCH 1/2] bpo-31095: fix potential crash during GC (GH-2974) (cherry picked from commit a6296d34a478b4f697ea9db798146195075d496c) --- Doc/extending/newtypes.rst | 29 +++++++++++++------ Doc/includes/noddy4.c | 1 + .../2017-08-01-18-48-30.bpo-31095.bXWZDb.rst | 2 ++ Modules/_collectionsmodule.c | 4 +++ Modules/_elementtree.c | 3 ++ Modules/_functoolsmodule.c | 1 + Modules/_io/bytesio.c | 2 ++ Modules/_json.c | 6 ++-- Modules/_ssl.c | 3 +- Objects/dictobject.c | 5 ++++ Objects/setobject.c | 3 ++ 11 files changed, 47 insertions(+), 12 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2017-08-01-18-48-30.bpo-31095.bXWZDb.rst diff --git a/Doc/extending/newtypes.rst b/Doc/extending/newtypes.rst index 5959e4f2b1ee6a..7eadcae5e61d1b 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 9feb71aae3be5b..0fd4dc378844cc 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 00000000000000..ca1f8bafba69b3 --- /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 1dd4b998100572..2e4da4c211f88f 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/_elementtree.c b/Modules/_elementtree.c index 929616f3e2e5b7..9ebaa25da1b6d4 100644 --- a/Modules/_elementtree.c +++ b/Modules/_elementtree.c @@ -544,6 +544,9 @@ subelement(PyObject* self, PyObject* args, PyObject* kw) static void element_dealloc(ElementObject* self) { + /* bpo-31095: UnTrack is needed before calling any callbacks */ + PyObject_GC_UnTrack(self); + if (self->extra) element_dealloc_extra(self); diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c index e0781a98d5e5b0..e9e89331b993e5 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 1335ae89032260..b869413525cbec 100644 --- a/Modules/_io/bytesio.c +++ b/Modules/_io/bytesio.c @@ -745,6 +745,8 @@ 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); _PyObject_GC_UNTRACK(self); if (self->buf != NULL) { PyMem_Free(self->buf); diff --git a/Modules/_json.c b/Modules/_json.c index 42c93aba1e22a2..be1e079696051c 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 @@ encoder_listencode_list(PyEncoderObject *s, PyObject *rval, PyObject *seq, Py_ss 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 45a1d0123109eb..62f1ecb09a90f4 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 @@ -3448,7 +3450,6 @@ static PyTypeObject PySSLContext_Type = { }; - #ifdef HAVE_OPENSSL_RAND /* helper routines for seeding the SSL PRNG */ diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 9506067c7db466..a792b2dfa21074 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 b3ca643c4f6caa..0c69fac8e6d418 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); } From caf722ff9daf0a62cf52d8d14dbe8eef2372370b Mon Sep 17 00:00:00 2001 From: INADA Naoki Date: Thu, 24 Aug 2017 15:53:19 +0900 Subject: [PATCH 2/2] remove merge garbage --- Modules/_elementtree.c | 3 --- Modules/_io/bytesio.c | 1 - Modules/_ssl.c | 1 + 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/Modules/_elementtree.c b/Modules/_elementtree.c index 9ebaa25da1b6d4..929616f3e2e5b7 100644 --- a/Modules/_elementtree.c +++ b/Modules/_elementtree.c @@ -544,9 +544,6 @@ subelement(PyObject* self, PyObject* args, PyObject* kw) static void element_dealloc(ElementObject* self) { - /* bpo-31095: UnTrack is needed before calling any callbacks */ - PyObject_GC_UnTrack(self); - if (self->extra) element_dealloc_extra(self); diff --git a/Modules/_io/bytesio.c b/Modules/_io/bytesio.c index b869413525cbec..0ee4b80434c804 100644 --- a/Modules/_io/bytesio.c +++ b/Modules/_io/bytesio.c @@ -746,7 +746,6 @@ static void bytesio_dealloc(bytesio *self) { /* bpo-31095: UnTrack is needed before calling any callbacks */ - PyObject_GC_UnTrack(self); _PyObject_GC_UNTRACK(self); if (self->buf != NULL) { PyMem_Free(self->buf); diff --git a/Modules/_ssl.c b/Modules/_ssl.c index 62f1ecb09a90f4..213c7d21510f62 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -3450,6 +3450,7 @@ static PyTypeObject PySSLContext_Type = { }; + #ifdef HAVE_OPENSSL_RAND /* helper routines for seeding the SSL PRNG */