Fix TryReadPlpBytes throws ArgumentException (#3470)

Wraith2 · Wraith2 · web-flow · commit 951da4578d54 · 2025-07-14T16:06:26.000-07:00
* add clearing of data sizes when a multi-part result is returned

* remove asserts detecting overwrite of snapshot storage which are now no longer accurate

* remove empty statement block

---------

Co-authored-by: Wraith2 &lt;Wraith2@gmaill.com&gt;
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
@@ -13405,6 +13405,13 @@ bool writeDataSizeToSnapshot
                     break;
                 }
             }
+
+            if (writeDataSizeToSnapshot)
+            {
+                stateObj.SetSnapshotStorage(null);
+                stateObj.ClearSnapshotDataSize();
+            }
+
             return TdsOperationStatus.Done;
 
             static int IncrementSnapshotDataSize(TdsParserStateObject stateObj, bool resetting, int previousPacketId, int value)
@@ -13424,15 +13431,15 @@ static int IncrementSnapshotDataSize(TdsParserStateObject stateObj, bool resetti
                         current = 0;
                     }
 
-                    stateObj.SetSnapshotDataSize(current + value);
+                    stateObj.AddSnapshotDataSize(current + value);
 
                     // return new packetid so next time we see this packet we know it isn't new
                     return currentPacketId;
                 }
                 else
                 {
                     current = stateObj.GetSnapshotDataSize();
-                    stateObj.SetSnapshotDataSize(current + value);
+                    stateObj.AddSnapshotDataSize(current + value);
                     return previousPacketId;
                 }
             }
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
@@ -13596,6 +13596,13 @@ bool writeDataSizeToSnapshot
                     break;
                 }
             }
+
+            if (writeDataSizeToSnapshot)
+            {
+                stateObj.SetSnapshotStorage(null);
+                stateObj.ClearSnapshotDataSize();
+            }
+
             return TdsOperationStatus.Done;
 
             static int IncrementSnapshotDataSize(TdsParserStateObject stateObj, bool resetting, int previousPacketId, int value)
@@ -13615,15 +13622,15 @@ static int IncrementSnapshotDataSize(TdsParserStateObject stateObj, bool resetti
                         current = 0;
                     }
 
-                    stateObj.SetSnapshotDataSize(current + value);
+                    stateObj.AddSnapshotDataSize(current + value);
 
                     // return new packetid so next time we see this packet we know it isn't new
                     return currentPacketId;
                 }
                 else
                 {
                     current = stateObj.GetSnapshotDataSize();
-                    stateObj.SetSnapshotDataSize(current + value);
+                    stateObj.AddSnapshotDataSize(current + value);
                     return previousPacketId;
                 }
             }
diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
@@ -1512,7 +1512,7 @@ public TdsOperationStatus TryReadByteArray(Span<byte> buff, int len, out int tot
 
                 if (writeDataSizeToSnapshot)
                 {
-                    SetSnapshotDataSize(bytesToRead);
+                    AddSnapshotDataSize(bytesToRead);
                 }
 
                 AssertValidState();
@@ -2058,7 +2058,7 @@ internal TdsOperationStatus TryReadPlpLength(bool returnPlpNullIfNull, out ulong
             // bool firstchunk = false;
             bool isNull = false;
 
-            Debug.Assert(_longlenleft == 0, "Out of synch length read request");
+            Debug.Assert(_longlenleft == 0, "Out of sync length read request");
             if (_longlen == 0)
             {
                 // First chunk is being read. Find out what type of chunk it is
@@ -2139,6 +2139,7 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len
             }
             return TryReadPlpBytes(ref buff, offset, len, out totalBytesRead, canContinue, canContinue, compatibilityMode);
         }
+
         // Reads the requested number of bytes from a plp data stream, or the entire data if
         // requested length is -1 or larger than the actual length of data. First call to this method
         //  should be preceeded by a call to ReadPlpLength or ReadDataLength.
@@ -2254,14 +2255,14 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len
                         SetSnapshotStorage(buff);
                         if (writeDataSizeToSnapshot)
                         {
-                            SetSnapshotDataSize(bytesRead);
+                            AddSnapshotDataSize(bytesRead);
                         }
                     }
                     return result;
                 }
                 if (writeDataSizeToSnapshot && canContinue)
                 {
-                    SetSnapshotDataSize(bytesRead);
+                    AddSnapshotDataSize(bytesRead);
                 }
 
                 if (_longlenleft == 0)
@@ -2279,10 +2280,7 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len
                         else if (canContinue && result == TdsOperationStatus.NeedMoreData)
                         {
                             SetSnapshotStorage(buff);
-                            if (writeDataSizeToSnapshot)
-                            {
-                                SetSnapshotDataSize(bytesRead);
-                            }
+                            // data bytes read from the current packet must be 0 here so do not save the snapshot data size
                         }
                         return result;
                     }
@@ -2296,6 +2294,12 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len
                     break;
                 }
             }
+
+            if (canContinue)
+            {
+                SetSnapshotStorage(null);
+                ClearSnapshotDataSize();
+            }
             return TdsOperationStatus.Done;
         }
 
@@ -3531,9 +3535,6 @@ internal void ResetSnapshot()
             {
                 StateSnapshot snapshot = _snapshot;
                 _snapshot = null;
-                // TODO(GH-3385): Not sure what this is trying to assert, but it
-                // currently fails the DataReader tests.
-                // Debug.Assert(snapshot._storage == null);
                 snapshot.Clear();
                 Interlocked.CompareExchange(ref _cachedSnapshot, snapshot, null);
             }
@@ -3591,9 +3592,6 @@ internal object TryTakeSnapshotStorage()
         internal void SetSnapshotStorage(object buffer)
         {
             Debug.Assert(_snapshot != null, "should not access snapshot accessor functions without first checking that the snapshot is available");
-            // TODO(GH-3385): Not sure what this is trying to assert, but it
-            // currently fails the DataReader tests.
-            // Debug.Assert(_snapshot._storage == null, "should not overwrite snapshot stored buffer");
             if (_snapshot != null)
             {
                 _snapshot._storage = buffer;
@@ -3605,12 +3603,18 @@ internal void SetSnapshotStorage(object buffer)
         /// countOfBytesCopiedFromCurrentPacket to be calculated
         /// </summary>
         /// <param name="countOfBytesCopiedFromCurrentPacket"></param>
-        internal void SetSnapshotDataSize(int countOfBytesCopiedFromCurrentPacket)
+        internal void AddSnapshotDataSize(int countOfBytesCopiedFromCurrentPacket)
         {
             Debug.Assert(_snapshot != null && _snapshot.ContinueEnabled, "_snapshot must exist to store packet data size");
             _snapshot.SetPacketDataSize(countOfBytesCopiedFromCurrentPacket);
         }
 
+        internal void ClearSnapshotDataSize()
+        {
+            Debug.Assert(_snapshot != null, "_snapshot must exist to store packet data size");
+            _snapshot?.ClearPacketDataSize();
+        }
+
         internal int GetSnapshotTotalSize()
         {
             Debug.Assert(_snapshot != null && _snapshot.ContinueEnabled, "_snapshot must exist to read total size");
@@ -4307,6 +4311,16 @@ internal void SetPacketDataSize(int size)
                 target.RunningDataSize = total + size;
             }
 
+            internal void ClearPacketDataSize()
+            {
+                PacketData current = _firstPacket;
+                while (current != null)
+                {
+                    current.RunningDataSize = 0;
+                    current = current.NextPacket;
+                }
+            }
+
             internal int GetPacketDataOffset()
             {
                 int offset = 0;
@@ -4356,9 +4370,6 @@ private void ClearPackets()
 
             private void ClearState()
             {
-                // TODO(GH-3385): Not sure what this is trying to assert, but it
-                // currently fails the DataReader tests.
-                // Debug.Assert(_storage == null);
                 _storage = null;
                 _replayStateData.Clear(_stateObj);
                 _continueStateData?.Clear(_stateObj, trackStack: false);

Original file line number	Diff line number	Diff line change
`@@ -13405,6 +13405,13 @@ bool writeDataSizeToSnapshot`
`13405`	`13405`	`break;`
`13406`	`13406`	`}`
`13407`	`13407`	`}`
	`13408`	`+`
	`13409`	`+ if (writeDataSizeToSnapshot)`
	`13410`	`+ {`
	`13411`	`+ stateObj.SetSnapshotStorage(null);`
	`13412`	`+ stateObj.ClearSnapshotDataSize();`
	`13413`	`+ }`
	`13414`	`+`
`13408`	`13415`	`return TdsOperationStatus.Done;`
`13409`	`13416`
`13410`	`13417`	`static int IncrementSnapshotDataSize(TdsParserStateObject stateObj, bool resetting, int previousPacketId, int value)`
`@@ -13424,15 +13431,15 @@ static int IncrementSnapshotDataSize(TdsParserStateObject stateObj, bool resetti`
`13424`	`13431`	`current = 0;`
`13425`	`13432`	`}`
`13426`	`13433`
`13427`		`- stateObj.SetSnapshotDataSize(current + value);`
	`13434`	`+ stateObj.AddSnapshotDataSize(current + value);`
`13428`	`13435`
`13429`	`13436`	`// return new packetid so next time we see this packet we know it isn't new`
`13430`	`13437`	`return currentPacketId;`
`13431`	`13438`	`}`
`13432`	`13439`	`else`
`13433`	`13440`	`{`
`13434`	`13441`	`current = stateObj.GetSnapshotDataSize();`
`13435`		`- stateObj.SetSnapshotDataSize(current + value);`
	`13442`	`+ stateObj.AddSnapshotDataSize(current + value);`
`13436`	`13443`	`return previousPacketId;`
`13437`	`13444`	`}`
`13438`	`13445`	`}`
Original file line number	Diff line number	Diff line change
`@@ -13596,6 +13596,13 @@ bool writeDataSizeToSnapshot`
`13596`	`13596`	`break;`
`13597`	`13597`	`}`
`13598`	`13598`	`}`
	`13599`	`+`
	`13600`	`+ if (writeDataSizeToSnapshot)`
	`13601`	`+ {`
	`13602`	`+ stateObj.SetSnapshotStorage(null);`
	`13603`	`+ stateObj.ClearSnapshotDataSize();`
	`13604`	`+ }`
	`13605`	`+`
`13599`	`13606`	`return TdsOperationStatus.Done;`
`13600`	`13607`
`13601`	`13608`	`static int IncrementSnapshotDataSize(TdsParserStateObject stateObj, bool resetting, int previousPacketId, int value)`
`@@ -13615,15 +13622,15 @@ static int IncrementSnapshotDataSize(TdsParserStateObject stateObj, bool resetti`
`13615`	`13622`	`current = 0;`
`13616`	`13623`	`}`
`13617`	`13624`
`13618`		`- stateObj.SetSnapshotDataSize(current + value);`
	`13625`	`+ stateObj.AddSnapshotDataSize(current + value);`
`13619`	`13626`
`13620`	`13627`	`// return new packetid so next time we see this packet we know it isn't new`
`13621`	`13628`	`return currentPacketId;`
`13622`	`13629`	`}`
`13623`	`13630`	`else`
`13624`	`13631`	`{`
`13625`	`13632`	`current = stateObj.GetSnapshotDataSize();`
`13626`		`- stateObj.SetSnapshotDataSize(current + value);`
	`13633`	`+ stateObj.AddSnapshotDataSize(current + value);`
`13627`	`13634`	`return previousPacketId;`
`13628`	`13635`	`}`
`13629`	`13636`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1512,7 +1512,7 @@ public TdsOperationStatus TryReadByteArray(Span<byte> buff, int len, out int tot`
`1512`	`1512`
`1513`	`1513`	`if (writeDataSizeToSnapshot)`
`1514`	`1514`	`{`
`1515`		`- SetSnapshotDataSize(bytesToRead);`
	`1515`	`+ AddSnapshotDataSize(bytesToRead);`
`1516`	`1516`	`}`
`1517`	`1517`
`1518`	`1518`	`AssertValidState();`
`@@ -2058,7 +2058,7 @@ internal TdsOperationStatus TryReadPlpLength(bool returnPlpNullIfNull, out ulong`
`2058`	`2058`	`// bool firstchunk = false;`
`2059`	`2059`	`bool isNull = false;`
`2060`	`2060`
`2061`		`- Debug.Assert(_longlenleft == 0, "Out of synch length read request");`
	`2061`	`+ Debug.Assert(_longlenleft == 0, "Out of sync length read request");`
`2062`	`2062`	`if (_longlen == 0)`
`2063`	`2063`	`{`
`2064`	`2064`	`// First chunk is being read. Find out what type of chunk it is`
`@@ -2139,6 +2139,7 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len`
`2139`	`2139`	`}`
`2140`	`2140`	`return TryReadPlpBytes(ref buff, offset, len, out totalBytesRead, canContinue, canContinue, compatibilityMode);`
`2141`	`2141`	`}`
	`2142`	`+`
`2142`	`2143`	`// Reads the requested number of bytes from a plp data stream, or the entire data if`
`2143`	`2144`	`// requested length is -1 or larger than the actual length of data. First call to this method`
`2144`	`2145`	`// should be preceeded by a call to ReadPlpLength or ReadDataLength.`
`@@ -2254,14 +2255,14 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len`
`2254`	`2255`	`SetSnapshotStorage(buff);`
`2255`	`2256`	`if (writeDataSizeToSnapshot)`
`2256`	`2257`	`{`
`2257`		`- SetSnapshotDataSize(bytesRead);`
	`2258`	`+ AddSnapshotDataSize(bytesRead);`
`2258`	`2259`	`}`
`2259`	`2260`	`}`
`2260`	`2261`	`return result;`
`2261`	`2262`	`}`
`2262`	`2263`	`if (writeDataSizeToSnapshot && canContinue)`
`2263`	`2264`	`{`
`2264`		`- SetSnapshotDataSize(bytesRead);`
	`2265`	`+ AddSnapshotDataSize(bytesRead);`
`2265`	`2266`	`}`
`2266`	`2267`
`2267`	`2268`	`if (_longlenleft == 0)`
`@@ -2279,10 +2280,7 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len`
`2279`	`2280`	`else if (canContinue && result == TdsOperationStatus.NeedMoreData)`
`2280`	`2281`	`{`
`2281`	`2282`	`SetSnapshotStorage(buff);`
`2282`		`- if (writeDataSizeToSnapshot)`
`2283`		`- {`
`2284`		`- SetSnapshotDataSize(bytesRead);`
`2285`		`- }`
	`2283`	`+ // data bytes read from the current packet must be 0 here so do not save the snapshot data size`
`2286`	`2284`	`}`
`2287`	`2285`	`return result;`
`2288`	`2286`	`}`
`@@ -2296,6 +2294,12 @@ internal TdsOperationStatus TryReadPlpBytes(ref byte[] buff, int offset, int len`
`2296`	`2294`	`break;`
`2297`	`2295`	`}`
`2298`	`2296`	`}`
	`2297`	`+`
	`2298`	`+ if (canContinue)`
	`2299`	`+ {`
	`2300`	`+ SetSnapshotStorage(null);`
	`2301`	`+ ClearSnapshotDataSize();`
	`2302`	`+ }`
`2299`	`2303`	`return TdsOperationStatus.Done;`
`2300`	`2304`	`}`
`2301`	`2305`
`@@ -3531,9 +3535,6 @@ internal void ResetSnapshot()`
`3531`	`3535`	`{`
`3532`	`3536`	`StateSnapshot snapshot = _snapshot;`
`3533`	`3537`	`_snapshot = null;`
`3534`		`- // TODO(GH-3385): Not sure what this is trying to assert, but it`
`3535`		`- // currently fails the DataReader tests.`
`3536`		`- // Debug.Assert(snapshot._storage == null);`
`3537`	`3538`	`snapshot.Clear();`
`3538`	`3539`	`Interlocked.CompareExchange(ref _cachedSnapshot, snapshot, null);`
`3539`	`3540`	`}`
`@@ -3591,9 +3592,6 @@ internal object TryTakeSnapshotStorage()`
`3591`	`3592`	`internal void SetSnapshotStorage(object buffer)`
`3592`	`3593`	`{`
`3593`	`3594`	`Debug.Assert(_snapshot != null, "should not access snapshot accessor functions without first checking that the snapshot is available");`
`3594`		`- // TODO(GH-3385): Not sure what this is trying to assert, but it`
`3595`		`- // currently fails the DataReader tests.`
`3596`		`- // Debug.Assert(_snapshot._storage == null, "should not overwrite snapshot stored buffer");`
`3597`	`3595`	`if (_snapshot != null)`
`3598`	`3596`	`{`
`3599`	`3597`	`_snapshot._storage = buffer;`
`@@ -3605,12 +3603,18 @@ internal void SetSnapshotStorage(object buffer)`
`3605`	`3603`	`/// countOfBytesCopiedFromCurrentPacket to be calculated`
`3606`	`3604`	`/// </summary>`
`3607`	`3605`	`/// <param name="countOfBytesCopiedFromCurrentPacket"></param>`
`3608`		`- internal void SetSnapshotDataSize(int countOfBytesCopiedFromCurrentPacket)`
	`3606`	`+ internal void AddSnapshotDataSize(int countOfBytesCopiedFromCurrentPacket)`
`3609`	`3607`	`{`
`3610`	`3608`	`Debug.Assert(_snapshot != null && _snapshot.ContinueEnabled, "_snapshot must exist to store packet data size");`
`3611`	`3609`	`_snapshot.SetPacketDataSize(countOfBytesCopiedFromCurrentPacket);`
`3612`	`3610`	`}`
`3613`	`3611`
	`3612`	`+ internal void ClearSnapshotDataSize()`
	`3613`	`+ {`
	`3614`	`+ Debug.Assert(_snapshot != null, "_snapshot must exist to store packet data size");`
	`3615`	`+ _snapshot?.ClearPacketDataSize();`
	`3616`	`+ }`
	`3617`	`+`
`3614`	`3618`	`internal int GetSnapshotTotalSize()`
`3615`	`3619`	`{`
`3616`	`3620`	`Debug.Assert(_snapshot != null && _snapshot.ContinueEnabled, "_snapshot must exist to read total size");`
`@@ -4307,6 +4311,16 @@ internal void SetPacketDataSize(int size)`
`4307`	`4311`	`target.RunningDataSize = total + size;`
`4308`	`4312`	`}`
`4309`	`4313`
	`4314`	`+ internal void ClearPacketDataSize()`
	`4315`	`+ {`
	`4316`	`+ PacketData current = _firstPacket;`
	`4317`	`+ while (current != null)`
	`4318`	`+ {`
	`4319`	`+ current.RunningDataSize = 0;`
	`4320`	`+ current = current.NextPacket;`
	`4321`	`+ }`
	`4322`	`+ }`
	`4323`	`+`
`4310`	`4324`	`internal int GetPacketDataOffset()`
`4311`	`4325`	`{`
`4312`	`4326`	`int offset = 0;`
`@@ -4356,9 +4370,6 @@ private void ClearPackets()`
`4356`	`4370`
`4357`	`4371`	`private void ClearState()`
`4358`	`4372`	`{`
`4359`		`- // TODO(GH-3385): Not sure what this is trying to assert, but it`
`4360`		`- // currently fails the DataReader tests.`
`4361`		`- // Debug.Assert(_storage == null);`
`4362`	`4373`	`_storage = null;`
`4363`	`4374`	`_replayStateData.Clear(_stateObj);`
`4364`	`4375`	`_continueStateData?.Clear(_stateObj, trackStack: false);`