From 5493c00cbc7124e8252490e5f1e097a32559e3e2 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Thu, 31 Jul 2025 18:53:46 +0100 Subject: [PATCH 01/16] remove spare packet --- .../Data/SqlClient/TdsParserStateObject.cs | 30 ++----------------- 1 file changed, 2 insertions(+), 28 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs index 2d1810abfb..8d7e79296c 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs @@ -3773,21 +3773,6 @@ internal int GetPacketDataSize() return Math.Max(RunningDataSize - previous, 0); } - internal void Clear() - { - Buffer = null; - Read = 0; - NextPacket = null; - if (PrevPacket != null) - { - PrevPacket.NextPacket = null; - PrevPacket = null; - } - SetDebugStackImpl(null); - SetDebugPacketId(0); - SetDebugDataHash(); - } - internal void SetDebugStack(string value) => SetDebugStackImpl(value); internal void SetDebugPacketId(int value) => SetDebugPacketIdImpl(value); internal void SetDebugDataHash() => SetDebugDataHashImpl(); @@ -4084,7 +4069,6 @@ internal void Restore(TdsParserStateObject stateObj) private PacketData _firstPacket; private PacketData _current; private PacketData _continuePacket; - private PacketData _sparePacket; #if DEBUG private int _packetCounter; @@ -4165,15 +4149,8 @@ internal void AppendPacketData(byte[] buffer, int read) } } #endif - PacketData packetData = _sparePacket; - if (packetData is null) - { - packetData = new PacketData(); - } - else - { - _sparePacket = null; - } + PacketData packetData = new PacketData(); + packetData.Buffer = buffer; packetData.Read = read; #if DEBUG @@ -4357,13 +4334,10 @@ internal void Clear() private void ClearPackets() { - PacketData packet = _firstPacket; _firstPacket = null; _lastPacket = null; _continuePacket = null; _current = null; - packet.Clear(); - _sparePacket = packet; } private void ClearState() From 1b9562764b122b87f583b91915f4be777a0306d3 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Thu, 31 Jul 2025 21:31:37 +0100 Subject: [PATCH 02/16] fix read error with reused uncleared packet fix 0 length read at the start of a packe in plp stream returning 0 when continuing handle char array sizing better change existing test to use multiple packet sizes --- .../src/Microsoft/Data/SqlClient/TdsParser.cs | 8 ++++---- .../src/Microsoft/Data/SqlClient/TdsParser.cs | 8 ++++---- .../Data/SqlClient/TdsParserStateObject.cs | 16 ++++++++++------ .../SQL/DataReaderTest/DataReaderTest.cs | 14 ++++++++++---- 4 files changed, 28 insertions(+), 18 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs index 8480409042..809d794355 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs @@ -13206,7 +13206,7 @@ bool writeDataSizeToSnapshot if (stateObj._longlen == 0) { Debug.Assert(stateObj._longlenleft == 0); - totalCharsRead = 0; + totalCharsRead = startOffsetByteCount >> 1; return TdsOperationStatus.Done; // No data } @@ -13226,14 +13226,14 @@ bool writeDataSizeToSnapshot // later needing to repeatedly allocate new target buffers and copy data as we discover new data if (buff == null && stateObj._longlen != TdsEnums.SQL_PLP_UNKNOWNLEN && stateObj._longlen < (int.MaxValue >> 1)) { - if (supportRentedBuff && stateObj._longlen < 1073741824) // 1 Gib + if (supportRentedBuff && stateObj._longlen >> 1 < 1073741824) // 1 Gib { - buff = ArrayPool.Shared.Rent((int)Math.Min((int)stateObj._longlen, len)); + buff = ArrayPool.Shared.Rent((int)Math.Min((int)stateObj._longlen >> 1, len)); rentedBuff = true; } else { - buff = new char[(int)Math.Min((int)stateObj._longlen, len)]; + buff = new char[(int)Math.Min((int)stateObj._longlen >> 1, len)]; rentedBuff = false; } } diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs index 3ad9c84e78..ac6363bc7c 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs @@ -13396,7 +13396,7 @@ bool writeDataSizeToSnapshot if (stateObj._longlen == 0) { Debug.Assert(stateObj._longlenleft == 0); - totalCharsRead = 0; + totalCharsRead = startOffsetByteCount >> 1; return TdsOperationStatus.Done; // No data } @@ -13416,14 +13416,14 @@ bool writeDataSizeToSnapshot // allocate the whole buffer in one shot instead of realloc'ing and copying over each time if (buff == null && stateObj._longlen != TdsEnums.SQL_PLP_UNKNOWNLEN && stateObj._longlen < (int.MaxValue >> 1)) { - if (supportRentedBuff && stateObj._longlen < 1073741824) // 1 Gib + if (supportRentedBuff && stateObj._longlen >> 1 < 1073741824) // 1 Gib { - buff = ArrayPool.Shared.Rent((int)Math.Min((int)stateObj._longlen, len)); + buff = ArrayPool.Shared.Rent((int)Math.Min((int)stateObj._longlen >> 1, len)); rentedBuff = true; } else { - buff = new char[(int)Math.Min((int)stateObj._longlen, len)]; + buff = new char[(int)Math.Min((int)stateObj._longlen >> 1, len)]; rentedBuff = false; } } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs index 8d7e79296c..05124cf826 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs @@ -3741,8 +3741,9 @@ internal sealed partial class StateSnapshot { private sealed partial class PacketData { - public byte[] Buffer; - public int Read; + public readonly byte[] Buffer; + public readonly int Read; + public PacketData NextPacket; public PacketData PrevPacket; @@ -3752,6 +3753,12 @@ private sealed partial class PacketData /// public int RunningDataSize; + public PacketData(byte[] buffer, int read) + { + Buffer = buffer; + Read = read; + } + public int PacketID => Packet.GetIDFromHeader(Buffer.AsSpan(0, TdsEnums.HEADER_LEN)); internal int GetPacketDataOffset() @@ -4149,10 +4156,7 @@ internal void AppendPacketData(byte[] buffer, int read) } } #endif - PacketData packetData = new PacketData(); - - packetData.Buffer = buffer; - packetData.Read = read; + PacketData packetData = new PacketData(buffer, read); #if DEBUG packetData.SetDebugStack(_stateObj._lastStack); packetData.SetDebugPacketId(Interlocked.Increment(ref _packetCounter)); diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs index b00662a90f..e51373d674 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs @@ -1035,11 +1035,18 @@ [Value] [nvarchar](max) NULL } } - int counter = 0; - while (counter < 10) + SqlConnectionStringBuilder builder = new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString); + builder.PersistSecurityInfo = true; + builder.Pooling = false; + + for (int packetSize = 512; packetSize<2048; packetSize++) { - using (var cmd = cn.CreateCommand()) + builder.PacketSize = packetSize; + using (SqlConnection sizedConnection = new SqlConnection(builder.ToString())) + using (SqlCommand cmd = sizedConnection.CreateCommand()) { + sizedConnection.Open(); + cmd.CommandText = $"SELECT [d].[Id], [d].[DocumentIdentificationId], [d].[Name], [d].[Value] FROM [{tableName}] AS [d]"; using (var reader = await cmd.ExecuteReaderAsync()) { @@ -1063,7 +1070,6 @@ [Value] [nvarchar](max) NULL } } } - counter++; } } finally From b2f4c7534c7048da002df96f7f798c143f7c6db8 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Fri, 1 Aug 2025 02:27:38 +0100 Subject: [PATCH 03/16] additinal fix for 0 length case --- .../Data/SqlClient/TdsParserStateObject.cs | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs index 05124cf826..6796ac93a5 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs @@ -1966,7 +1966,8 @@ internal TdsOperationStatus TryReadStringWithEncoding(int length, System.Text.En if (isPlp) { - TdsOperationStatus result = TryReadPlpBytes(ref buf, 0, int.MaxValue, out length); + bool compatibilityMode = LocalAppContextSwitches.UseCompatibilityAsyncBehaviour; + TdsOperationStatus result = TryReadPlpBytes(ref buf, 0, int.MaxValue, out length, canContinue && !compatibilityMode, canContinue && !compatibilityMode, compatibilityMode); if (result != TdsOperationStatus.Done) { value = null; @@ -2128,12 +2129,10 @@ internal int ReadPlpBytesChunk(byte[] buff, int offset, int len) internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len, out int totalBytesRead) { bool canContinue = false; - bool isStarting = false; - bool isContinuing = false; bool compatibilityMode = LocalAppContextSwitches.UseCompatibilityAsyncBehaviour; if (!compatibilityMode) { - (canContinue, isStarting, isContinuing) = GetSnapshotStatuses(); + (canContinue, _, _) = GetSnapshotStatuses(); } return TryReadPlpBytes(ref buff, offset, len, out totalBytesRead, canContinue, canContinue, compatibilityMode); } @@ -2157,7 +2156,16 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len } AssertValidState(); - totalBytesRead = 0; + if (writeDataSizeToSnapshot && canContinue && _snapshot != null) + { + // if there is a snapshot which it contains a stored plp buffer take it + // and try to use it if it is the right length + buff = TryTakeSnapshotStorage() as byte[]; + if (buff != null) + { + totalBytesRead = _snapshot.GetPacketDataOffset(); + } + } return TdsOperationStatus.Done; // No data } From d38da1c717db43987beecf4eefee60c8ffc38cf4 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Tue, 12 Aug 2025 21:31:14 +0100 Subject: [PATCH 04/16] fix pending read counters force process sni compatibility mode by default --- .../Microsoft/Data/SqlClient/LocalAppContextSwitches.cs | 6 +++--- .../src/Microsoft/Data/SqlClient/TdsParserStateObject.cs | 8 +++++++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs index ee5d088dc4..da00ef97cb 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs @@ -78,13 +78,13 @@ public static bool UseCompatibilityProcessSni { if (s_useCompatibilityProcessSni == Tristate.NotInitialized) { - if (AppContext.TryGetSwitch(UseCompatibilityProcessSniString, out bool returnedValue) && returnedValue) + if (AppContext.TryGetSwitch(UseCompatibilityProcessSniString, out bool returnedValue) && !returnedValue) { - s_useCompatibilityProcessSni = Tristate.True; + s_useCompatibilityProcessSni = Tristate.False; } else { - s_useCompatibilityProcessSni = Tristate.False; + s_useCompatibilityProcessSni = Tristate.True; } } return s_useCompatibilityProcessSni == Tristate.True; diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs index 6796ac93a5..5637c44890 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs @@ -3207,7 +3207,13 @@ internal void ReadSni(TaskCompletionSource completion) } } else - { + { + // this call to IncrementPendingCallbacks is required for balance + // the _pendingCallbacks counter will be unconditionally decremented in ReadAsyncCallback + // so we must make sure that even though we are not making a network call that we do + // not cause an incorrect decrement which will cause disconnection from the native + // component + IncrementPendingCallbacks(); readPacket = default; error = TdsEnums.SNI_SUCCESS; } From 507973d289e30e5f8581162406be211815f7107c Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Tue, 12 Aug 2025 22:37:16 +0100 Subject: [PATCH 05/16] align test with switch values --- .../Microsoft/Data/SqlClient/LocalAppContextSwitchesTest.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/LocalAppContextSwitchesTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/LocalAppContextSwitchesTest.cs index ec60f0d3cd..c28fe18978 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/LocalAppContextSwitchesTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/LocalAppContextSwitchesTest.cs @@ -23,8 +23,8 @@ public void TestDefaultAppContextSwitchValues() Assert.False(LocalAppContextSwitches.MakeReadAsyncBlocking); Assert.True(LocalAppContextSwitches.UseMinimumLoginTimeout); Assert.True(LocalAppContextSwitches.LegacyVarTimeZeroScaleBehaviour); - Assert.False(LocalAppContextSwitches.UseCompatibilityProcessSni); - Assert.False(LocalAppContextSwitches.UseCompatibilityAsyncBehaviour); + Assert.True(LocalAppContextSwitches.UseCompatibilityProcessSni); + Assert.True(LocalAppContextSwitches.UseCompatibilityAsyncBehaviour); Assert.False(LocalAppContextSwitches.UseConnectionPoolV2); Assert.False(LocalAppContextSwitches.TruncateScaledDecimal); #if NETFRAMEWORK From 845bfc1689eedd75c6da0685dc0bd486b06d97a1 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Wed, 13 Aug 2025 00:25:10 +0100 Subject: [PATCH 06/16] use division not shift --- .../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs | 2 +- .../netfx/src/Microsoft/Data/SqlClient/TdsParser.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs index 809d794355..f5c145e166 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs @@ -13206,7 +13206,7 @@ bool writeDataSizeToSnapshot if (stateObj._longlen == 0) { Debug.Assert(stateObj._longlenleft == 0); - totalCharsRead = startOffsetByteCount >> 1; + totalCharsRead = startOffsetByteCount / 2; return TdsOperationStatus.Done; // No data } diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs index ac6363bc7c..2cd9d3c03d 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs @@ -13396,7 +13396,7 @@ bool writeDataSizeToSnapshot if (stateObj._longlen == 0) { Debug.Assert(stateObj._longlenleft == 0); - totalCharsRead = startOffsetByteCount >> 1; + totalCharsRead = startOffsetByteCount / 2; return TdsOperationStatus.Done; // No data } From 2ed316a6a259bb4a6266b57a267c23bdf5f438b3 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Wed, 13 Aug 2025 00:30:29 +0100 Subject: [PATCH 07/16] invert multiplexer test condition to match definition in library --- .../tests/FunctionalTests/MultiplexerTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/FunctionalTests/MultiplexerTests.cs b/src/Microsoft.Data.SqlClient/tests/FunctionalTests/MultiplexerTests.cs index 288586fb17..b8454f0bd9 100644 --- a/src/Microsoft.Data.SqlClient/tests/FunctionalTests/MultiplexerTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/FunctionalTests/MultiplexerTests.cs @@ -23,9 +23,9 @@ public static bool IsUsingCompatibilityProcessSni { if (AppContext.TryGetSwitch(@"Switch.Microsoft.Data.SqlClient.UseCompatibilityProcessSni", out bool foundValue)) { - return foundValue; + return !foundValue; } - return false; + return true; } } From 7f51dc4e6bbe47311c3cd4a1affb090a60bc7ae1 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Fri, 15 Aug 2025 01:50:42 +0100 Subject: [PATCH 08/16] Fix cached buffer attempting to use storage when continue is not occuring --- .../src/Microsoft/Data/SqlClient/SqlCachedBuffer.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCachedBuffer.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCachedBuffer.cs index 82ef37fb6b..a03eefce09 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCachedBuffer.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCachedBuffer.cs @@ -39,10 +39,10 @@ internal static TdsOperationStatus TryCreate(SqlMetaDataPriv metadata, TdsParser { buffer = null; - (bool canContinue, bool isStarting, _) = stateObj.GetSnapshotStatuses(); + (bool canContinue, bool isStarting, bool isContinuing) = stateObj.GetSnapshotStatuses(); List cachedBytes = null; - if (canContinue) + if (canContinue && isContinuing) { cachedBytes = stateObj.TryTakeSnapshotStorage() as List; if (isStarting) @@ -81,7 +81,7 @@ internal static TdsOperationStatus TryCreate(SqlMetaDataPriv metadata, TdsParser result = stateObj.TryReadPlpBytes(ref byteArr, 0, cb, out cb, canContinue, writeDataSizeToSnapshot: false, compatibilityMode: false); if (result != TdsOperationStatus.Done) { - if (result == TdsOperationStatus.NeedMoreData && canContinue && cb == byteArr.Length) + if (result == TdsOperationStatus.NeedMoreData && canContinue && cb == byteArr.Length && (isContinuing || !isStarting)) { // succeeded in getting the data but failed to find the next plp length returnAfterAdd = true; From 47f223ba3e2f906e2f4d43fa05058881f0b202a8 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Fri, 15 Aug 2025 02:12:10 +0100 Subject: [PATCH 09/16] fix and clarify RequiredLength vs CurrentLength in ProcessSniPacket --- .../Data/SqlClient/TdsParserStateObject.Multiplexer.cs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.Multiplexer.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.Multiplexer.cs index 77bd1c982b..8364666572 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.Multiplexer.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.Multiplexer.cs @@ -95,16 +95,18 @@ out recurse // if a partial packet was reconstructed it must be handled first if (consumePartialPacket) { + // the partial packet has been processed by the multiplexer and should now have + // only data from a single packet in it so we should use RequiredLength which + // is defined by the packet header here not CurrentLength if (_snapshot != null) { - _snapshot.AppendPacketData(PartialPacket.Buffer, PartialPacket.CurrentLength); + _snapshot.AppendPacketData(PartialPacket.Buffer, PartialPacket.RequiredLength); SetBuffer(new byte[_inBuff.Length], 0, 0); appended = true; } else { - SetBuffer(PartialPacket.Buffer, 0, PartialPacket.CurrentLength); - + SetBuffer(PartialPacket.Buffer, 0, PartialPacket.RequiredLength); } bufferIsPartialCompleted = true; ClearPartialPacket(); From 9c224e718372a469d5c3ef59fc7e500af2cf8cb0 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Fri, 15 Aug 2025 02:39:26 +0100 Subject: [PATCH 10/16] sync assertions --- .../src/Microsoft/Data/SqlClient/TdsParserStateObject.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs index 5637c44890..73c606bc9f 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs @@ -4154,8 +4154,8 @@ internal void CloneCleanupAltMetaDataSetArray() internal void AppendPacketData(byte[] buffer, int read) { Debug.Assert(buffer != null, "packet data cannot be null"); - Debug.Assert(read >= TdsEnums.HEADER_LEN, "minimum packet length is TdsEnums.HEADER_LEN"); - Debug.Assert(TdsEnums.HEADER_LEN + Packet.GetDataLengthFromHeader(buffer) == read, "partially read packets cannot be appended to the snapshot"); + Debug.Assert(LocalAppContextSwitches.UseCompatibilityProcessSni || read >= TdsEnums.HEADER_LEN, "minimum packet length is TdsEnums.HEADER_LEN"); + Debug.Assert(LocalAppContextSwitches.UseCompatibilityProcessSni || TdsEnums.HEADER_LEN + Packet.GetDataLengthFromHeader(buffer) == read, "partially read packets cannot be appended to the snapshot"); #if DEBUG for (PacketData current = _firstPacket; current != null; current = current.NextPacket) { From 0d74d998c41ba54e6595d0c20eb0abdcfb3cc785 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Fri, 15 Aug 2025 02:46:19 +0100 Subject: [PATCH 11/16] prevent multiple appends to an open snapshot --- .../Data/SqlClient/TdsParserStateObject.Multiplexer.cs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.Multiplexer.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.Multiplexer.cs index 8364666572..719e078fb7 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.Multiplexer.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.Multiplexer.cs @@ -151,6 +151,15 @@ out recurse if (remainderPacketProduced) { SetPartialPacket(remainderPacket); + if (appended && recurse) + { + // if we've appended to the snapshot already we can't recurse and append to it again because the + // snapshot might be cleared by the async cleanup functions + // the only way to get a recurse output from the multiplexer is if it has produced a remainder packet so + // assert that this is the case and the put the remainder packet in the partial packet so that it + // can be picked up in another call. + recurse = false; + } if (!bufferIsPartialCompleted) { // we are keeping the partial packet buffer so replace it with a new one From b1e5f5c76f911574cbd1d62f3fa9d2c98abe394b Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Sat, 16 Aug 2025 22:03:21 +0100 Subject: [PATCH 12/16] add continue capability back and make it opt-in for supporting callee functions --- .../src/Microsoft/Data/SqlClient/TdsParser.cs | 2 ++ .../TdsParserStateObject.Multiplexer.cs | 19 ++++++---- .../Data/SqlClient/TdsParserStateObject.cs | 36 +++++++++++++++++-- .../TdsParserStateObject.TestHarness.cs | 8 +++++ 4 files changed, 55 insertions(+), 10 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs index f5c145e166..c6e11a313f 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs @@ -13112,6 +13112,8 @@ internal TdsOperationStatus TryReadPlpUnicodeCharsWithContinue(TdsParserStateObj char[] temp = null; bool buffIsRented = false; int startOffset = 0; + + stateObj.RequestContinue(true); (bool canContinue, bool isStarting, bool isContinuing) = stateObj.GetSnapshotStatuses(); if (canContinue) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.Multiplexer.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.Multiplexer.cs index 719e078fb7..9f0627f623 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.Multiplexer.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.Multiplexer.cs @@ -41,6 +41,10 @@ public void ProcessSniPacket(PacketHandle packet, uint error) if (PartialPacketContainsCompletePacket()) { Packet partialPacket = _partialPacket; + // the partial packet can contain more than a single packet worth of data so to consume the + // partial packet we must use the CurrentLength not just the RequiredLength and then later + // the multiplexer will split out the complete packet for consumption and maintain the + // additional data SetBuffer(partialPacket.Buffer, 0, partialPacket.CurrentLength); ClearPartialPacket(); getDataError = TdsEnums.SNI_SUCCESS; @@ -50,7 +54,7 @@ public void ProcessSniPacket(PacketHandle packet, uint error) { if (_inBytesRead != 0) { - SetBuffer(new byte[_inBuff.Length], 0, 0); + NewBuffer(_inBuff.Length); } getDataError = GetSniPacket(packet, ref dataSize); } @@ -76,7 +80,7 @@ public void ProcessSniPacket(PacketHandle packet, uint error) { if (recurse && appended) { - SetBuffer(new byte[_inBuff.Length], 0, 0); + NewBuffer(_inBuff.Length); appended = false; } MultiplexPackets( @@ -98,10 +102,11 @@ out recurse // the partial packet has been processed by the multiplexer and should now have // only data from a single packet in it so we should use RequiredLength which // is defined by the packet header here not CurrentLength + Debug.Assert(PartialPacket != null && PartialPacket.RequiredLength == PartialPacket.CurrentLength); if (_snapshot != null) { _snapshot.AppendPacketData(PartialPacket.Buffer, PartialPacket.RequiredLength); - SetBuffer(new byte[_inBuff.Length], 0, 0); + NewBuffer(_inBuff.Length); appended = true; } else @@ -127,7 +132,7 @@ out recurse // if we SetBuffer here to clear the packet buffer we will break the attention handling which relies // on the attention containing packet remaining in the active buffer even if we're appending to the // snapshot so we will have to use the appended variable to prevent the same buffer being added again - //// SetBuffer(new byte[_inBuff.Length], 0, 0); + //// NewBuffer(_inBuff.Length); appended = true; } else @@ -143,7 +148,7 @@ out recurse // we don't process it if (!bufferIsPartialCompleted) { - SetBuffer(_inBuff, 0, 0); + NewBuffer(_inBuff.Length); } } @@ -164,7 +169,7 @@ out recurse { // we are keeping the partial packet buffer so replace it with a new one // unless we have already set the buffer to the partial packet buffer - SetBuffer(new byte[_inBuff.Length], 0, 0); + NewBuffer(_inBuff.Length); } } @@ -312,7 +317,7 @@ out bool recurse remainderPacket = new Packet { Buffer = new byte[dataBuffer.Length], - CurrentLength = remainderLength, + CurrentLength = remainderLength }; remainderPacket.SetCreatedBy(1); diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs index 73c606bc9f..1322a0b75c 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs @@ -1261,6 +1261,13 @@ internal void SetBuffer(byte[] buffer, int inBytesUsed, int inBytesRead/*, [Call _inBytesRead = inBytesRead; } + internal void NewBuffer(int size) + { + _inBuff = new byte[size]; + _inBytesUsed = 0; + _inBytesRead = 0; + } + // This ensure that there is data available to be read in the buffer and that the header has been processed // NOTE: This method (and all it calls) should be retryable without replaying a snapshot internal TdsOperationStatus TryPrepareBuffer() @@ -1372,7 +1379,7 @@ internal bool SetPacketSize(int size) // Allocate or re-allocate _inBuff. if (_inBuff == null) { - SetBuffer(new byte[size], 0, 0); + NewBuffer(size); } else if (size != _inBuff.Length) { @@ -1399,7 +1406,7 @@ internal bool SetPacketSize(int size) else { // buffer is empty - just create the new one that is double the size of the old one - SetBuffer(new byte[size], 0, 0); + NewBuffer(size); } } @@ -1962,6 +1969,7 @@ internal TdsOperationStatus TryReadStringWithEncoding(int length, System.Text.En } byte[] buf = null; int offset = 0; + RequestContinue(true); (bool canContinue, bool isStarting, bool isContinuing) = GetSnapshotStatuses(); if (isPlp) @@ -3621,6 +3629,9 @@ internal void AddSnapshotDataSize(int countOfBytesCopiedFromCurrentPacket) _snapshot.SetPacketDataSize(countOfBytesCopiedFromCurrentPacket); } + /// + /// clears the stored count of bytes to be copied from the all packets in the snapshot + /// internal void ClearSnapshotDataSize() { Debug.Assert(_snapshot != null, "_snapshot must exist to store packet data size"); @@ -3647,6 +3658,17 @@ internal int GetSnapshotPacketID() return _snapshot.GetPacketID(); } + /// + /// sets a value on the snapshot to allow the ContinueEnabled property to return true.
+ /// this function should be called only by functions that explicitly support the snapshot status + /// status + /// + internal void RequestContinue(bool value) + { + Debug.Assert(_snapshot != null); + _snapshot.RequestContinue(value); + } + /// /// Debug Only: Ensures that the TdsParserStateObject has no lingering state and can safely be re-used /// @@ -4085,6 +4107,7 @@ internal void Restore(TdsParserStateObject stateObj) private StateObjectData _continueStateData; internal object _storage; + internal bool _continueRequested; private PacketData _lastPacket; private PacketData _firstPacket; @@ -4133,7 +4156,7 @@ internal void CheckStack(string trace) } #endif - public bool ContinueEnabled => !LocalAppContextSwitches.UseCompatibilityAsyncBehaviour; + public bool ContinueEnabled => _continueRequested && !LocalAppContextSwitches.UseCompatibilityAsyncBehaviour; internal void CloneNullBitmapInfo() { @@ -4269,6 +4292,7 @@ internal void CaptureAsContinue(TdsParserStateObject stateObj) if (ContinueEnabled) { Debug.Assert(_stateObj == stateObj); + Debug.Assert(_stateObj._bTmpRead == 0); if (_current is not null) { _continueStateData ??= new StateObjectData(); @@ -4278,6 +4302,11 @@ internal void CaptureAsContinue(TdsParserStateObject stateObj) } } + internal void RequestContinue(bool value) + { + _continueRequested = value; + } + internal void SetPacketDataSize(int size) { PacketData target = _current; @@ -4361,6 +4390,7 @@ private void ClearPackets() private void ClearState() { _storage = null; + _continueRequested = false; _replayStateData.Clear(_stateObj); _continueStateData?.Clear(_stateObj, trackStack: false); #if DEBUG diff --git a/src/Microsoft.Data.SqlClient/tests/FunctionalTests/TdsParserStateObject.TestHarness.cs b/src/Microsoft.Data.SqlClient/tests/FunctionalTests/TdsParserStateObject.TestHarness.cs index 89807b0132..6e8f64fcc8 100644 --- a/src/Microsoft.Data.SqlClient/tests/FunctionalTests/TdsParserStateObject.TestHarness.cs +++ b/src/Microsoft.Data.SqlClient/tests/FunctionalTests/TdsParserStateObject.TestHarness.cs @@ -142,6 +142,14 @@ void SetBuffer(byte[] buffer, int inBytesUsed, int inBytesRead) _inBytesRead = inBytesRead; } + [DebuggerStepThrough] + internal void NewBuffer(int size) + { + _inBuff = new byte[size]; + _inBytesUsed = 0; + _inBytesRead = 0; + } + // stubs private LastIOTimer _lastSuccessfulIOTimer = new LastIOTimer(); private Parser _parser = new Parser(); From 89d9a4cc37a480a6bd98f9a5bf2c219d388c6882 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Sat, 16 Aug 2025 22:12:25 +0100 Subject: [PATCH 13/16] reduce memory usage when reading strings in continue mode --- .../Data/SqlClient/TdsParserStateObject.cs | 100 ++++++++++-------- 1 file changed, 55 insertions(+), 45 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs index 1322a0b75c..1f32463139 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs @@ -2185,22 +2185,25 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len // If total length is known up front, allocate the whole buffer in one shot instead of realloc'ing and copying over each time if (buff == null && _longlen != TdsEnums.SQL_PLP_UNKNOWNLEN) { - if (compatibilityMode && _snapshot != null && _snapshotStatus != SnapshotStatus.NotActive) + if (_snapshot != null) { - // legacy replay path perf optimization - // if there is a snapshot which contains a stored plp buffer take it - // and try to use it if it is the right length - buff = TryTakeSnapshotStorage() as byte[]; - } - else if (writeDataSizeToSnapshot && canContinue && _snapshot != null) - { - // if there is a snapshot which it contains a stored plp buffer take it - // and try to use it if it is the right length - buff = TryTakeSnapshotStorage() as byte[]; - if (buff != null) + if (compatibilityMode && _snapshotStatus != SnapshotStatus.NotActive) + { + // legacy replay path perf optimization + // if there is a snapshot which contains a stored plp buffer take it + // and try to use it if it is the right length + buff = TryTakeSnapshotStorage() as byte[]; + } + else { - offset = _snapshot.GetPacketDataOffset(); - totalBytesRead = offset; + // if there is a snapshot which it contains a stored plp buffer take it + // and try to use it if it is the right length + buff = TryTakeSnapshotStorage() as byte[]; + if (buff != null && writeDataSizeToSnapshot && canContinue) + { + offset = _snapshot.GetPacketDataOffset(); + totalBytesRead = offset; + } } } @@ -2215,17 +2218,19 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len if (_longlenleft == 0) { TdsOperationStatus result = TryReadPlpLength(false, out _); - if (result != TdsOperationStatus.Done) + if (result != TdsOperationStatus.Done || _longlenleft == 0) { - totalBytesRead = 0; + if (_snapshot != null) + { + if (writeDataSizeToSnapshot && canContinue && buff != null) + { + totalBytesRead = _snapshot.GetPacketDataOffset(); + ClearSnapshotDataSize(); + } + SetSnapshotStorage(null); + } return result; } - if (_longlenleft == 0) - { - // Data read complete - totalBytesRead = 0; - return TdsOperationStatus.Done; - } } if (buff == null) @@ -2255,21 +2260,24 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len _longlenleft -= (ulong)bytesRead; if (result != TdsOperationStatus.Done) { - if (compatibilityMode && _snapshot != null) + if (_snapshot != null) { - // legacy replay path perf optimization - // a partial read has happened so store the target buffer in the snapshot - // so it can be re-used when another packet arrives and we read again - SetSnapshotStorage(buff); - } - else if (canContinue) - { - // a partial read has happened so store the target buffer in the snapshot - // so it can be re-used when another packet arrives and we read again - SetSnapshotStorage(buff); - if (writeDataSizeToSnapshot) + if (compatibilityMode) + { + // legacy replay path perf optimization + // a partial read has happened so store the target buffer in the snapshot + // so it can be re-used when another packet arrives and we read again + SetSnapshotStorage(buff); + } + else if (canContinue) { - AddSnapshotDataSize(bytesRead); + // a partial read has happened so store the target buffer in the snapshot + // so it can be re-used when another packet arrives and we read again + SetSnapshotStorage(buff); + if (writeDataSizeToSnapshot) + { + AddSnapshotDataSize(bytesRead); + } } } return result; @@ -2285,16 +2293,19 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len result = TryReadPlpLength(false, out _); if (result != TdsOperationStatus.Done) { - if (compatibilityMode && _snapshot != null) + if (_snapshot != null) { - // a partial read has happened so store the target buffer in the snapshot - // so it can be re-used when another packet arrives and we read again - SetSnapshotStorage(buff); - } - else if (canContinue && result == TdsOperationStatus.NeedMoreData) - { - SetSnapshotStorage(buff); - // data bytes read from the current packet must be 0 here so do not save the snapshot data size + if (compatibilityMode) + { + // a partial read has happened so store the target buffer in the snapshot + // so it can be re-used when another packet arrives and we read again + SetSnapshotStorage(buff); + } + else if (result == TdsOperationStatus.NeedMoreData) + { + SetSnapshotStorage(buff); + // data bytes read from the current packet must be 0 here so do not save the snapshot data size + } } return result; } @@ -2316,7 +2327,6 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len } return TdsOperationStatus.Done; } - ///////////////////////////////////////// // Value Skip Logic // ///////////////////////////////////////// From 10f1be9f7b59726cc085341ef7d705f9392d6def Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Sat, 16 Aug 2025 22:20:06 +0100 Subject: [PATCH 14/16] avoid 0 length array allocations --- .../Microsoft/Data/SqlClient/TdsParserStaticMethods.cs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStaticMethods.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStaticMethods.cs index e21bfc7826..569a3fbc16 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStaticMethods.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStaticMethods.cs @@ -100,6 +100,10 @@ static internal void AliasRegistryLookup(ref string host, ref string protocol) // If this logic changed, SNIPacketSetData needs to be changed as well internal static byte[] ObfuscatePassword(string password) { + if (string.IsNullOrEmpty(password)) + { + return Array.Empty(); + } byte[] bObfuscated = new byte[password.Length << 1]; int s; byte bLo; @@ -118,6 +122,10 @@ internal static byte[] ObfuscatePassword(string password) internal static byte[] ObfuscatePassword(byte[] password) { + if (password == null || password.Length == 0) + { + return Array.Empty(); + } byte bLo; byte bHi; From b624a07c5fce270e2b6da0b363a96fc1beefd9af Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Sun, 17 Aug 2025 00:19:54 +0100 Subject: [PATCH 15/16] fix ReqeustContinue to be sync safe --- .../src/Microsoft/Data/SqlClient/TdsParserStateObject.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs index 1f32463139..a24bfefd2a 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs @@ -3675,8 +3675,10 @@ internal int GetSnapshotPacketID() /// internal void RequestContinue(bool value) { - Debug.Assert(_snapshot != null); - _snapshot.RequestContinue(value); + if (_snapshot != null) + { + _snapshot.RequestContinue(value); + } } /// From 59da02b3911d069d508b40c9756fe5cc4c5a1fa6 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Wed, 20 Aug 2025 20:23:37 +0100 Subject: [PATCH 16/16] fix 3527 shotren new test duration by using larger increments --- .../src/Microsoft/Data/SqlClient/SqlDataReader.cs | 8 +++++--- .../ManualTests/SQL/DataReaderTest/DataReaderTest.cs | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs index 926b714462..94371a7021 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs @@ -4596,9 +4596,7 @@ private TdsOperationStatus TryResetBlobState() else { Debug.Assert( - (_sharedState._columnDataBytesRemaining == 0 || _sharedState._columnDataBytesRemaining == -1) - && - (_stateObj._longlen == 0 || _stateObj.IsSnapshotContinuing()), + (_sharedState._columnDataBytesRemaining == 0 || _sharedState._columnDataBytesRemaining == -1), "Haven't read header yet, but column is partially read?" ); } @@ -5753,6 +5751,10 @@ private static Task GetFieldValueAsyncExecute(Task task, object state) { return Task.FromResult(reader.GetFieldValueFromSqlBufferInternal(reader._data[columnIndex], reader._metaData[columnIndex], isAsync: true)); } + else + { + return reader.ExecuteAsyncCall(context); + } } } diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs index e51373d674..60813a5fb3 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs @@ -1039,7 +1039,7 @@ [Value] [nvarchar](max) NULL builder.PersistSecurityInfo = true; builder.Pooling = false; - for (int packetSize = 512; packetSize<2048; packetSize++) + for (int packetSize = 512; packetSize<2048; packetSize+=3) { builder.PacketSize = packetSize; using (SqlConnection sizedConnection = new SqlConnection(builder.ToString()))