Fix | Scope configurable retry logic assembly resolution to opt-in callers

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Reliability/SqlConfigurableRetryLogicLoader.cs

@@ -50,14 +50,6 @@ public SqlConfigurableRetryLogicLoader(
             string cnnSectionName = SqlConfigurableRetryConnectionSection.Name,
             string cmdSectionName = SqlConfigurableRetryCommandSection.Name)
         {
-            #if NET
-            // Just only one subscription to this event is required.
-            // This class isn't supposed to be called more than one time;
-            // SqlConfigurableRetryLogicManager manages a single instance of this class.
-            System.Runtime.Loader.AssemblyLoadContext.Default.Resolving -= Default_Resolving;
-            System.Runtime.Loader.AssemblyLoadContext.Default.Resolving += Default_Resolving;
-            #endif
-
             AssignProviders(connectionRetryConfigs == null ? null : CreateRetryLogicProvider(cnnSectionName, connectionRetryConfigs),
                             commandRetryConfigs == null ? null : CreateRetryLogicProvider(cmdSectionName, commandRetryConfigs));
         }
@@ -119,20 +111,36 @@ private static SqlRetryLogicBaseProvider ResolveRetryLogicProvider(string config
                 throw new ArgumentNullException(nameof(retryMethod), StringsHelper.GetString(Strings.SQLCR_RetryMethodNullOrEmpty));
             }
 
-            Type type = null;
-            try
+            Type type;
+            if (string.IsNullOrEmpty(configurableRetryType))
             {
-                // Resolve a Type object from the given type name
-                // Different implementation in .NET Framework & .NET Core
-                type = LoadType(configurableRetryType);
+                // No custom retry logic type was configured, so there is nothing to resolve and the
+                // built-in factory is used to discover the requested retry method.
+                //
+                // Short-circuiting here matters for more than performance: on .NET, LoadType
+                // temporarily subscribes an assembly resolving handler to the default
+                // AssemblyLoadContext. That handler should never be installed on behalf of callers
+                // who did not explicitly opt in to loading a custom retry logic type.
+                type = typeof(SqlConfigurableRetryFactory);
+                SqlClientEventSource.Log.TryTraceEvent("<sc.{0}.{1}|INFO> No custom retry logic type is configured; Using the internal `{2}` type.",
+                                                       TypeName, methodName, type.FullName);
             }
-            catch (Exception e)
+            else
             {
-                // Try to use 'SqlConfigurableRetryFactory' as a default type to discover retry methods
-                // if there is a problem, resolve using the 'configurableRetryType' type.
-                type = typeof(SqlConfigurableRetryFactory);
-                SqlClientEventSource.Log.TryTraceEvent("<sc.{0}.{1}|INFO> Unable to load the '{2}' type; Trying to use the internal `{3}` type: {4}",
-                                                       TypeName, methodName, configurableRetryType, type.FullName, e);
+                try
+                {
+                    // Resolve a Type object from the given type name
+                    // Different implementation in .NET Framework & .NET Core
+                    type = LoadType(configurableRetryType);
+                }
+                catch (Exception e)
+                {
+                    // Try to use 'SqlConfigurableRetryFactory' as a default type to discover retry methods
+                    // if there is a problem, resolve using the 'configurableRetryType' type.
+                    type = typeof(SqlConfigurableRetryFactory);
+                    SqlClientEventSource.Log.TryTraceEvent("<sc.{0}.{1}|INFO> Unable to load the '{2}' type; Trying to use the internal `{3}` type: {4}",
+                                                           TypeName, methodName, configurableRetryType, type.FullName, e);
+                }
             }
 
             // Run the function by using the resolved values to get the SqlRetryLogicBaseProvider object
@@ -336,25 +344,42 @@ private static ICollection<int> SplitErrorNumberList(string list)
         #region Type Resolution
 
         #if NET
+        /// <summary>
+        /// The directory that user-supplied configurable retry logic assemblies are probed from.
+        /// </summary>
+        /// <remarks>
+        /// This is deliberately the application base directory rather than the current working
+        /// directory. The working directory is ambient process state that can be changed at any
+        /// time and is not necessarily related to where the application's binaries live, so
+        /// probing it can load assemblies from an unintended and untrusted location.
+        /// </remarks>
+        private static string ProbingDirectory => AppContext.BaseDirectory;
+
         private static Assembly AssemblyResolver(AssemblyName arg)
         {
             string methodName = nameof(AssemblyResolver);
 
-            string fullPath = MakeFullPath(Environment.CurrentDirectory, arg.Name);
+            string fullPath = MakeFullPath(ProbingDirectory, arg.Name);
             SqlClientEventSource.Log.TryTraceEvent("<sc.{0}.{1}|INFO> Looking for '{2}' assembly by '{3}' full path."
                 , TypeName, methodName, arg, fullPath);
 
             return fullPath == null ? null : AssemblyLoadContext.Default.LoadFromAssemblyPath(fullPath);
         }
 
         /// <summary>
-        /// Load assemblies on request.
+        /// Load dependencies of a user-supplied configurable retry logic assembly on request.
         /// </summary>
+        /// <remarks>
+        /// This handler is only subscribed for the duration of the type resolution performed by
+        /// <see cref="LoadType"/>, and only when a custom retry logic type has been configured.
+        /// It must never remain subscribed to <see cref="AssemblyLoadContext.Default"/> after that,
+        /// because doing so changes assembly resolution behavior for the entire application.
+        /// </remarks>
         private static Assembly Default_Resolving(AssemblyLoadContext arg1, AssemblyName arg2)
         {
             string methodName = nameof(Default_Resolving);
 
-            string target = MakeFullPath(Environment.CurrentDirectory, arg2.Name);
+            string target = MakeFullPath(ProbingDirectory, arg2.Name);
             SqlClientEventSource.Log.TryTraceEvent("<sc.{0}.{1}|INFO> Looking for '{2}' assembly that is requested by '{3}' ALC from '{4}' path."
                 , TypeName, methodName, arg2, arg1, target);
 
@@ -371,7 +396,21 @@ private static Type LoadType(string fullyQualifiedName)
             string methodName = nameof(LoadType);
             SqlClientEventSource.Log.TryTraceEvent("<sc.{0}.{1}|INFO> Entry point.", TypeName, methodName);
 
-            var result = Type.GetType(fullyQualifiedName, AssemblyResolver, TypeResolver);
+            Type result;
+
+            // Scope the resolving handler to this call only. Leaving it subscribed would alter
+            // assembly resolution for the whole application, so unrelated assemblies the host
+            // failed to find could be served from this component's probing directory.
+            AssemblyLoadContext.Default.Resolving += Default_Resolving;
+            try
+            {
+                result = Type.GetType(fullyQualifiedName, AssemblyResolver, TypeResolver);
+            }
+            finally
+            {
+                AssemblyLoadContext.Default.Resolving -= Default_Resolving;
+            }
+
             if (result != null)
             {
                 SqlClientEventSource.Log.TryTraceEvent("<sc.{0}.{1}|INFO> The '{2}' type is resolved.",

src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConfigurableRetryLogicTest.cs

@@ -3,6 +3,8 @@
 // See the LICENSE file in the project root for more information.
 
 using System;
+using System.IO;
+using System.Reflection;
 using System.Threading.Tasks;
 using Xunit;
 
@@ -80,5 +82,49 @@ public void ValidateRetryParameters()
             option.AuthorizedSqlCondition = null;
             SqlConfigurableRetryFactory.CreateIncrementalRetryProvider(option);
         }
+
+#if NET
+        /// <summary>
+        /// Regression test: triggering the configurable retry logic loader through its normal
+        /// entry points must not leave a process-wide
+        /// <see cref="System.Runtime.Loader.AssemblyLoadContext.Default"/> resolving handler
+        /// installed. Such a handler participates in resolution of every assembly the host
+        /// application fails to find, and serves them out of this component's probing directory,
+        /// which can load code from an unintended location.
+        /// </summary>
+        [Fact]
+        public void RetryLogicProviderDoesNotLeaveAssemblyProbingEnabled()
+        {
+            // Touch the default retry logic providers to force SqlConfigurableRetryLogicLoader
+            // construction via its normal code path.
+            Assert.NotNull(new SqlCommand().RetryLogicProvider);
+            Assert.NotNull(new SqlConnection().RetryLogicProvider);
+
+            // A file that is not a valid assembly, planted in the loader's probing directory
+            // under a name no other component could be asking for. If a handler is still
+            // subscribed it finds this file and fails with BadImageFormatException. With correct
+            // behavior the runtime never looks here and reports the assembly as simply not found.
+            string assemblySimpleName = "MdsProbeAssembly_" + Guid.NewGuid().ToString("N");
+            string plantedFile = Path.Combine(AppContext.BaseDirectory, assemblySimpleName + ".dll");
+
+            File.WriteAllText(plantedFile, "not an assembly");
+            try
+            {
+                Assert.Throws<FileNotFoundException>(
+                    () => Assembly.Load(new AssemblyName(assemblySimpleName)));
+            }
+            finally
+            {
+                try
+                {
+                    File.Delete(plantedFile);
+                }
+                catch (IOException)
+                {
+                    // Best effort cleanup.
+                }
+            }
+        }
+#endif
     }
 }

src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConfigurableRetryLogicLoaderTest.cs

@@ -0,0 +1,145 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+#if NET
+
+using System;
+using System.IO;
+using System.Reflection;
+using System.Runtime.Loader;
+using Xunit;
+
+namespace Microsoft.Data.SqlClient.UnitTests;
+
+/// <summary>
+/// Unit tests validating that <see cref="SqlConfigurableRetryLogicLoader"/> never leaves a
+/// process-wide assembly resolving handler attached to
+/// <see cref="AssemblyLoadContext.Default"/>.
+/// </summary>
+/// <remarks>
+/// A handler left attached there participates in resolution of every assembly the host
+/// application fails to find, not just the retry logic assembly the loader was interested in.
+/// That silently changes assembly loading behaviour for code that never opted in to this
+/// feature, and can serve unrelated assemblies out of this component's probing directory.
+/// </remarks>
+public class SqlConfigurableRetryLogicLoaderTest
+{
+    /// <summary>
+    /// The default code path: no configuration at all. The loader must not subscribe to the
+    /// default load context.
+    /// </summary>
+    [Fact]
+    public void Constructor_WithNoConfiguration_DoesNotLeaveAssemblyProbingEnabled()
+    {
+        _ = new SqlConfigurableRetryLogicLoader(null, null);
+
+        AssertNoAssemblyProbingHandlerInstalled();
+    }
+
+    /// <summary>
+    /// A configuration that supplies only a retry method, which is the documented way to select
+    /// one of the built-in retry providers. No custom type is being requested, so no assembly
+    /// resolution is required and no handler may be subscribed - not even transiently.
+    /// </summary>
+    [Theory]
+    [InlineData(null)]
+    [InlineData("")]
+    public void Constructor_WithoutRetryLogicType_DoesNotLeaveAssemblyProbingEnabled(string? retryLogicType)
+    {
+        TestRetryConnectionSection section = CreateSection(retryLogicType);
+
+        SqlConfigurableRetryLogicLoader loader = new(section, null);
+
+        // The built-in factory still resolves the requested method.
+        Assert.NotNull(loader.ConnectionProvider);
+        AssertNoAssemblyProbingHandlerInstalled();
+    }
+
+    /// <summary>
+    /// A configuration that requests a custom retry logic type legitimately needs assembly
+    /// resolution, but the handler must be removed again once type resolution has finished.
+    /// </summary>
+    [Fact]
+    public void Constructor_WithUnresolvableRetryLogicType_DoesNotLeaveAssemblyProbingEnabled()
+    {
+        TestRetryConnectionSection section =
+            CreateSection("Some.Namespace.NoSuchType, Some.Assembly.That.Does.Not.Exist");
+
+        SqlConfigurableRetryLogicLoader loader = new(section, null);
+
+        // Resolution fails and falls back to the built-in factory rather than throwing.
+        Assert.NotNull(loader.ConnectionProvider);
+        AssertNoAssemblyProbingHandlerInstalled();
+    }
+
+    private static TestRetryConnectionSection CreateSection(string? retryLogicType) =>
+        new()
+        {
+            RetryLogicType = retryLogicType!,
+            RetryMethod = nameof(SqlConfigurableRetryFactory.CreateFixedRetryProvider),
+            NumberOfTries = 2,
+            DeltaTime = TimeSpan.FromSeconds(1),
+            MinTimeInterval = TimeSpan.Zero,
+            MaxTimeInterval = TimeSpan.FromSeconds(10),
+        };
+
+    /// <summary>
+    /// Asserts that a failed assembly load is not served out of the loader's probing directory,
+    /// which can only happen while a resolving handler installed by
+    /// <see cref="SqlConfigurableRetryLogicLoader"/> is subscribed to
+    /// <see cref="AssemblyLoadContext.Default"/>.
+    /// </summary>
+    /// <remarks>
+    /// A file that is not a valid assembly is planted in the probing directory under a name no
+    /// other component could be asking for. If a handler is still subscribed it finds that file
+    /// and tries to load it, which surfaces as <see cref="BadImageFormatException"/>. With no
+    /// handler subscribed the runtime never looks there and reports the assembly as simply not
+    /// found. This asserts the behaviour that actually matters to a host application rather than
+    /// inspecting loader or runtime internals.
+    /// </remarks>
+    private static void AssertNoAssemblyProbingHandlerInstalled()
+    {
+        // The probing directory is the application base directory, which for a test run is the
+        // directory the test assembly was loaded from.
+        string assemblySimpleName = "MdsProbeAssembly_" + Guid.NewGuid().ToString("N");
+        string plantedFile = Path.Combine(AppContext.BaseDirectory, assemblySimpleName + ".dll");
+
+        File.WriteAllText(plantedFile, "not an assembly");
+        try
+        {
+            Assert.Throws<FileNotFoundException>(
+                () => Assembly.Load(new AssemblyName(assemblySimpleName)));
+        }
+        finally
+        {
+            try
+            {
+                File.Delete(plantedFile);
+            }
+            catch (IOException)
+            {
+                // Best effort cleanup.
+            }
+        }
+    }
+
+    private sealed class TestRetryConnectionSection : ISqlConfigurableRetryConnectionSection
+    {
+        public TimeSpan DeltaTime { get; set; }
+
+        public TimeSpan MaxTimeInterval { get; set; }
+
+        public TimeSpan MinTimeInterval { get; set; }
+
+        public int NumberOfTries { get; set; }
+
+        public string RetryLogicType { get; set; } = string.Empty;
+
+        public string RetryMethod { get; set; } = string.Empty;
+
+        public string TransientErrors { get; set; } = string.Empty;
+    }
+}
+
+#endif