From 421d5ec0be06e5f53607a1f3dd1f46367e67cdef Mon Sep 17 00:00:00 2001
From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Thu, 24 Jun 2021 09:12:31 -0400
Subject: [PATCH] Use Py_TPFLAGS_HAVE_GC for Row

Fixed an issue in the C extension for the :class:`_result.Row` class which
could lead to a memory leak in the unlikely case of a :class:`_result.Row`
object which referred to an ORM object that then was mutated to refer back
to the ``Row`` itself, creating a cycle. The Python C APIs for tracking GC
cycles has been added to the native :class:`_result.Row` implementation to
accommodate for this case.

Fixes: #5348
Change-Id: I3ac32012f29fbb59f8921cf2a124fa3a7ac5f0d1
---
 doc/build/changelog/unreleased_14/5348.rst | 11 +++++++++
 lib/sqlalchemy/cextension/resultproxy.c    | 28 ++++++++++++++++------
 test/aaa_profiling/test_memusage.py        | 19 +++++++++++++++
 3 files changed, 51 insertions(+), 7 deletions(-)
 create mode 100644 doc/build/changelog/unreleased_14/5348.rst

diff --git a/doc/build/changelog/unreleased_14/5348.rst b/doc/build/changelog/unreleased_14/5348.rst
new file mode 100644
index 0000000000..337620aa1c
--- /dev/null
+++ b/doc/build/changelog/unreleased_14/5348.rst
@@ -0,0 +1,11 @@
+.. change::
+    :tags: bug, engine
+    :tickets: 5348
+
+    Fixed an issue in the C extension for the :class:`_result.Row` class which
+    could lead to a memory leak in the unlikely case of a :class:`_result.Row`
+    object which referred to an ORM object that then was mutated to refer back
+    to the ``Row`` itself, creating a cycle. The Python C APIs for tracking GC
+    cycles has been added to the native :class:`_result.Row` implementation to
+    accommodate for this case.
+
diff --git a/lib/sqlalchemy/cextension/resultproxy.c b/lib/sqlalchemy/cextension/resultproxy.c
index 596b264d03..d869a058b7 100644
--- a/lib/sqlalchemy/cextension/resultproxy.c
+++ b/lib/sqlalchemy/cextension/resultproxy.c
@@ -178,6 +178,22 @@ BaseRow_init(BaseRow *self, PyObject *args, PyObject *kwds)
     Py_INCREF(keymap);
     self->keymap = keymap;
     self->key_style = PyLong_AsLong(key_style);
+
+    // observation: because we have not implemented our own new method,
+    // cPython is apparently already calling PyObject_GC_Track for us.
+    // We assume it also called PyObject_GC_New since prior to #5348 we
+    // were already relying upon it to call PyObject_New, and we have now
+    // set Py_TPFLAGS_HAVE_GC.
+
+    return 0;
+}
+
+static int
+BaseRow_traverse(BaseRow *self, visitproc visit, void *arg)
+{
+    Py_VISIT(self->parent);
+    Py_VISIT(self->row);
+    Py_VISIT(self->keymap);
     return 0;
 }
 
@@ -258,14 +274,12 @@ BaseRow_filter_on_values(BaseRow *self, PyObject *filters)
 static void
 BaseRow_dealloc(BaseRow *self)
 {
+    PyObject_GC_UnTrack(self);
     Py_XDECREF(self->parent);
     Py_XDECREF(self->row);
     Py_XDECREF(self->keymap);
-#if PY_MAJOR_VERSION >= 3
-    Py_TYPE(self)->tp_free((PyObject *)self);
-#else
-    self->ob_type->tp_free((PyObject *)self);
-#endif
+    PyObject_GC_Del(self);
+
 }
 
 static PyObject *
@@ -760,9 +774,9 @@ static PyTypeObject BaseRowType = {
     (getattrofunc)BaseRow_getattro,/* tp_getattro */
     0,                                  /* tp_setattro */
     0,                                  /* tp_as_buffer */
-    Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE,               /* tp_flags */
+    Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE | Py_TPFLAGS_HAVE_GC, /* tp_flags */
     "BaseRow is a abstract base class for Row",   /* tp_doc */
-    0,                                  /* tp_traverse */
+    (traverseproc)BaseRow_traverse,           /* tp_traverse */
     0,                                  /* tp_clear */
     0,                                  /* tp_richcompare */
     0,                                  /* tp_weaklistoffset */
diff --git a/test/aaa_profiling/test_memusage.py b/test/aaa_profiling/test_memusage.py
index 28865d0109..2fedcdfdc5 100644
--- a/test/aaa_profiling/test_memusage.py
+++ b/test/aaa_profiling/test_memusage.py
@@ -14,6 +14,7 @@ from sqlalchemy import String
 from sqlalchemy import testing
 from sqlalchemy import Unicode
 from sqlalchemy import util
+from sqlalchemy.engine import result
 from sqlalchemy.orm import aliased
 from sqlalchemy.orm import clear_mappers
 from sqlalchemy.orm import configure_mappers
@@ -293,6 +294,24 @@ class MemUsageTest(EnsureZeroed):
 
         go()
 
+    @testing.requires.cextensions
+    def test_cycles_in_row(self):
+
+        tup = result.result_tuple(["a", "b", "c"])
+
+        @profile_memory()
+        def go():
+            obj = {"foo": {}}
+            obj["foo"]["bar"] = obj
+
+            row = tup([1, 2, obj])
+
+            obj["foo"]["row"] = row
+
+            del row
+
+        go()
+
     def test_ad_hoc_types(self):
         """test storage of bind processors, result processors
         in dialect-wide registry."""
-- 
2.47.2