From cbce046fd5f6e710f3bb86e665cdeee69a3bbdaf Mon Sep 17 00:00:00 2001 From: INADA Naoki Date: Tue, 1 Aug 2017 18:48:46 +0900 Subject: [PATCH 1/4] bpo-31095: fix potential crash during GC. Some tp_dealloc methods of GC types didn't call PyObject_GC_UnTrack(). --- .../Core and Builtins/2017-08-01-18-48-30.bpo-31095.bXWZDb.rst | 2 ++ Modules/_collectionsmodule.c | 2 ++ Modules/_elementtree.c | 2 +- Modules/_functoolsmodule.c | 1 + Modules/_io/bytesio.c | 1 + Modules/_json.c | 2 ++ Modules/_ssl.c | 1 + Modules/_struct.c | 1 + Objects/dictobject.c | 2 ++ Objects/setobject.c | 1 + Parser/asdl_c.py | 1 + Python/Python-ast.c | 1 + 12 files changed, 16 insertions(+), 1 deletion(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2017-08-01-18-48-30.bpo-31095.bXWZDb.rst 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 aa582ed8dec43a..c09d12ce265986 100644 --- a/Modules/_collectionsmodule.c +++ b/Modules/_collectionsmodule.c @@ -1717,6 +1717,7 @@ dequeiter_traverse(dequeiterobject *dio, visitproc visit, void *arg) static void dequeiter_dealloc(dequeiterobject *dio) { + PyObject_GC_UnTrack(dio); Py_XDECREF(dio->deque); PyObject_GC_Del(dio); } @@ -2097,6 +2098,7 @@ static PyMemberDef defdict_members[] = { static void defdict_dealloc(defdictobject *dd) { + 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 3537f19b1c7a9c..fb250aaaa2f20d 100644 --- a/Modules/_elementtree.c +++ b/Modules/_elementtree.c @@ -2076,6 +2076,7 @@ elementiter_dealloc(ElementIterObject *it) { Py_ssize_t i = it->parent_stack_used; it->parent_stack_used = 0; + PyObject_GC_UnTrack(it); while (i--) Py_XDECREF(it->parent_stack[i].parent); PyMem_Free(it->parent_stack); @@ -2083,7 +2084,6 @@ elementiter_dealloc(ElementIterObject *it) Py_XDECREF(it->sought_tag); Py_XDECREF(it->root_element); - PyObject_GC_UnTrack(it); PyObject_GC_Del(it); } diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c index da1d2e16dba746..5ee877fc6a0160 100644 --- a/Modules/_functoolsmodule.c +++ b/Modules/_functoolsmodule.c @@ -1073,6 +1073,7 @@ lru_cache_clear_list(lru_list_elem *link) static void lru_cache_dealloc(lru_cache_object *obj) { + _PyObject_GC_UNTRACK(obj); lru_list_elem *list = lru_cache_unlink_list(obj); Py_XDECREF(obj->maxsize_O); Py_XDECREF(obj->func); diff --git a/Modules/_io/bytesio.c b/Modules/_io/bytesio.c index 42c7f7933e00f5..62a4fb3ecba0f4 100644 --- a/Modules/_io/bytesio.c +++ b/Modules/_io/bytesio.c @@ -1084,6 +1084,7 @@ bytesiobuf_traverse(bytesiobuf *self, visitproc visit, void *arg) static void bytesiobuf_dealloc(bytesiobuf *self) { + PyObject_GC_UnTrack(self); Py_CLEAR(self->source); Py_TYPE(self)->tp_free(self); } diff --git a/Modules/_json.c b/Modules/_json.c index 6cc31c6e371224..58595654999388 100644 --- a/Modules/_json.c +++ b/Modules/_json.c @@ -656,6 +656,7 @@ static void scanner_dealloc(PyObject *self) { /* Deallocate scanner object */ + PyObject_GC_UnTrack(self); scanner_clear(self); Py_TYPE(self)->tp_free(self); } @@ -1779,6 +1780,7 @@ static void encoder_dealloc(PyObject *self) { /* Deallocate Encoder */ + PyObject_GC_UnTrack(self); encoder_clear(self); Py_TYPE(self)->tp_free(self); } diff --git a/Modules/_ssl.c b/Modules/_ssl.c index 1380c575119bf4..f6eb400ac3b51c 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -2778,6 +2778,7 @@ context_clear(PySSLContext *self) static void context_dealloc(PySSLContext *self) { + PyObject_GC_UnTrack(self); context_clear(self); SSL_CTX_free(self->ctx); #ifdef OPENSSL_NPN_NEGOTIATED diff --git a/Modules/_struct.c b/Modules/_struct.c index b5fbc43564c0f6..b8c0101436a4e4 100644 --- a/Modules/_struct.c +++ b/Modules/_struct.c @@ -1589,6 +1589,7 @@ typedef struct { static void unpackiter_dealloc(unpackiterobject *self) { + PyObject_GC_UnTrack(self); Py_XDECREF(self->so); PyBuffer_Release(&self->buf); PyObject_GC_Del(self); diff --git a/Objects/dictobject.c b/Objects/dictobject.c index c44ff47f8b33cd..9a33743c691209 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -3407,6 +3407,7 @@ dictiter_new(PyDictObject *dict, PyTypeObject *itertype) static void dictiter_dealloc(dictiterobject *di) { + _PyObject_GC_UNTRACK(di); Py_XDECREF(di->di_dict); Py_XDECREF(di->di_result); PyObject_GC_Del(di); @@ -3766,6 +3767,7 @@ dictiter_reduce(dictiterobject *di) static void dictview_dealloc(_PyDictViewObject *dv) { + _PyObject_GC_UNTRACK(dv); Py_XDECREF(dv->dv_dict); PyObject_GC_Del(dv); } diff --git a/Objects/setobject.c b/Objects/setobject.c index 2347b9d70adb2b..2cfdfa56b91bc3 100644 --- a/Objects/setobject.c +++ b/Objects/setobject.c @@ -809,6 +809,7 @@ typedef struct { static void setiter_dealloc(setiterobject *si) { + _PyObject_GC_UNTRACK(si); Py_XDECREF(si->si_set); PyObject_GC_Del(si); } diff --git a/Parser/asdl_c.py b/Parser/asdl_c.py index 096f5f8c7494f7..7b228b60343fdc 100644 --- a/Parser/asdl_c.py +++ b/Parser/asdl_c.py @@ -633,6 +633,7 @@ def visitModule(self, mod): static void ast_dealloc(AST_object *self) { + _PyObject_GC_UNTRACK(self); Py_CLEAR(self->dict); Py_TYPE(self)->tp_free(self); } diff --git a/Python/Python-ast.c b/Python/Python-ast.c index 2759b2fe9c4b33..3148ea17e7a9ac 100644 --- a/Python/Python-ast.c +++ b/Python/Python-ast.c @@ -520,6 +520,7 @@ typedef struct { static void ast_dealloc(AST_object *self) { + _PyObject_GC_UNTRACK(self); Py_CLEAR(self->dict); Py_TYPE(self)->tp_free(self); } From 0d576ca1ff950726796e08eeee8fd16da15ff1a2 Mon Sep 17 00:00:00 2001 From: INADA Naoki Date: Fri, 4 Aug 2017 02:08:22 +0900 Subject: [PATCH 2/4] Replace _PyObject_GC_UNTRACK with PyObject_GC_UnTrack --- Modules/_functoolsmodule.c | 6 ++++-- Parser/asdl_c.py | 2 +- Python/Python-ast.c | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c index 5ee877fc6a0160..a283bb34250ad9 100644 --- a/Modules/_functoolsmodule.c +++ b/Modules/_functoolsmodule.c @@ -1073,8 +1073,10 @@ lru_cache_clear_list(lru_list_elem *link) static void lru_cache_dealloc(lru_cache_object *obj) { - _PyObject_GC_UNTRACK(obj); - lru_list_elem *list = lru_cache_unlink_list(obj); + lru_list_elem *list; + PyObject_GC_UnTrack(obj); + + list = lru_cache_unlink_list(obj); Py_XDECREF(obj->maxsize_O); Py_XDECREF(obj->func); Py_XDECREF(obj->cache); diff --git a/Parser/asdl_c.py b/Parser/asdl_c.py index 7b228b60343fdc..695f576f9ad6f9 100644 --- a/Parser/asdl_c.py +++ b/Parser/asdl_c.py @@ -633,7 +633,7 @@ def visitModule(self, mod): static void ast_dealloc(AST_object *self) { - _PyObject_GC_UNTRACK(self); + PyObject_GC_UnTrack(self); Py_CLEAR(self->dict); Py_TYPE(self)->tp_free(self); } diff --git a/Python/Python-ast.c b/Python/Python-ast.c index 3148ea17e7a9ac..7999e4f67872dd 100644 --- a/Python/Python-ast.c +++ b/Python/Python-ast.c @@ -520,7 +520,7 @@ typedef struct { static void ast_dealloc(AST_object *self) { - _PyObject_GC_UNTRACK(self); + PyObject_GC_UnTrack(self); Py_CLEAR(self->dict); Py_TYPE(self)->tp_free(self); } From 4e97f02130839052dba633382912214e73084653 Mon Sep 17 00:00:00 2001 From: INADA Naoki Date: Fri, 4 Aug 2017 02:27:53 +0900 Subject: [PATCH 3/4] bpo-31095: Document PyObject_GC_UnTrack requirement --- Doc/extending/newtypes.rst | 29 ++++++++++++++++++++--------- Doc/includes/noddy4.c | 1 + 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/Doc/extending/newtypes.rst b/Doc/extending/newtypes.rst index bd09aa695d36ff..f0e8985c61563d 100644 --- a/Doc/extending/newtypes.rst +++ b/Doc/extending/newtypes.rst @@ -728,8 +728,9 @@ functions. With :c:func:`Py_VISIT`, :c:func:`Noddy_traverse` can be 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) @@ -747,13 +748,6 @@ to use it:: return 0; } - static void - Noddy_dealloc(Noddy* self) - { - Noddy_clear(self); - Py_TYPE(self)->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 @@ -776,6 +770,23 @@ be simplified:: 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 eb9622a87d99cc..08ba4c3d91a030 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); } From 1b2e3544c827b5da02f8aa96721a38b6d0367fd5 Mon Sep 17 00:00:00 2001 From: INADA Naoki Date: Wed, 23 Aug 2017 17:52:29 +0900 Subject: [PATCH 4/4] Add note comments --- Modules/_collectionsmodule.c | 2 ++ Modules/_elementtree.c | 2 ++ Modules/_functoolsmodule.c | 2 ++ Modules/_io/bytesio.c | 1 + Modules/_json.c | 4 ++-- Modules/_ssl.c | 2 ++ Modules/_struct.c | 1 + Objects/dictobject.c | 4 ++++ Objects/setobject.c | 2 ++ Parser/asdl_c.py | 1 + Python/Python-ast.c | 1 + 11 files changed, 20 insertions(+), 2 deletions(-) diff --git a/Modules/_collectionsmodule.c b/Modules/_collectionsmodule.c index c09d12ce265986..adbe7897004197 100644 --- a/Modules/_collectionsmodule.c +++ b/Modules/_collectionsmodule.c @@ -1717,6 +1717,7 @@ 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); @@ -2098,6 +2099,7 @@ 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 fb250aaaa2f20d..857005a2a9b8ad 100644 --- a/Modules/_elementtree.c +++ b/Modules/_elementtree.c @@ -627,6 +627,7 @@ element_gc_clear(ElementObject *self) static void element_dealloc(ElementObject* self) { + /* bpo-31095: UnTrack is needed before calling any callbacks */ PyObject_GC_UnTrack(self); Py_TRASHCAN_SAFE_BEGIN(self) @@ -2076,6 +2077,7 @@ elementiter_dealloc(ElementIterObject *it) { Py_ssize_t i = it->parent_stack_used; it->parent_stack_used = 0; + /* bpo-31095: UnTrack is needed before calling any callbacks */ PyObject_GC_UnTrack(it); while (i--) Py_XDECREF(it->parent_stack[i].parent); diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c index a283bb34250ad9..33761a46667dd0 100644 --- a/Modules/_functoolsmodule.c +++ b/Modules/_functoolsmodule.c @@ -113,6 +113,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); @@ -1074,6 +1075,7 @@ static void lru_cache_dealloc(lru_cache_object *obj) { lru_list_elem *list; + /* bpo-31095: UnTrack is needed before calling any callbacks */ PyObject_GC_UnTrack(obj); list = lru_cache_unlink_list(obj); diff --git a/Modules/_io/bytesio.c b/Modules/_io/bytesio.c index 62a4fb3ecba0f4..ba33f8c50d6aad 100644 --- a/Modules/_io/bytesio.c +++ b/Modules/_io/bytesio.c @@ -1084,6 +1084,7 @@ bytesiobuf_traverse(bytesiobuf *self, visitproc visit, void *arg) static void bytesiobuf_dealloc(bytesiobuf *self) { + /* bpo-31095: UnTrack is needed before calling any callbacks */ PyObject_GC_UnTrack(self); Py_CLEAR(self->source); Py_TYPE(self)->tp_free(self); diff --git a/Modules/_json.c b/Modules/_json.c index 58595654999388..54fc90cfece82e 100644 --- a/Modules/_json.c +++ b/Modules/_json.c @@ -655,7 +655,7 @@ py_encode_basestring(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); @@ -1779,7 +1779,7 @@ encoder_listencode_list(PyEncoderObject *s, _PyAccu *acc, 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 f6eb400ac3b51c..e8abcb286f28f8 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -2778,6 +2778,7 @@ 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); @@ -4293,6 +4294,7 @@ static PyTypeObject PySSLMemoryBIO_Type = { static void PySSLSession_dealloc(PySSLSession *self) { + /* bpo-31095: UnTrack is needed before calling any callbacks */ PyObject_GC_UnTrack(self); Py_XDECREF(self->ctx); if (self->session != NULL) { diff --git a/Modules/_struct.c b/Modules/_struct.c index b8c0101436a4e4..69a1e996d05d3d 100644 --- a/Modules/_struct.c +++ b/Modules/_struct.c @@ -1589,6 +1589,7 @@ typedef struct { static void unpackiter_dealloc(unpackiterobject *self) { + /* bpo-31095: UnTrack is needed before calling any callbacks */ PyObject_GC_UnTrack(self); Py_XDECREF(self->so); PyBuffer_Release(&self->buf); diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 9a33743c691209..86543206efd523 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1982,6 +1982,8 @@ dict_dealloc(PyDictObject *mp) PyObject **values = mp->ma_values; PyDictKeysObject *keys = mp->ma_keys; Py_ssize_t i, n; + + /* bpo-31095: UnTrack is needed before calling any callbacks */ PyObject_GC_UnTrack(mp); Py_TRASHCAN_SAFE_BEGIN(mp) if (values != NULL) { @@ -3407,6 +3409,7 @@ 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); @@ -3767,6 +3770,7 @@ dictiter_reduce(dictiterobject *di) static void dictview_dealloc(_PyDictViewObject *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 2cfdfa56b91bc3..5c61bc71795fa1 100644 --- a/Objects/setobject.c +++ b/Objects/setobject.c @@ -553,6 +553,7 @@ set_dealloc(PySetObject *so) setentry *entry; Py_ssize_t used = so->used; + /* bpo-31095: UnTrack is needed before calling any callbacks */ PyObject_GC_UnTrack(so); Py_TRASHCAN_SAFE_BEGIN(so) if (so->weakreflist != NULL) @@ -809,6 +810,7 @@ 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); diff --git a/Parser/asdl_c.py b/Parser/asdl_c.py index 695f576f9ad6f9..1cc74628e74e0d 100644 --- a/Parser/asdl_c.py +++ b/Parser/asdl_c.py @@ -633,6 +633,7 @@ def visitModule(self, mod): static void ast_dealloc(AST_object *self) { + /* bpo-31095: UnTrack is needed before calling any callbacks */ PyObject_GC_UnTrack(self); Py_CLEAR(self->dict); Py_TYPE(self)->tp_free(self); diff --git a/Python/Python-ast.c b/Python/Python-ast.c index 7999e4f67872dd..fe9e4d985c75f8 100644 --- a/Python/Python-ast.c +++ b/Python/Python-ast.c @@ -520,6 +520,7 @@ typedef struct { static void ast_dealloc(AST_object *self) { + /* bpo-31095: UnTrack is needed before calling any callbacks */ PyObject_GC_UnTrack(self); Py_CLEAR(self->dict); Py_TYPE(self)->tp_free(self);