Skip to content

Merge | Remove OS-specific TdsParser files #3469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Merge | Remove OS-specific TdsParser files #3469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
aaa8161
Remove uncalled LoadSSPILibrary method
edwardneal Jul 9, 2025
a66fe7e
netcore: move PostReadAsyncForMars to TdsParserStateObject
edwardneal Jul 9, 2025
494e74a
Merge TdsParser handling of PostReadAsyncForMars
edwardneal Jul 9, 2025
1e677d0
netcore: remove WaitForSSLHandShakeToComplete
edwardneal Jul 9, 2025
6c84a21
Remove now-unused files
edwardneal Jul 9, 2025
0bb49cf
Merge and move WaitForSSLHandShakeToComplete
edwardneal Jul 9, 2025
86f4c91
Coding style cleanup
edwardneal Jul 9, 2025
bb7ad23
Merge branch 'main' into merge/tdsparser-partial-methods
edwardneal Jul 9, 2025
f39e962
Remove OS-specific TdsParser files from netcore csproj
edwardneal Jul 13, 2025
ef3ca3a
Respond to code review
edwardneal Jul 25, 2025
8bf5970
Merge main
edwardneal Jul 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -958,7 +958,6 @@
<Link>Microsoft\Data\SqlTypes\SqlFileStream.Windows.cs</Link>
</Compile>

<Compile Include="Microsoft\Data\SqlClient\TdsParser.Windows.cs" />
<Compile Include="Microsoft\Data\SqlClient\TdsParserStateObjectNative.cs" />

<EmbeddedResource Include="$(CommonSourceRoot)Resources\ILLink.Substitutions.Windows.xml">
Expand Down Expand Up @@ -1006,8 +1005,6 @@
<Link>Microsoft\Data\SqlTypes\SqlFileStream.netcore.Unix.cs</Link>
</Compile>

<Compile Include="Microsoft\Data\SqlClient\TdsParser.Unix.cs" />

<EmbeddedResource Include="$(CommonSourceRoot)Resources\ILLink.Substitutions.Unix.xml">
<LogicalName>ILLink.Substitutions.xml</LogicalName>
<Link>Resources\ILLink.Substitutions.Unix.xml</Link>
Expand Down
26 changes: 0 additions & 26 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.Unix.cs
View file
Open in desktop

This file was deleted.

63 changes: 0 additions & 63 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.Windows.cs
View file
Open in desktop

This file was deleted.

31 changes: 25 additions & 6 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,8 @@
using System.Diagnostics;
using System.Globalization;
using System.IO;
#if NET
using System.Security.Authentication;
#else
#if NETFRAMEWORK
using System.Runtime.CompilerServices;
#endif
using System.Text;
Expand Down Expand Up @@ -632,7 +631,13 @@ internal void EnableMars()
ThrowExceptionAndWarning(_physicalStateObj);
}

PostReadAsyncForMars();
error = _pMarsPhysicalConObj.PostReadAsyncForMars(_physicalStateObj);
if (error != TdsEnums.SNI_SUCCESS_IO_PENDING)
{
Debug.Assert(error != TdsEnums.SNI_SUCCESS, "Unexpected successful read async on physical connection before enabling MARS!");
_physicalStateObj.AddError(ProcessSNIError(_physicalStateObj));
ThrowExceptionAndWarning(_physicalStateObj);
}

_physicalStateObj = CreateSession(); // Create and open default MARS stateObj and connection.
}
Expand Down Expand Up @@ -888,10 +893,24 @@ private void EnableSsl(uint info, SqlConnectionEncryptOption encrypt, bool integ
ThrowExceptionAndWarning(_physicalStateObj);
}

int protocolVersion = 0;
WaitForSSLHandShakeToComplete(ref error, ref protocolVersion);
SslProtocols protocol = 0;

// in the case where an async connection is made, encryption is used and Windows Authentication is used,
// wait for SSL handshake to complete, so that the SSL context is fully negotiated before we try to use its
// Channel Bindings as part of the Windows Authentication context build (SSL handshake must complete
// before calling SNISecGenClientContext).
#if NET
if (OperatingSystem.IsWindows())
#endif
{
error = _physicalStateObj.WaitForSSLHandShakeToComplete(out protocol);
if (error != TdsEnums.SNI_SUCCESS)
{
_physicalStateObj.AddError(ProcessSNIError(_physicalStateObj));
ThrowExceptionAndWarning(_physicalStateObj);
}
}

SslProtocols protocol = (SslProtocols)protocolVersion;
string warningMessage = protocol.GetProtocolWarning();
if (!string.IsNullOrEmpty(warningMessage))
{
Expand Down
2 changes: 0 additions & 2 deletions ...osoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.netcore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,6 @@ internal abstract void CreatePhysicalSNIHandle(

internal abstract uint EnableSsl(ref uint info, bool tlsFirst, string serverCertificateFilename);

internal abstract uint WaitForSSLHandShakeToComplete(out int protocolVersion);

internal abstract void Dispose();

internal abstract uint CheckConnection();
Expand Down
7 changes: 5 additions & 2 deletions ...rosoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
using System.Security.Authentication;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Data.Common;
Expand Down Expand Up @@ -365,6 +366,8 @@ internal override uint EnableMars(ref uint info)
return TdsEnums.SNI_ERROR;
}

internal override uint PostReadAsyncForMars(TdsParserStateObject physicalStateObject) => TdsEnums.SNI_SUCCESS_IO_PENDING;

internal override uint EnableSsl(ref uint info, bool tlsFirst, string serverCertificateFilename)
{
SniHandle sessionHandle = GetSessionSNIHandleHandleOrThrow();
Expand All @@ -386,10 +389,10 @@ internal override uint SetConnectionBufferSize(ref uint unsignedPacketSize)
return TdsEnums.SNI_SUCCESS;
}

internal override uint WaitForSSLHandShakeToComplete(out int protocolVersion)
internal override uint WaitForSSLHandShakeToComplete(out SslProtocols protocolVersion)
{
protocolVersion = GetSessionSNIHandleHandleOrThrow().ProtocolVersion;
return 0;
return TdsEnums.SNI_SUCCESS;
}

internal override SniErrorDetails GetErrorDetails()
Expand Down
53 changes: 45 additions & 8 deletions ...crosoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectNative.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,43 @@ internal override uint DisableSsl()
internal override uint EnableMars(ref uint info)
=> SniNativeWrapper.SniAddProvider(Handle, Provider.SMUX_PROV, ref info);

internal override uint PostReadAsyncForMars(TdsParserStateObject physicalStateObject)
{
// HACK HACK HACK - for Async only
// Have to post read to initialize MARS - will get callback on this when connection goes
// down or is closed.

PacketHandle temp = default;
uint error = TdsEnums.SNI_SUCCESS;

#if NETFRAMEWORK
RuntimeHelpers.PrepareConstrainedRegions();
#endif
try
{ }
finally
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get this at all ... isn't the point of a finally block to always run regardless of what happens in the try block, or what exceptions are caught? If there's nothing to run in the try block, doesn't it just immediately skip to the finally block? And if it does, what's the point of the try/finally in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is part of the CER infrastructure. The finally block will always run, even in the event of a ThreadAbortException. Without the call to PrepareConstrainedRegions and a try block, this wouldn't be guaranteed.

I think the goal in this specific case is to make sure that if a thread is aborted while setting up the physical MARS connection, the same physical connection is left in a known-good state.

{
IncrementPendingCallbacks();
SessionHandle handle = SessionHandle;
// we do not need to consider partial packets when making this read because we
// expect this read to pend. a partial packet should not exist at setup of the
// parser
Debug.Assert(physicalStateObject.PartialPacket == null);
temp = ReadAsync(handle, out error);

Debug.Assert(temp.Type == PacketHandle.NativePointerType, "unexpected packet type when requiring NativePointer");

if (temp.NativePointer != IntPtr.Zero)
{
// Be sure to release packet, otherwise it will be leaked by native.
ReleasePacket(temp);
}
}

Debug.Assert(IntPtr.Zero == temp.NativePointer, "unexpected syncReadPacket without corresponding SNIPacketRelease");
return error;
}

internal override uint EnableSsl(ref uint info, bool tlsFirst, string serverCertificateFilename)
{
AuthProviderInfo authInfo = new AuthProviderInfo();
Expand All @@ -399,43 +436,43 @@ internal override uint EnableSsl(ref uint info, bool tlsFirst, string serverCert
internal override uint SetConnectionBufferSize(ref uint unsignedPacketSize)
=> SniNativeWrapper.SniSetInfo(Handle, QueryType.SNI_QUERY_CONN_BUFSIZE, ref unsignedPacketSize);

internal override uint WaitForSSLHandShakeToComplete(out int protocolVersion)
internal override uint WaitForSSLHandShakeToComplete(out SslProtocols protocolVersion)
{
uint returnValue = SniNativeWrapper.SniWaitForSslHandshakeToComplete(Handle, GetTimeoutRemaining(), out uint nativeProtocolVersion);
var nativeProtocol = (NativeProtocols)nativeProtocolVersion;

#pragma warning disable CA5398 // Avoid hardcoded SslProtocols values
if (nativeProtocol.HasFlag(NativeProtocols.SP_PROT_TLS1_2_CLIENT) || nativeProtocol.HasFlag(NativeProtocols.SP_PROT_TLS1_2_SERVER))
{
protocolVersion = (int)SslProtocols.Tls12;
protocolVersion = SslProtocols.Tls12;
}
else if (nativeProtocol.HasFlag(NativeProtocols.SP_PROT_TLS1_3_CLIENT) || nativeProtocol.HasFlag(NativeProtocols.SP_PROT_TLS1_3_SERVER))
{
/* The SslProtocols.Tls13 is supported by netcoreapp3.1 and later */
protocolVersion = (int)SslProtocols.Tls13;
protocolVersion = SslProtocols.Tls13;
}
else if (nativeProtocol.HasFlag(NativeProtocols.SP_PROT_TLS1_1_CLIENT) || nativeProtocol.HasFlag(NativeProtocols.SP_PROT_TLS1_1_SERVER))
{
protocolVersion = (int)SslProtocols.Tls11;
protocolVersion = SslProtocols.Tls11;
}
else if (nativeProtocol.HasFlag(NativeProtocols.SP_PROT_TLS1_0_CLIENT) || nativeProtocol.HasFlag(NativeProtocols.SP_PROT_TLS1_0_SERVER))
{
protocolVersion = (int)SslProtocols.Tls;
protocolVersion = SslProtocols.Tls;
}
else if (nativeProtocol.HasFlag(NativeProtocols.SP_PROT_SSL3_CLIENT) || nativeProtocol.HasFlag(NativeProtocols.SP_PROT_SSL3_SERVER))
{
// SSL 2.0 and 3.0 are only referenced to log a warning, not explicitly used for connections
#pragma warning disable CS0618, CA5397
protocolVersion = (int)SslProtocols.Ssl3;
protocolVersion = SslProtocols.Ssl3;
}
else if (nativeProtocol.HasFlag(NativeProtocols.SP_PROT_SSL2_CLIENT) || nativeProtocol.HasFlag(NativeProtocols.SP_PROT_SSL2_SERVER))
{
protocolVersion = (int)SslProtocols.Ssl2;
protocolVersion = SslProtocols.Ssl2;
#pragma warning restore CS0618, CA5397
}
else //if (nativeProtocol.HasFlag(NativeProtocols.SP_PROT_NONE))
{
protocolVersion = (int)SslProtocols.None;
protocolVersion = SslProtocols.None;
}
#pragma warning restore CA5398 // Avoid hardcoded SslProtocols values
return returnValue;
Expand Down
48 changes: 16 additions & 32 deletions src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,8 @@
using System.Diagnostics;
using System.Globalization;
using System.IO;
#if NET
using System.Security.Authentication;
#else
#if NETFRAMEWORK
using System.Runtime.CompilerServices;
#endif
using System.Text;
Expand Down Expand Up @@ -683,31 +682,10 @@ internal void EnableMars()
ThrowExceptionAndWarning(_physicalStateObj);
}

// HACK HACK HACK - for Async only
// Have to post read to intialize MARS - will get callback on this when connection goes
// down or is closed.

IntPtr temp = IntPtr.Zero;

RuntimeHelpers.PrepareConstrainedRegions();
try
{ }
finally
{
_pMarsPhysicalConObj.IncrementPendingCallbacks();

error = SniNativeWrapper.SniReadAsync(_pMarsPhysicalConObj.Handle, ref temp);

if (temp != IntPtr.Zero)
{
// Be sure to release packet, otherwise it will be leaked by native.
SniNativeWrapper.SniPacketRelease(temp);
}
}
Debug.Assert(IntPtr.Zero == temp, "unexpected syncReadPacket without corresponding SNIPacketRelease");
if (TdsEnums.SNI_SUCCESS_IO_PENDING != error)
error = _pMarsPhysicalConObj.PostReadAsyncForMars(_physicalStateObj);
if (error != TdsEnums.SNI_SUCCESS_IO_PENDING)
{
Debug.Assert(TdsEnums.SNI_SUCCESS != error, "Unexpected successful read async on physical connection before enabling MARS!");
Debug.Assert(error != TdsEnums.SNI_SUCCESS, "Unexpected successful read async on physical connection before enabling MARS!");
_physicalStateObj.AddError(ProcessSNIError(_physicalStateObj));
ThrowExceptionAndWarning(_physicalStateObj);
}
Expand Down Expand Up @@ -982,19 +960,25 @@ private void EnableSsl(uint info, SqlConnectionEncryptOption encrypt, bool integ
ThrowExceptionAndWarning(_physicalStateObj);
}

SslProtocols protocol = 0;

// in the case where an async connection is made, encryption is used and Windows Authentication is used,
// wait for SSL handshake to complete, so that the SSL context is fully negotiated before we try to use its
// Channel Bindings as part of the Windows Authentication context build (SSL handshake must complete
// before calling SNISecGenClientContext).
error = SniNativeWrapper.SniWaitForSslHandshakeToComplete(_physicalStateObj.Handle, _physicalStateObj.GetTimeoutRemaining(), out uint protocolVersion);

if (error != TdsEnums.SNI_SUCCESS)
#if NET
if (OperatingSystem.IsWindows())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My hope is eventually we will have compile time constants for this so we can replace the whole check with #if WINDOWS around the whole if block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being able to use #if WINDOWS or #if UNIX would be good, it'd simplify the OS-specific files in general. In this specific case, I'm planning to enable the SSL protocol warning in a separate PR.

#endif
{
_physicalStateObj.AddError(ProcessSNIError(_physicalStateObj));
ThrowExceptionAndWarning(_physicalStateObj);
error = _physicalStateObj.WaitForSSLHandShakeToComplete(out protocol);
if (error != TdsEnums.SNI_SUCCESS)
{
_physicalStateObj.AddError(ProcessSNIError(_physicalStateObj));
ThrowExceptionAndWarning(_physicalStateObj);
}
}

string warningMessage = ((System.Security.Authentication.SslProtocols)protocolVersion).GetProtocolWarning();
string warningMessage = protocol.GetProtocolWarning();
if (!string.IsNullOrEmpty(warningMessage))
{
if (!encrypt && LocalAppContextSwitches.SuppressInsecureTlsWarning)
Expand Down
Loading