bpo-31095: fix potential crash during GC (GH-2974)

diff --git a/Doc/extending/newtypes.rst b/Doc/extending/newtypes.rst
index bd09aa695d36ff1ea8d6ce072f2af4c33fcc4270..f0e8985c61563dca156767ae3b91990d854d07da 100644 (file)
--- 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 eb9622a87d99cc97ef81dd1c4544c6c0f81385b4..08ba4c3d91a03059c1cb324633a74b950dde13d7 100644 (file)
--- 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 (file)
index 0000000..ca1f8ba
--- /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 aa582ed8dec43a5ad27010e472266777e6ea9ed5..adbe78970041975e03f6e7206f415ed40429a1d7 100644 (file)
--- a/Modules/_collectionsmodule.c
+++ b/Modules/_collectionsmodule.c
@@ -1717,6 +1717,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);
 }
@@ -2097,6 +2099,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 3537f19b1c7a9ce8a6665f5d604979ae2ceecfc6..857005a2a9b8ad84c223d1d0bc254b1c6584fe81 100644 (file)
--- 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,8 @@ 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);
     PyMem_Free(it->parent_stack);
@@ -2083,7 +2086,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 da1d2e16dba746ec63338d5a247a568a8893d3ed..33761a46667dd05b04421cdbd35a89eba8d333ad 100644 (file)
--- 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);
@@ -1073,7 +1074,11 @@ lru_cache_clear_list(lru_list_elem *link)
 static void
 lru_cache_dealloc(lru_cache_object *obj)
 {
-    lru_list_elem *list = lru_cache_unlink_list(obj);
+    lru_list_elem *list;
+    /* bpo-31095: UnTrack is needed before calling any callbacks */
+    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/Modules/_io/bytesio.c b/Modules/_io/bytesio.c
index 42c7f7933e00f587f46734ec431abc6c1897d4a5..ba33f8c50d6aadee2ffd3e2b5a74495003d3f763 100644 (file)
--- a/Modules/_io/bytesio.c
+++ b/Modules/_io/bytesio.c
@@ -1084,6 +1084,8 @@ 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 6cc31c6e3712245b51e2e36c11c6e75187d2395c..54fc90cfece82e5ccee0b854bff5bc40491c224c 100644 (file)
--- a/Modules/_json.c
+++ b/Modules/_json.c
@@ -655,7 +655,8 @@ 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);
 }
@@ -1778,7 +1779,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 1380c575119bf41dde74491abd17446f17aea599..e8abcb286f28f8466a54fc828cb9119669a4a5c5 100644 (file)
--- a/Modules/_ssl.c
+++ b/Modules/_ssl.c
@@ -2778,6 +2778,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
@@ -4292,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 b5fbc43564c0f6fe4737532cf364cd5bebdaa6c5..69a1e996d05d3ded56e6d8a8e0050ca0ed9be18a 100644 (file)
--- a/Modules/_struct.c
+++ b/Modules/_struct.c
@@ -1589,6 +1589,8 @@ 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);
     PyObject_GC_Del(self);

diff --git a/Objects/dictobject.c b/Objects/dictobject.c
index e395f4dabb3e6c87c674e415e62c59b616c9c271..f055170750ded01a56430524eb8a0524edab97b9 100644 (file)
--- a/Objects/dictobject.c
+++ b/Objects/dictobject.c
@@ -1847,6 +1847,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) {
@@ -3270,6 +3272,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);
@@ -3629,6 +3633,8 @@ 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 2347b9d70adb2b7b54a447a1d85aaaf1d522d6b8..5c61bc71795fa1e457b2f10e37a7d830764b9216 100644 (file)
--- 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,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);
 }

diff --git a/Parser/asdl_c.py b/Parser/asdl_c.py
index b367e10231c040647685d8ad234b0d715f6e7257..f6c3a66117250013f20d2e164c2abbd10dd680a6 100644 (file)
--- a/Parser/asdl_c.py
+++ b/Parser/asdl_c.py
@@ -633,6 +633,8 @@ 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);
 }

diff --git a/Python/Python-ast.c b/Python/Python-ast.c
index 77852f923f4af563a01b11b8bfe945ada190ef70..2f1e737ea656f6eb35c097345552bd07ef686383 100644 (file)
--- a/Python/Python-ast.c
+++ b/Python/Python-ast.c
@@ -520,6 +520,8 @@ 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);
 }