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

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 324978202fb32d4c1e2f6daf6af197b875938dc7..ee9251d0d14967d3f7b0b263d7a2a98697bae826 100644 (file)
--- a/lib/sqlalchemy/engine/create.py
+++ b/lib/sqlalchemy/engine/create.py
@@ -614,9 +614,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 = list(cargs_tup)  # allow mutability
+    (cargs_tup, _cparams) = dialect.create_connect_args(u)
+    cparams = util.immutabledict(_cparams).union(pop_kwarg("connect_args", {}))
 
     # look for existing pool or create
     pool = pop_kwarg("pool", None)
@@ -626,15 +625,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 600cf10da9cf8252f05eecc2e4f8ccfb770b8428..98a42f11e9f97f0996d24540178dca1f026021b5 100644 (file)
--- a/test/engine/test_execute.py
+++ b/test/engine/test_execute.py
@@ -3955,6 +3955,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