Fix TryOpenInner InvalidCast race and add unit regression tests (#3314) (#4179)

Author: paulmedynski

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs

@@ -2242,7 +2242,18 @@ private bool TryOpenInner(TaskCompletionSource<DbConnectionInternal> retry)
             }
             // does not require GC.KeepAlive(this) because of ReRegisterForFinalize below.
 
-            var tdsInnerConnection = (SqlConnectionInternal)InnerConnection;
+            // Capture InnerConnection once into a local to avoid a TOCTOU race: another thread
+            // concurrently calling Open() on the same SqlConnection instance can change
+            // _innerConnection to DbConnectionClosedConnecting between the TryOpenConnection()
+            // call above and the cast below. Without this local capture the second read of
+            // InnerConnection may return DbConnectionClosedConnecting, which is not assignable
+            // to SqlConnectionInternal and would produce an opaque InvalidCastException.
+            // See GitHub issue #3314.
+            var innerConnection = InnerConnection;
+            if (innerConnection is not SqlConnectionInternal tdsInnerConnection)
+            {
+                throw ADP.ConnectionAlreadyOpen(innerConnection.State);
+            }
 
             Debug.Assert(tdsInnerConnection.Parser != null, "Where's the parser?");
 

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

@@ -0,0 +1,113 @@
+// 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.
+
+using System;
+using System.Data;
+using System.Reflection;
+using System.Threading.Tasks;
+using Microsoft.Data.ProviderBase;
+using Microsoft.Data.SqlClient.Connection;
+using Xunit;
+
+namespace Microsoft.Data.SqlClient.UnitTests.Microsoft.Data.SqlClient
+{
+    /// <summary>
+    /// Regression tests for GitHub issue #3314.
+    ///
+    /// Root cause: TryOpenInner() read InnerConnection twice - once for TryOpenConnection() and
+    /// again for the cast to SqlConnectionInternal. Between those two reads another thread could
+    /// change _innerConnection to DbConnectionClosedConnecting, which is not assignable to
+    /// SqlConnectionInternal, causing an opaque InvalidCastException.
+    ///
+    /// Fix: InnerConnection is now captured into a local variable once; if it is not a
+    /// SqlConnectionInternal an InvalidOperationException with a descriptive message is thrown
+    /// instead of an InvalidCastException.
+    /// </summary>
+    public class SqlConnectionConcurrentOpenTests
+    {
+        private static readonly MethodInfo s_tryOpenInner = typeof(SqlConnection)
+            .GetMethod("TryOpenInner", BindingFlags.Instance | BindingFlags.NonPublic)!;
+
+        private static DbConnectionInternal GetConnectingSingleton()
+        {
+            return DbConnectionClosedConnecting.SingletonInstance;
+        }
+
+        private static void ForceInnerConnection(SqlConnection connection, DbConnectionInternal innerConnectionValue)
+        {
+            connection.SetInnerConnectionTo(innerConnectionValue);
+        }
+
+        private static bool InvokeTryOpenInner(SqlConnection connection, TaskCompletionSource<DbConnectionInternal> retry)
+        {
+            try
+            {
+                return (bool)s_tryOpenInner.Invoke(connection, [retry])!;
+            }
+            catch (TargetInvocationException tie) when (tie.InnerException != null)
+            {
+                throw tie.InnerException;
+            }
+        }
+
+        [Fact]
+        public void InnerConnection_DbConnectionClosedConnecting_IsNotAssignableToSqlConnectionInternal()
+        {
+            DbConnectionInternal connectingSingleton = GetConnectingSingleton();
+
+            Assert.False(
+                connectingSingleton is SqlConnectionInternal,
+                "DbConnectionClosedConnecting must NOT be assignable to SqlConnectionInternal. " +
+                "If it were, the race condition in #3314 would not manifest.");
+        }
+
+        [Fact]
+        public void InnerConnection_InConnectingState_ReportsConnectingState()
+        {
+            DbConnectionInternal connectingSingleton = GetConnectingSingleton();
+
+            var connection = new SqlConnection("Data Source=localhost");
+            ForceInnerConnection(connection, connectingSingleton);
+
+            Assert.Equal(ConnectionState.Connecting, connection.State);
+        }
+
+        [Fact]
+        public void Open_WhenAlreadyConnecting_ThrowsInvalidOperation()
+        {
+            DbConnectionInternal connectingSingleton = GetConnectingSingleton();
+
+            var connection = new SqlConnection("Data Source=localhost");
+            ForceInnerConnection(connection, connectingSingleton);
+
+            Assert.Throws<InvalidOperationException>(() => connection.Open());
+        }
+
+        [Fact]
+        public void TryOpenInner_WhenInnerConnectionRacesToNonSqlConnectionInternalState_ThrowsInvalidOperation_NotInvalidCast()
+        {
+            DbConnectionInternal initialConnectingState = GetConnectingSingleton();
+            DbConnectionInternal racedNonSqlConnectionInternalState = DbConnectionOpenBusy.SingletonInstance;
+
+            var connection = new SqlConnection("Data Source=localhost");
+            ForceInnerConnection(connection, initialConnectingState);
+
+            TaskCompletionSource<DbConnectionInternal> completedRetry = new();
+            completedRetry.SetResult(racedNonSqlConnectionInternalState);
+
+            Exception ex = Assert.ThrowsAny<Exception>(() =>
+            {
+                InvokeTryOpenInner(connection, completedRetry);
+            });
+
+            Assert.True(
+                ex is InvalidOperationException,
+                $"Expected InvalidOperationException but got {ex.GetType().Name}: {ex.Message}. " +
+                "The fix for #3314 must throw InvalidOperationException (not InvalidCastException) " +
+                "when _innerConnection races to a non-SqlConnectionInternal state inside TryOpenInner.");
+
+            Assert.Contains("connection", ex.Message, StringComparison.OrdinalIgnoreCase);
+        }
+    }
+}