Fix crash during deallocation of self-referencing lists

When a list or any of its nodes references itself in a way that creates a cycle,
their PyObjects may be visited multiple times during deallocation of the list.
To handle this case cleanly, Python documentation recommends to release
references held by such PyObject with Py_CLEAR.
The macro works similarly to Py_XDECREF, but also sets the reference to NULL,
which prevents repeated deallocation of the reference during subsequent visits
of its owner PyObject.

Most of the references held by llist PyObjects are released by Py_CLEAR.
One of the exceptions was Py_None, which is a global variable that may not be
set to NULL.
To work around this limitation, llist module simply invoked Py_DECREF on Py_None
during each visit of a garbage collected PyObject. Unfortunately this could
release Py_None too many times, effectively 'stealing' references from other
objects and eventually leading to a crash.

To fix this issue, each linked list and node which holds a Py_None reference
will now set a flag in its PyObject structure. The first release of the Py_None
reference will clear the flag to prevent future traversal of this object from
repeating the operation.

Closes issue #11.

CHANGES

@@ -2,6 +2,8 @@
 
   - fixed repr() and str() for self-referencing lists and nodes
     (closes issue #10)
+  - fixed duplicated release of Py_None during destruction of
+    self-referencing lists and nodes (closes issue #11)
 
 -----------------------------------------------------------------------
 

src/dllist.c

@@ -4,7 +4,9 @@
 
 #include <Python.h>
 #include <structmember.h>
+
 #include "config.h"
+#include "flags.h"
 #include "py23macros.h"
 #include "utils.h"
 
@@ -28,6 +30,7 @@ typedef struct
     PyObject* prev;
     PyObject* next;
     PyObject* list_weakref;
+    unsigned char flags;
 } DLListNodeObject;
 
 /* Convenience function for linking list nodes.
@@ -187,7 +190,12 @@ static int dllistnode_clear_refs(DLListNodeObject* self)
 {
     Py_CLEAR(self->value);
     Py_CLEAR(self->list_weakref);
-    Py_DECREF(Py_None);
+
+    if ((self->flags & LLIST_HAS_PY_NONE_REF) != 0)
+    {
+        self->flags &= ~LLIST_HAS_PY_NONE_REF;
+        Py_DECREF(Py_None);
+    }
 
     return 0;
 }
@@ -219,6 +227,7 @@ static PyObject* dllistnode_new(PyTypeObject* type,
     self->prev = Py_None;
     self->next = Py_None;
     self->list_weakref = Py_None;
+    self->flags = LLIST_HAS_PY_NONE_REF;
 
     Py_INCREF(self->value);
     Py_INCREF(self->list_weakref);
@@ -333,6 +342,7 @@ typedef struct
     Py_ssize_t last_accessed_idx;
     Py_ssize_t size;
     PyObject* weakref_list;
+    unsigned char flags;
 } DLListObject;
 
 static Py_ssize_t py_ssize_t_abs(Py_ssize_t x)
@@ -578,7 +588,11 @@ static int dllist_clear_refs(DLListObject* self)
         }
     }
 
-    Py_DECREF(Py_None);
+    if ((self->flags & LLIST_HAS_PY_NONE_REF) != 0)
+    {
+        self->flags &= ~LLIST_HAS_PY_NONE_REF;
+        Py_DECREF(Py_None);
+    }
 
     return 0;
 }
@@ -612,6 +626,7 @@ static PyObject* dllist_new(PyTypeObject* type,
     self->last_accessed_idx = -1;
     self->size = 0;
     self->weakref_list = NULL;
+    self->flags = LLIST_HAS_PY_NONE_REF;
 
     return (PyObject*)self;
 }

src/flags.h

@@ -0,0 +1,10 @@
+/* Copyright (c) 2019 Adam Jakubek, Rafał Gałczyński
+ * Released under the MIT license (see attached LICENSE file).
+ */
+
+#ifndef FLAGS_H
+#define FLAGS_H
+
+#define LLIST_HAS_PY_NONE_REF (0x01)
+
+#endif /* FLAGS_H */

src/sllist.c

@@ -4,7 +4,9 @@
 
 #include <Python.h>
 #include <structmember.h>
+
 #include "config.h"
+#include "flags.h"
 #include "py23macros.h"
 #include "utils.h"
 
@@ -27,6 +29,7 @@ typedef struct
     PyObject* value;
     PyObject* next;
     PyObject* list_weakref;
+    unsigned char flags;
 } SLListNodeObject;
 
 
@@ -115,7 +118,12 @@ static int sllistnode_clear_refs(SLListNodeObject* self)
 {
     Py_CLEAR(self->value);
     Py_CLEAR(self->list_weakref);
-    Py_DECREF(Py_None);
+
+    if ((self->flags & LLIST_HAS_PY_NONE_REF) != 0)
+    {
+        self->flags &= ~LLIST_HAS_PY_NONE_REF;
+        Py_DECREF(Py_None);
+    }
 
     return 0;
 }
@@ -168,6 +176,7 @@ static PyObject* sllistnode_new(PyTypeObject* type,
     self->next = Py_None;
     self->value = Py_None;
     self->list_weakref = Py_None;
+    self->flags = LLIST_HAS_PY_NONE_REF;
 
     Py_INCREF(self->value);
     Py_INCREF(self->list_weakref);
@@ -309,6 +318,7 @@ typedef struct
     PyObject* last;
     Py_ssize_t size;
     PyObject* weakref_list;
+    unsigned char flags;
 } SLListObject;
 
 
@@ -351,7 +361,11 @@ static int sllist_clear_refs(SLListObject* self)
         }
     }
 
-    Py_DECREF(Py_None);
+    if ((self->flags & LLIST_HAS_PY_NONE_REF) != 0)
+    {
+      self->flags &= ~LLIST_HAS_PY_NONE_REF;
+      Py_DECREF(Py_None);
+    }
 
     return 0;
 }
@@ -383,6 +397,7 @@ static PyObject* sllist_new(PyTypeObject* type,
     self->last = Py_None;
     self->weakref_list = NULL;
     self->size = 0;
+    self->flags = LLIST_HAS_PY_NONE_REF;
 
     return (PyObject*)self;
 }

tests/llist_test.py

@@ -1012,6 +1012,26 @@ def test_list_node_can_be_subclassed(self):
         class DerivedNode(sllistnode):
             pass
 
+    def test_cyclic_list_destruction_does_not_release_extra_None_refs(self):
+        original_ref_count = sys.getrefcount(None)
+
+        for _ in range(original_ref_count * 10):
+            ll = sllist()
+            ll.append(sllistnode(ll))
+            del ll
+
+        self.assertGreater(sys.getrefcount(None), 0)
+
+    def test_cyclic_node_destruction_does_not_release_extra_None_refs(self):
+        original_ref_count = sys.getrefcount(None)
+
+        for _ in range(original_ref_count * 10):
+            ll = self.make_recursive_node_list()
+            del ll
+
+        self.assertGreater(sys.getrefcount(None), 0)
+
+
 class testdllist(unittest.TestCase):
 
     def test_init_empty(self):
@@ -1911,6 +1931,25 @@ def test_list_node_can_be_subclassed(self):
         class DerivedNode(dllistnode):
             pass
 
+    def test_cyclic_list_destruction_does_not_release_extra_None_refs(self):
+        original_ref_count = sys.getrefcount(None)
+
+        for _ in range(original_ref_count * 10):
+            ll = dllist()
+            ll.append(dllistnode(ll))
+            del ll
+
+        self.assertGreater(sys.getrefcount(None), 0)
+
+    def test_cyclic_node_destruction_does_not_release_extra_None_refs(self):
+        original_ref_count = sys.getrefcount(None)
+
+        for _ in range(original_ref_count * 10):
+            ll = self.make_recursive_node_list()
+            del ll
+
+        self.assertGreater(sys.getrefcount(None), 0)
+
 
 def suite():
     suite = unittest.TestSuite()