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 5959e4f2b1ee6a6699d2a17315e3364b339b098d..7eadcae5e61d1b9c95f70ac5d43c8d4881769a46 100644 (file)
--- 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 9feb71aae3be5b0e9a62a45d7ce63067ac936f2e..0fd4dc378844cc23e089a480b45645300732f69a 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 1dd4b998100572b40d6ae22bfc73708d4a10c694..2e4da4c211f88f4c5b2e9890df4f4d1a97420450 100644 (file)
--- 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 e0781a98d5e5b0ffe5aad2fd3ee27dc18689639e..e9e89331b993e5c73faebeac7a79d985d642ab6f 100644 (file)
--- 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 1335ae89032260ba7f807ba39174f418639b23bc..0ee4b80434c8043b0e5740f8d3a6ed8939ec2e61 100644 (file)
--- 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 42c93aba1e22a28a6e17b46e29ad44b75991f13a..be1e079696051c33a1a7eab6c691a697044ca6dd 100644 (file)
--- 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 45a1d0123109eba71fe2638bdcffa29402c4cb1c..213c7d21510f62b93619a950eae282301f51d805 100644 (file)
--- 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 9506067c7db466d160d24b6c1355070804adf9d1..a792b2dfa21074b490fe46f4ad4b74393f06d7d4 100644 (file)
--- 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 b3ca643c4f6caaefd177ad877f0726936c97cdc4..0c69fac8e6d41855f879af35a92a01833ee7ce77 100644 (file)
--- 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);
 }