From: Olivier Grisel <olivier.grisel@ensta.org>
Date: Wed, 25 Apr 2018 13:54:00 +0000 (-0400)
Subject: Fix reference leak in compiled cache
X-Git-Tag: rel_1_3_0b1~197
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=30b02003a70f37aa83e20de6229afe2a3600b648;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

Fix reference leak in compiled cache

Fixed a reference leak issue where the values of the parameter dictionary
used in a statement execution would remain referenced by the "compiled
cache", as a result of storing the key view used by Python 3 dictionary
keys().  Pull request courtesy Olivier Grisel.

Change-Id: Icfb0f38111a165780f6dd3e4e3382a03df79ce26
Pull-request: https://github.com/zzzeek/sqlalchemy/pull/441
---

diff --git a/doc/build/changelog/unreleased_12/pr_441.rst b/doc/build/changelog/unreleased_12/pr_441.rst
new file mode 100644
index 0000000000..28ddb7dda7
--- /dev/null
+++ b/doc/build/changelog/unreleased_12/pr_441.rst
@@ -0,0 +1,8 @@
+.. change::
+    :tags: bug, engine
+    :versions: 1.3.0b1
+
+    Fixed a reference leak issue where the values of the parameter dictionary
+    used in a statement execution would remain referenced by the "compiled
+    cache", as a result of storing the key view used by Python 3 dictionary
+    keys().  Pull request courtesy Olivier Grisel.
diff --git a/lib/sqlalchemy/engine/base.py b/lib/sqlalchemy/engine/base.py
index aa9358cd69..4a057ee596 100644
--- a/lib/sqlalchemy/engine/base.py
+++ b/lib/sqlalchemy/engine/base.py
@@ -1023,9 +1023,9 @@ class Connection(Connectable):
 
         distilled_params = _distill_params(multiparams, params)
         if distilled_params:
-            # note this is usually dict but we support RowProxy
-            # as well; but dict.keys() as an iterable is OK
-            keys = distilled_params[0].keys()
+            # ensure we don't retain a link to the view object for keys()
+            # which links to the values, which we don't want to cache
+            keys = list(distilled_params[0].keys())
         else:
             keys = []
 
diff --git a/test/engine/test_execute.py b/test/engine/test_execute.py
index 2d602fa125..30c41c6ee5 100644
--- a/test/engine/test_execute.py
+++ b/test/engine/test_execute.py
@@ -1,12 +1,15 @@
 # coding: utf-8
 
+import weakref
 from sqlalchemy.testing import eq_, assert_raises, assert_raises_message, \
     config, is_, is_not_, le_, expect_warnings
 import re
 from sqlalchemy.testing.util import picklers
+from sqlalchemy.testing.util import gc_collect
 from sqlalchemy.interfaces import ConnectionProxy
 from sqlalchemy import MetaData, Integer, String, INT, VARCHAR, func, \
-    bindparam, select, event, TypeDecorator, create_engine, Sequence
+    bindparam, select, event, TypeDecorator, create_engine, Sequence, \
+    LargeBinary
 from sqlalchemy.sql import column, literal
 from sqlalchemy.testing.schema import Table, Column
 import sqlalchemy as tsa
@@ -42,7 +45,7 @@ class ExecuteTest(fixtures.TestBase):
         users = Table(
             'users', metadata,
             Column('user_id', INT, primary_key=True, autoincrement=False),
-            Column('user_name', VARCHAR(20)),
+            Column('user_name', VARCHAR(20))
         )
         users_autoinc = Table(
             'users_autoinc', metadata,
@@ -770,6 +773,49 @@ class CompiledCacheTest(fixtures.TestBase):
         assert len(cache) == 1
         eq_(conn.execute("select count(*) from users").scalar(), 3)
 
+    @testing.only_on(["sqlite", "mysql", "postgresql"],
+                     "uses blob value that is problematic for some DBAPIs")
+    @testing.provide_metadata
+    def test_cache_noleak_on_statement_values(self):
+        # This is a non regression test for an object reference leak caused
+        # by the compiled_cache.
+
+        metadata = self.metadata
+        photo = Table(
+            'photo', metadata,
+            Column('id', Integer, primary_key=True,
+                   test_needs_autoincrement=True),
+            Column('photo_blob', LargeBinary()),
+        )
+        metadata.create_all()
+
+        conn = testing.db.connect()
+        cache = {}
+        cached_conn = conn.execution_options(compiled_cache=cache)
+
+        class PhotoBlob(bytearray):
+            pass
+
+        blob = PhotoBlob(100)
+        ref_blob = weakref.ref(blob)
+
+        ins = photo.insert()
+        with patch.object(
+            ins, "compile",
+                Mock(side_effect=ins.compile)) as compile_mock:
+            cached_conn.execute(ins, {'photo_blob': blob})
+        eq_(compile_mock.call_count, 1)
+        eq_(len(cache), 1)
+        eq_(conn.execute("select count(*) from photo").scalar(), 1)
+
+        del blob
+
+        gc_collect()
+
+        # The compiled statement cache should not hold any reference to the
+        # the statement values (only the keys).
+        eq_(ref_blob(), None)
+
     def test_keys_independent_of_ordering(self):
         conn = testing.db.connect()
         conn.execute(