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

(cherry picked from commit a6296d3)

Author: methane

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 */

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);
 }

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()``.

Modules/_collectionsmodule.c

@@ -1641,6 +1641,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);
 }
@@ -2021,6 +2023,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);
 }

Modules/_elementtree.c

@@ -653,6 +653,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)
 
@@ -2025,6 +2026,9 @@ static void
 elementiter_dealloc(ElementIterObject *it)
 {
     ParentLocator *p = it->parent_stack;
+    /* bpo-31095: UnTrack is needed before calling any callbacks */
+    PyObject_GC_UnTrack(it);
+
     while (p) {
         ParentLocator *temp = p;
         Py_XDECREF(p->parent);
@@ -2034,8 +2038,6 @@ elementiter_dealloc(ElementIterObject *it)
 
     Py_XDECREF(it->sought_tag);
     Py_XDECREF(it->root_element);
-
-    PyObject_GC_UnTrack(it);
     PyObject_GC_Del(it);
 }
 

Modules/_functoolsmodule.c

@@ -116,6 +116,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);
@@ -1039,7 +1040,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);

Modules/_io/bytesio.c

@@ -1133,6 +1133,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);
 }

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);
 }
@@ -1793,7 +1794,8 @@ 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);
 }

Modules/_ssl.c

@@ -2465,6 +2465,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

Modules/_struct.c

@@ -1581,6 +1581,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);