Skip to content

Commit 8d5e4f2

Browse files
Wraith2Wraith2
authored
Add partial packet detection and fixup (#2714)
1 parent 17cb0b0 commit 8d5e4f2

File tree

17 files changed

+2106
-282
lines changed

17 files changed

+2106
-282
lines changed

‎src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,9 @@
188188
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\OnChangedEventHandler.cs">
189189
<Link>Microsoft\Data\SqlClient\OnChangedEventHandler.cs</Link>
190190
</Compile>
191+
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\Packet.cs">
192+
<Link>Microsoft\Data\SqlClient\Packet.cs</Link>
193+
</Compile>
191194
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\ParameterPeekAheadValue.cs">
192195
<Link>Microsoft\Data\SqlClient\ParameterPeekAheadValue.cs</Link>
193196
</Compile>
@@ -584,6 +587,9 @@
584587
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\TdsParserStateObject.cs">
585588
<Link>Microsoft\Data\SqlClient\TdsParserStateObject.cs</Link>
586589
</Compile>
590+
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\TdsParserStateObject.Multiplexer.cs">
591+
<Link>Microsoft\Data\SqlClient\TdsParserStateObject.Multiplexer.cs</Link>
592+
</Compile>
587593
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\TdsParserStaticMethods.cs">
588594
<Link>Microsoft\Data\SqlClient\TdsParserStaticMethods.cs</Link>
589595
</Compile>

‎src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlDataReader.cs‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3724,7 +3724,7 @@ private TdsOperationStatus TryNextResult(out bool more)
37243724

37253725
/// <include file='../../../../../../../doc/snippets/Microsoft.Data.SqlClient/SqlDataReader.xml' path='docs/members[@name="SqlDataReader"]/Read/*' />
37263726
// user must call Read() to position on the first row
3727-
override public bool Read()
3727+
public override bool Read()
37283728
{
37293729
if (_currentTask != null)
37303730
{
@@ -4564,9 +4564,10 @@ internal TdsOperationStatus TrySetMetaData(_SqlMetaDataSet metaData, bool moreIn
45644564
_metaDataConsumed = true;
45654565

45664566
if (_parser != null)
4567-
{ // There is a valid case where parser is null
4568-
// Peek, and if row token present, set _hasRows true since there is a
4569-
// row in the result
4567+
{
4568+
// There is a valid case where parser is null
4569+
// Peek, and if row token present, set _hasRows true since there is a
4570+
// row in the result
45704571
byte b;
45714572
TdsOperationStatus result = _stateObj.TryPeekByte(out b);
45724573
if (result != TdsOperationStatus.Done)

‎src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.Windows.cs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,10 @@ internal void PostReadAsyncForMars()
2525

2626
_pMarsPhysicalConObj.IncrementPendingCallbacks();
2727
SessionHandle handle = _pMarsPhysicalConObj.SessionHandle;
28+
// we do not need to consider partial packets when making this read because we
29+
// expect this read to pend. a partial packet should not exist at setup of the
30+
// parser
31+
Debug.Assert(_physicalStateObj.PartialPacket==null);
2832
temp = _pMarsPhysicalConObj.ReadAsync(handle, out error);
2933

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

‎src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2047,11 +2047,19 @@ internal TdsOperationStatus TryRun(RunBehavior runBehavior, SqlCommand cmdHandle
20472047

20482048
if (!IsValidTdsToken(token))
20492049
{
2050-
Debug.Fail($"unexpected token; token = {token,-2:X2}");
2050+
#if DEBUG
2051+
string message = stateObj.DumpBuffer();
2052+
Debug.Fail(message);
2053+
#endif
20512054
_state = TdsParserState.Broken;
20522055
_connHandler.BreakConnection();
20532056
SqlClientEventSource.Log.TryTraceEvent("<sc.TdsParser.Run|ERR> Potential multi-threaded misuse of connection, unexpected TDS token found {0}", ObjectID);
2057+
#if DEBUG
2058+
throw new InvalidOperationException(message);
2059+
#else
20542060
throw SQL.ParsingError();
2061+
#endif
2062+
20552063
}
20562064

20572065
int tokenLength;
@@ -4143,6 +4151,7 @@ internal TdsOperationStatus TryProcessReturnValue(int length, TdsParserStateObje
41434151
{
41444152
return result;
41454153
}
4154+
41464155
byte len;
41474156
result = stateObj.TryReadByte(out len);
41484157
if (result != TdsOperationStatus.Done)
@@ -4540,7 +4549,6 @@ internal TdsOperationStatus TryProcessCollation(TdsParserStateObject stateObj, o
45404549
collation = null;
45414550
return result;
45424551
}
4543-
45444552
if (SqlCollation.Equals(_cachedCollation, info, sortId))
45454553
{
45464554
collation = _cachedCollation;
@@ -5263,7 +5271,7 @@ private TdsOperationStatus TryCommonProcessMetaData(TdsParserStateObject stateOb
52635271
{
52645272
// If the column is encrypted, we should have a valid cipherTable
52655273
if (cipherTable != null)
5266-
{
5274+
{
52675275
result = TryProcessTceCryptoMetadata(stateObj, col, cipherTable, columnEncryptionSetting, isReturnValue: false);
52685276
if (result != TdsOperationStatus.Done)
52695277
{

‎src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.netcore.cs‎

Lines changed: 17 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -322,14 +322,22 @@ private void ReadSniError(TdsParserStateObject stateObj, uint error)
322322
stateObj.SendAttention(mustTakeWriteLock: true);
323323

324324
PacketHandle syncReadPacket = default;
325+
bool readFromNetwork = true;
325326
RuntimeHelpers.PrepareConstrainedRegions();
326327
bool shouldDecrement = false;
327328
try
328329
{
329330
Interlocked.Increment(ref _readingCount);
330331
shouldDecrement = true;
331-
332-
syncReadPacket = ReadSyncOverAsync(stateObj.GetTimeoutRemaining(), out error);
332+
readFromNetwork = !PartialPacketContainsCompletePacket();
333+
if (readFromNetwork)
334+
{
335+
syncReadPacket = ReadSyncOverAsync(stateObj.GetTimeoutRemaining(), out error);
336+
}
337+
else
338+
{
339+
error = TdsEnums.SNI_SUCCESS;
340+
}
333341

334342
Interlocked.Decrement(ref _readingCount);
335343
shouldDecrement = false;
@@ -342,7 +350,7 @@ private void ReadSniError(TdsParserStateObject stateObj, uint error)
342350
}
343351
else
344352
{
345-
Debug.Assert(!IsValidPacket(syncReadPacket), "unexpected syncReadPacket without corresponding SNIPacketRelease");
353+
Debug.Assert(!readFromNetwork || !IsValidPacket(syncReadPacket), "unexpected syncReadPacket without corresponding SNIPacketRelease");
346354
fail = true; // Subsequent read failed, time to give up.
347355
}
348356
}
@@ -353,7 +361,7 @@ private void ReadSniError(TdsParserStateObject stateObj, uint error)
353361
Interlocked.Decrement(ref _readingCount);
354362
}
355363

356-
if (!IsPacketEmpty(syncReadPacket))
364+
if (readFromNetwork && !IsPacketEmpty(syncReadPacket))
357365
{
358366
ReleasePacket(syncReadPacket);
359367
}
@@ -393,60 +401,9 @@ private void ReadSniError(TdsParserStateObject stateObj, uint error)
393401
AssertValidState();
394402
}
395403

396-
public void ProcessSniPacket(PacketHandle packet, uint error)
404+
private uint GetSniPacket(PacketHandle packet, ref uint dataSize)
397405
{
398-
if (error != 0)
399-
{
400-
if ((_parser.State == TdsParserState.Closed) || (_parser.State == TdsParserState.Broken))
401-
{
402-
// Do nothing with callback if closed or broken and error not 0 - callback can occur
403-
// after connection has been closed. PROBLEM IN NETLIB - DESIGN FLAW.
404-
return;
405-
}
406-
407-
AddError(_parser.ProcessSNIError(this));
408-
AssertValidState();
409-
}
410-
else
411-
{
412-
uint dataSize = 0;
413-
414-
uint getDataError = SNIPacketGetData(packet, _inBuff, ref dataSize);
415-
416-
if (getDataError == TdsEnums.SNI_SUCCESS)
417-
{
418-
if (_inBuff.Length < dataSize)
419-
{
420-
Debug.Assert(true, "Unexpected dataSize on Read");
421-
throw SQL.InvalidInternalPacketSize(StringsHelper.GetString(Strings.SqlMisc_InvalidArraySizeMessage));
422-
}
423-
424-
_lastSuccessfulIOTimer._value = DateTime.UtcNow.Ticks;
425-
_inBytesRead = (int)dataSize;
426-
_inBytesUsed = 0;
427-
428-
if (_snapshot != null)
429-
{
430-
_snapshot.AppendPacketData(_inBuff, _inBytesRead);
431-
if (_snapshotReplay)
432-
{
433-
_snapshot.MoveNext();
434-
#if DEBUG
435-
_snapshot.AssertCurrent();
436-
#endif
437-
}
438-
}
439-
440-
SniReadStatisticsAndTracing();
441-
SqlClientEventSource.Log.TryAdvancedTraceBinEvent("TdsParser.ReadNetworkPacketAsyncCallback | INFO | ADV | State Object Id {0}, Packet read. In Buffer: {1}, In Bytes Read: {2}", ObjectID, _inBuff, _inBytesRead);
442-
443-
AssertValidState();
444-
}
445-
else
446-
{
447-
throw SQL.ParsingError(ParsingErrorState.ProcessSniPacketFailed);
448-
}
449-
}
406+
return SNIPacketGetData(packet, _inBuff, ref dataSize);
450407
}
451408

452409
private void ChangeNetworkPacketTimeout(int dueTime, int period)
@@ -535,7 +492,7 @@ public void ReadAsyncCallback(IntPtr key, PacketHandle packet, uint error)
535492
bool processFinallyBlock = true;
536493
try
537494
{
538-
Debug.Assert(CheckPacket(packet, source) && source != null, "AsyncResult null on callback");
495+
Debug.Assert((packet.Type == 0 && PartialPacketContainsCompletePacket()) || (CheckPacket(packet, source) && source != null), "AsyncResult null on callback");
539496

540497
if (_parser.MARSOn)
541498
{
@@ -547,7 +504,7 @@ public void ReadAsyncCallback(IntPtr key, PacketHandle packet, uint error)
547504

548505
// The timer thread may be unreliable under high contention scenarios. It cannot be
549506
// assumed that the timeout has happened on the timer thread callback. Check the timeout
550-
// synchrnously and then call OnTimeoutSync to force an atomic change of state.
507+
// synchronously and then call OnTimeoutSync to force an atomic change of state.
551508
if (TimeoutHasExpired)
552509
{
553510
OnTimeoutSync(asyncClose: true);
@@ -1633,7 +1590,7 @@ internal void AssertStateIsClean()
16331590
if ((parser != null) && (parser.State != TdsParserState.Closed) && (parser.State != TdsParserState.Broken))
16341591
{
16351592
// Async reads
1636-
Debug.Assert(_snapshot == null && !_snapshotReplay, "StateObj has leftover snapshot state");
1593+
Debug.Assert(_snapshot == null && _snapshotStatus == SnapshotStatus.NotActive, "StateObj has leftover snapshot state");
16371594
Debug.Assert(!_asyncReadWithoutSnapshot, "StateObj has AsyncReadWithoutSnapshot still enabled");
16381595
Debug.Assert(_executionContext == null, "StateObj has a stored execution context from an async read");
16391596
// Async writes

‎src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -391,6 +391,9 @@
391391
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\OnChangedEventHandler.cs">
392392
<Link>Microsoft\Data\SqlClient\OnChangedEventHandler.cs</Link>
393393
</Compile>
394+
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\Packet.cs">
395+
<Link>Microsoft\Data\SqlClient\Packet.cs</Link>
396+
</Compile>
394397
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\ParameterPeekAheadValue.cs">
395398
<Link>Microsoft\Data\SqlClient\ParameterPeekAheadValue.cs</Link>
396399
</Compile>
@@ -775,6 +778,9 @@
775778
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\TdsParserStateObject.cs">
776779
<Link>Microsoft\Data\SqlClient\TdsParserStateObject.cs</Link>
777780
</Compile>
781+
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\TdsParserStateObject.Multiplexer.cs">
782+
<Link>Microsoft\Data\SqlClient\TdsParserStateObject.Multiplexer.cs</Link>
783+
</Compile>
778784
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\TdsParserStaticMethods.cs">
779785
<Link>Microsoft\Data\SqlClient\TdsParserStaticMethods.cs</Link>
780786
</Compile>

‎src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4364,6 +4364,7 @@ internal TdsOperationStatus TryProcessReturnValue(int length,
43644364
return result;
43654365
}
43664366
}
4367+
43674368
byte len;
43684369
result = stateObj.TryReadByte(out len);
43694370
if (result != TdsOperationStatus.Done)

‎src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.netfx.cs‎

Lines changed: 15 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -453,14 +453,22 @@ private void ReadSniError(TdsParserStateObject stateObj, uint error)
453453
stateObj.SendAttention(mustTakeWriteLock: true);
454454

455455
PacketHandle syncReadPacket = default;
456+
bool readFromNetwork = true;
456457
RuntimeHelpers.PrepareConstrainedRegions();
457458
bool shouldDecrement = false;
458459
try
459460
{
460461
Interlocked.Increment(ref _readingCount);
461462
shouldDecrement = true;
462-
463-
syncReadPacket = ReadSyncOverAsync(stateObj.GetTimeoutRemaining(), out error);
463+
readFromNetwork = !PartialPacketContainsCompletePacket();
464+
if (readFromNetwork)
465+
{
466+
syncReadPacket = ReadSyncOverAsync(stateObj.GetTimeoutRemaining(), out error);
467+
}
468+
else
469+
{
470+
error = TdsEnums.SNI_SUCCESS;
471+
}
464472

465473
Interlocked.Decrement(ref _readingCount);
466474
shouldDecrement = false;
@@ -473,7 +481,7 @@ private void ReadSniError(TdsParserStateObject stateObj, uint error)
473481
}
474482
else
475483
{
476-
Debug.Assert(!IsValidPacket(syncReadPacket), "unexpected syncReadPacket without corresponding SNIPacketRelease");
484+
Debug.Assert(!readFromNetwork || !IsValidPacket(syncReadPacket), "unexpected syncReadPacket without corresponding SNIPacketRelease");
477485
fail = true; // Subsequent read failed, time to give up.
478486
}
479487
}
@@ -484,7 +492,7 @@ private void ReadSniError(TdsParserStateObject stateObj, uint error)
484492
Interlocked.Decrement(ref _readingCount);
485493
}
486494

487-
if (!IsPacketEmpty(syncReadPacket))
495+
if (readFromNetwork && !IsPacketEmpty(syncReadPacket))
488496
{
489497
// Be sure to release packet, otherwise it will be leaked by native.
490498
ReleasePacket(syncReadPacket);
@@ -525,60 +533,9 @@ private void ReadSniError(TdsParserStateObject stateObj, uint error)
525533
AssertValidState();
526534
}
527535

528-
public void ProcessSniPacket(PacketHandle packet, uint error)
536+
private uint GetSniPacket(PacketHandle packet, ref uint dataSize)
529537
{
530-
if (error != 0)
531-
{
532-
if ((_parser.State == TdsParserState.Closed) || (_parser.State == TdsParserState.Broken))
533-
{
534-
// Do nothing with callback if closed or broken and error not 0 - callback can occur
535-
// after connection has been closed. PROBLEM IN NETLIB - DESIGN FLAW.
536-
return;
537-
}
538-
539-
AddError(_parser.ProcessSNIError(this));
540-
AssertValidState();
541-
}
542-
else
543-
{
544-
uint dataSize = 0;
545-
546-
uint getDataError = SniNativeWrapper.SNIPacketGetData(packet, _inBuff, ref dataSize);
547-
548-
if (getDataError == TdsEnums.SNI_SUCCESS)
549-
{
550-
if (_inBuff.Length < dataSize)
551-
{
552-
Debug.Assert(true, "Unexpected dataSize on Read");
553-
throw SQL.InvalidInternalPacketSize(StringsHelper.GetString(Strings.SqlMisc_InvalidArraySizeMessage));
554-
}
555-
556-
_lastSuccessfulIOTimer._value = DateTime.UtcNow.Ticks;
557-
_inBytesRead = (int)dataSize;
558-
_inBytesUsed = 0;
559-
560-
if (_snapshot != null)
561-
{
562-
_snapshot.AppendPacketData(_inBuff, _inBytesRead);
563-
if (_snapshotReplay)
564-
{
565-
_snapshot.MoveNext();
566-
#if DEBUG
567-
_snapshot.AssertCurrent();
568-
#endif
569-
}
570-
}
571-
572-
SniReadStatisticsAndTracing();
573-
SqlClientEventSource.Log.TryAdvancedTraceBinEvent("TdsParser.ReadNetworkPacketAsyncCallback | INFO | ADV | State Object Id {0}, Packet read. In Buffer: {1}, In Bytes Read: {2}", ObjectID, _inBuff, _inBytesRead);
574-
575-
AssertValidState();
576-
}
577-
else
578-
{
579-
throw SQL.ParsingError(ParsingErrorState.ProcessSniPacketFailed);
580-
}
581-
}
538+
return SniNativeWrapper.SNIPacketGetData(packet, _inBuff, ref dataSize);
582539
}
583540

584541
private void ChangeNetworkPacketTimeout(int dueTime, int period)
@@ -1774,7 +1731,7 @@ internal void AssertStateIsClean()
17741731
if ((parser != null) && (parser.State != TdsParserState.Closed) && (parser.State != TdsParserState.Broken))
17751732
{
17761733
// Async reads
1777-
Debug.Assert(_snapshot == null && !_snapshotReplay, "StateObj has leftover snapshot state");
1734+
Debug.Assert(_snapshot == null && _snapshotStatus == SnapshotStatus.NotActive, "StateObj has leftover snapshot state");
17781735
Debug.Assert(!_asyncReadWithoutSnapshot, "StateObj has AsyncReadWithoutSnapshot still enabled");
17791736
Debug.Assert(_executionContext == null, "StateObj has a stored execution context from an async read");
17801737
// Async writes

0 commit comments

Comments
 (0)