make local mutable copies for cargs / cparams in do_connect

Fixed a critical issue in :class:`.Engine` where connections created in
conjunction with the :meth:`.ConnectionEvents.do_connect` event listeners
would receive shared, mutable collections for the connection arguments,
leading to a variety of potential issues including unlimited growth of the
argument list as well as elements within the parameter dictionary being
shared among concurrent connection calls.  In particular this could impact
do_connect routines making use of complex mutable authentication
structures.

Fixes: #13144
Change-Id: I1549dae36e8e7e6cf50fdaf796659b53e7b78234
(cherry picked from commit dfb1c49cd7306eeca49fd7bb7ec4bcbef0e68d79)

diff --git a/doc/build/changelog/unreleased_20/13144.rst b/doc/build/changelog/unreleased_20/13144.rst
new file mode 100644 (file)
index 0000000..1514afc
--- /dev/null
+++ b/doc/build/changelog/unreleased_20/13144.rst
@@ -0,0 +1,12 @@
+.. change::
+    :tags: bug, engine
+    :tickets: 13144
+
+    Fixed a critical issue in :class:`.Engine` where connections created in
+    conjunction with the :meth:`.ConnectionEvents.do_connect` event listeners
+    would receive shared, mutable collections for the connection arguments,
+    leading to a variety of potential issues including unlimited growth of the
+    argument list as well as elements within the parameter dictionary being
+    shared among concurrent connection calls.  In particular this could impact
+    do_connect routines making use of complex mutable authentication
+    structures.

diff --git a/lib/sqlalchemy/engine/create.py b/lib/sqlalchemy/engine/create.py
index 9abb08f3252d3263d2bdce7732f3c3e0ad84da8e..9ab94763b0ad384bea4abcd14c87cddcbdaca9eb 100644 (file)
--- a/lib/sqlalchemy/engine/create.py
+++ b/lib/sqlalchemy/engine/create.py
@@ -630,8 +630,8 @@ def create_engine(url: Union[str, _url.URL], **kwargs: Any) -> Engine:
     dialect = dialect_cls(**dialect_args)
 
     # assemble connection arguments
-    (cargs_tup, cparams) = dialect.create_connect_args(u)
-    cparams.update(pop_kwarg("connect_args", {}))
+    (cargs_tup, _cparams) = dialect.create_connect_args(u)
+    cparams = util.immutabledict(_cparams).union(pop_kwarg("connect_args", {}))
 
     if "async_fallback" in cparams and util.asbool(cparams["async_fallback"]):
         util.warn_deprecated(
@@ -640,8 +640,6 @@ def create_engine(url: Union[str, _url.URL], **kwargs: Any) -> Engine:
             "2.0",
         )
 
-    cargs = list(cargs_tup)  # allow mutability
-
     # look for existing pool or create
     pool = pop_kwarg("pool", None)
     if pool is None:
@@ -650,15 +648,23 @@ def create_engine(url: Union[str, _url.URL], **kwargs: Any) -> Engine:
             connection_record: Optional[ConnectionPoolEntry] = None,
         ) -> DBAPIConnection:
             if dialect._has_events:
+                mutable_cargs = list(cargs_tup)
+                mutable_cparams = dict(cparams)
                 for fn in dialect.dispatch.do_connect:
                     connection = cast(
                         DBAPIConnection,
-                        fn(dialect, connection_record, cargs, cparams),
+                        fn(
+                            dialect,
+                            connection_record,
+                            mutable_cargs,
+                            mutable_cparams,
+                        ),
                     )
                     if connection is not None:
                         return connection
-
-            return dialect.connect(*cargs, **cparams)
+                return dialect.connect(*mutable_cargs, **mutable_cparams)
+            else:
+                return dialect.connect(*cargs_tup, **cparams)
 
         creator = pop_kwarg("creator", connect)
 

diff --git a/test/engine/test_execute.py b/test/engine/test_execute.py
index c0d80a738bb7246c1ad7e1a379d1f0f66a9b17ee..4c6bf8a28fb3e34941150bc08a4b563670cb70c0 100644 (file)
--- a/test/engine/test_execute.py
+++ b/test/engine/test_execute.py
@@ -3897,6 +3897,54 @@ class DialectEventTest(fixtures.TestBase):
         with e.connect() as conn:
             eq_(conn.info["boom"], "one")
 
+    def test_connect_do_connect_no_cargs_cparams_leak_nullpool(self):
+        """test #13144, cargs/cparams in do_connect are not shared"""
+
+        e = engines.testing_engine(
+            options={"_initialize": False, "poolclass": NullPool}
+        )
+
+        m1 = Mock()
+        e.dialect.connect = m1.mock_connect
+
+        cargs_list = []
+        cparams_list = []
+        call_count = 0
+
+        @event.listens_for(e, "do_connect")
+        def evt(dialect, conn_rec, cargs, cparams):
+            nonlocal call_count
+            cargs_list.append(cargs)
+            cparams_list.append(cparams)
+
+            cargs.append(f"extra_arg_{call_count}")
+            cparams[f"extra_param_{call_count}"] = "value"
+            call_count += 1
+
+        for _ in range(3):
+            with e.connect():
+                pass
+
+        all_list_keys = ["extra_arg_0", "extra_arg_1", "extra_arg_2"]
+
+        for carg, expected in zip(cargs_list, all_list_keys):
+            eq_(set(carg).intersection(all_list_keys), {expected})
+
+        all_param_keys = ["extra_param_0", "extra_param_1", "extra_param_2"]
+        for cparam, expected in zip(cparams_list, all_param_keys):
+            eq_(set(cparam).intersection(all_param_keys), {expected})
+
+        eq_(
+            len(set(id(carg) for carg in cargs_list)),
+            3,
+            "cargs should be different objects",
+        )
+        eq_(
+            len(set(id(cparam) for cparam in cparams_list)),
+            3,
+            "cparams should be different objects",
+        )
+
 
 class SetInputSizesTest(fixtures.TablesTest):
     __backend__ = True