Skip to content

Add partial packet detection and fixup #2714

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

Merged
merged 30 commits into from
Feb 11, 2025
Merged

Add partial packet detection and fixup #2714

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
94b6808
reimplementation of experimental branch on main
Wraith2 Nov 4, 2024
53c1139
integrated multiplexer tests and align multiplexer with dev version
Wraith2 Oct 29, 2024
3622f9c
address feedback, minor fixes and tuning
Wraith2 Jul 24, 2024
d0fd956
add Packet comments
Wraith2 Jul 25, 2024
25f0c62
Fix async cancellation and add test coverage for the scenario.
Wraith2 Aug 9, 2024
cc694b9
reduce CancelAsyncConnections sensitivity to match main
Wraith2 Aug 12, 2024
fb9be3b
make multiplexer not require snapshot to consume partial packets
Wraith2 Oct 2, 2024
7f262eb
refine AppendPacketData checks and fix behaviour that was causing it …
Wraith2 Oct 15, 2024
67b7762
add more debugging
Wraith2 Oct 18, 2024
57c8b86
add debug fail to sanity check in multiplexer to make it clear that i…
Wraith2 Oct 23, 2024
b00a379
update multiplexer to deal with multiple sequential packets less than…
Wraith2 Oct 24, 2024
352ac25
change multiplexer to deal with trailing partial packets correctly wh…
Wraith2 Oct 29, 2024
51137f9
fix rebase conflicts
Wraith2 Oct 29, 2024
17ad988
review feedback and misc fix
Wraith2 Oct 29, 2024
eae759a
protect missed locations where network reads can happen
Wraith2 Oct 30, 2024
29ad90b
fix CheckPacket assertion
Wraith2 Oct 31, 2024
5d3f8a6
add try catch around pending read to align with others
Wraith2 Nov 4, 2024
36cdd88
Merge branch 'main' of https://github.com/dotnet/SqlClient into opera…
David-Engel Nov 8, 2024
433ccf7
Merge branch 'main' of https://github.com/dotnet/SqlClient into opera…
David-Engel Dec 18, 2024
c65da55
Apply suggestions from code review
David-Engel Dec 18, 2024
b4460ec
Fix merge error
David-Engel Dec 18, 2024
80cbb6e
add DumpBuffer function description and format code
Wraith2 Dec 27, 2024
bb4fa9e
review feedback 2
Wraith2 Dec 31, 2024
79f9fbc
move Packet disposal to debug only
Wraith2 Jan 3, 2025
d31e908
review feedback 3
Wraith2 Jan 7, 2025
429b7ac
review feedback 4
Wraith2 Jan 17, 2025
11dd84e
add app context switched behaviour to allow reverting to previous pro…
Wraith2 Jan 19, 2025
7684861
revirew feedback 5
Wraith2 Jan 31, 2025
cd1e025
fix tests build
Wraith2 Jan 31, 2025
3ed9c83
fix netfx builds
Wraith2 Feb 7, 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
6 changes: 6 additions & 0 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 @@ -188,6 +188,9 @@
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\OnChangedEventHandler.cs">
<Link>Microsoft\Data\SqlClient\OnChangedEventHandler.cs</Link>
</Compile>
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\Packet.cs">
<Link>Microsoft\Data\SqlClient\Packet.cs</Link>
</Compile>
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\ParameterPeekAheadValue.cs">
<Link>Microsoft\Data\SqlClient\ParameterPeekAheadValue.cs</Link>
</Compile>
Expand Down Expand Up @@ -581,6 +584,9 @@
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\TdsParserStateObject.cs">
<Link>Microsoft\Data\SqlClient\TdsParserStateObject.cs</Link>
</Compile>
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\TdsParserStateObject.Multiplexer.cs">
<Link>Microsoft\Data\SqlClient\TdsParserStateObject.Multiplexer.cs</Link>
</Compile>
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\TdsParserStaticMethods.cs">
<Link>Microsoft\Data\SqlClient\TdsParserStaticMethods.cs</Link>
</Compile>
Expand Down
9 changes: 5 additions & 4 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlDataReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3724,7 +3724,7 @@ private TdsOperationStatus TryNextResult(out bool more)

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

if (_parser != null)
{ // There is a valid case where parser is null
// Peek, and if row token present, set _hasRows true since there is a
// row in the result
{
// There is a valid case where parser is null
// Peek, and if row token present, set _hasRows true since there is a
// row in the result
byte b;
TdsOperationStatus result = _stateObj.TryPeekByte(out b);
if (result != TdsOperationStatus.Done)
Expand Down
4 changes: 4 additions & 0 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ internal void PostReadAsyncForMars()

_pMarsPhysicalConObj.IncrementPendingCallbacks();
SessionHandle handle = _pMarsPhysicalConObj.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(_physicalStateObj.PartialPacket==null);
temp = _pMarsPhysicalConObj.ReadAsync(handle, out error);

Debug.Assert(temp.Type == PacketHandle.NativePointerType, "unexpected packet type when requiring NativePointer");
Expand Down
14 changes: 11 additions & 3 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 @@ -2047,11 +2047,19 @@ internal TdsOperationStatus TryRun(RunBehavior runBehavior, SqlCommand cmdHandle

if (!IsValidTdsToken(token))
{
Debug.Fail($"unexpected token; token = {token,-2:X2}");
#if DEBUG
string message = stateObj.DumpBuffer();
Debug.Fail(message);
#endif
_state = TdsParserState.Broken;
_connHandler.BreakConnection();
SqlClientEventSource.Log.TryTraceEvent("<sc.TdsParser.Run|ERR> Potential multi-threaded misuse of connection, unexpected TDS token found {0}", ObjectID);
#if DEBUG
throw new InvalidOperationException(message);
#else
throw SQL.ParsingError();
#endif

}

int tokenLength;
Expand Down Expand Up @@ -4143,6 +4151,7 @@ internal TdsOperationStatus TryProcessReturnValue(int length, TdsParserStateObje
{
return result;
}

byte len;
result = stateObj.TryReadByte(out len);
if (result != TdsOperationStatus.Done)
Expand Down Expand Up @@ -4540,7 +4549,6 @@ internal TdsOperationStatus TryProcessCollation(TdsParserStateObject stateObj, o
collation = null;
return result;
}

if (SqlCollation.Equals(_cachedCollation, info, sortId))
{
collation = _cachedCollation;
Expand Down Expand Up @@ -5263,7 +5271,7 @@ private TdsOperationStatus TryCommonProcessMetaData(TdsParserStateObject stateOb
{
// If the column is encrypted, we should have a valid cipherTable
if (cipherTable != null)
{
{
result = TryProcessTceCryptoMetadata(stateObj, col, cipherTable, columnEncryptionSetting, isReturnValue: false);
if (result != TdsOperationStatus.Done)
{
Expand Down
77 changes: 17 additions & 60 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 @@ -322,14 +322,22 @@ private void ReadSniError(TdsParserStateObject stateObj, uint error)
stateObj.SendAttention(mustTakeWriteLock: true);

PacketHandle syncReadPacket = default;
bool readFromNetwork = true;
RuntimeHelpers.PrepareConstrainedRegions();
bool shouldDecrement = false;
try
{
Interlocked.Increment(ref _readingCount);
shouldDecrement = true;

syncReadPacket = ReadSyncOverAsync(stateObj.GetTimeoutRemaining(), out error);
readFromNetwork = !PartialPacketContainsCompletePacket();
if (readFromNetwork)
{
syncReadPacket = ReadSyncOverAsync(stateObj.GetTimeoutRemaining(), out error);
}
else
{
error = TdsEnums.SNI_SUCCESS;
}

Interlocked.Decrement(ref _readingCount);
shouldDecrement = false;
Expand All @@ -342,7 +350,7 @@ private void ReadSniError(TdsParserStateObject stateObj, uint error)
}
else
{
Debug.Assert(!IsValidPacket(syncReadPacket), "unexpected syncReadPacket without corresponding SNIPacketRelease");
Debug.Assert(!readFromNetwork || !IsValidPacket(syncReadPacket), "unexpected syncReadPacket without corresponding SNIPacketRelease");
fail = true; // Subsequent read failed, time to give up.
}
}
Expand All @@ -353,7 +361,7 @@ private void ReadSniError(TdsParserStateObject stateObj, uint error)
Interlocked.Decrement(ref _readingCount);
}

if (!IsPacketEmpty(syncReadPacket))
if (readFromNetwork && !IsPacketEmpty(syncReadPacket))
{
ReleasePacket(syncReadPacket);
}
Expand Down Expand Up @@ -393,60 +401,9 @@ private void ReadSniError(TdsParserStateObject stateObj, uint error)
AssertValidState();
}

public void ProcessSniPacket(PacketHandle packet, uint error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we still keep the older function? I am still trying to understand all the checks that were removed with the new function call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous version of ProcessSniPacket ? Why would we? we can't swap out the implementations without also adding the ability to disable all partial packet checking. The core idea of the PR was that the change was limited to a small location, primarily ProcessSniPacket which is the communication point between two layers. Unfortunately the rather messy bug fixes that have been needed in other places make this more complex but ProcessSniPacket is still the primary location where new functionality is present.

private uint GetSniPacket(PacketHandle packet, ref uint dataSize)
{
if (error != 0)
{
if ((_parser.State == TdsParserState.Closed) || (_parser.State == TdsParserState.Broken))
{
// Do nothing with callback if closed or broken and error not 0 - callback can occur
// after connection has been closed. PROBLEM IN NETLIB - DESIGN FLAW.
return;
}

AddError(_parser.ProcessSNIError(this));
AssertValidState();
}
else
{
uint dataSize = 0;

uint getDataError = SNIPacketGetData(packet, _inBuff, ref dataSize);

if (getDataError == TdsEnums.SNI_SUCCESS)
{
if (_inBuff.Length < dataSize)
{
Debug.Assert(true, "Unexpected dataSize on Read");
throw SQL.InvalidInternalPacketSize(StringsHelper.GetString(Strings.SqlMisc_InvalidArraySizeMessage));
}

_lastSuccessfulIOTimer._value = DateTime.UtcNow.Ticks;
_inBytesRead = (int)dataSize;
_inBytesUsed = 0;

if (_snapshot != null)
{
_snapshot.AppendPacketData(_inBuff, _inBytesRead);
if (_snapshotReplay)
{
_snapshot.MoveNext();
#if DEBUG
_snapshot.AssertCurrent();
#endif
}
}

SniReadStatisticsAndTracing();
SqlClientEventSource.Log.TryAdvancedTraceBinEvent("TdsParser.ReadNetworkPacketAsyncCallback | INFO | ADV | State Object Id {0}, Packet read. In Buffer: {1}, In Bytes Read: {2}", ObjectID, _inBuff, _inBytesRead);

AssertValidState();
}
else
{
throw SQL.ParsingError(ParsingErrorState.ProcessSniPacketFailed);
}
}
return SNIPacketGetData(packet, _inBuff, ref dataSize);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it will be possible to avoid this function hop GetSniPacket->SNIPacketGetData?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly. It's part of the separation between the managed and unmanaged SNI implementations and looks like it's currently just a function to function indirection to the override. If it's really important I can collapse it but it's unlikely to cause any problems and it keeps the existing structure in place for those familiar with the layering making it easier to understand.

}

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

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

// The timer thread may be unreliable under high contention scenarios. It cannot be
// assumed that the timeout has happened on the timer thread callback. Check the timeout
// synchrnously and then call OnTimeoutSync to force an atomic change of state.
// synchronously and then call OnTimeoutSync to force an atomic change of state.
if (TimeoutHasExpired)
{
OnTimeoutSync(asyncClose: true);
Expand Down Expand Up @@ -1633,7 +1590,7 @@ internal void AssertStateIsClean()
if ((parser != null) && (parser.State != TdsParserState.Closed) && (parser.State != TdsParserState.Broken))
{
// Async reads
Debug.Assert(_snapshot == null && !_snapshotReplay, "StateObj has leftover snapshot state");
Debug.Assert(_snapshot == null && _snapshotStatus == SnapshotStatus.NotActive, "StateObj has leftover snapshot state");
Debug.Assert(!_asyncReadWithoutSnapshot, "StateObj has AsyncReadWithoutSnapshot still enabled");
Debug.Assert(_executionContext == null, "StateObj has a stored execution context from an async read");
// Async writes
Expand Down
6 changes: 6 additions & 0 deletions src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,9 @@
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\OnChangedEventHandler.cs">
<Link>Microsoft\Data\SqlClient\OnChangedEventHandler.cs</Link>
</Compile>
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\Packet.cs">
<Link>Microsoft\Data\SqlClient\Packet.cs</Link>
</Compile>
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\ParameterPeekAheadValue.cs">
<Link>Microsoft\Data\SqlClient\ParameterPeekAheadValue.cs</Link>
</Compile>
Expand Down Expand Up @@ -772,6 +775,9 @@
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\TdsParserStateObject.cs">
<Link>Microsoft\Data\SqlClient\TdsParserStateObject.cs</Link>
</Compile>
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\TdsParserStateObject.Multiplexer.cs">
<Link>Microsoft\Data\SqlClient\TdsParserStateObject.Multiplexer.cs</Link>
</Compile>
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\TdsParserStaticMethods.cs">
<Link>Microsoft\Data\SqlClient\TdsParserStaticMethods.cs</Link>
</Compile>
Expand Down
1 change: 1 addition & 0 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 @@ -4364,6 +4364,7 @@ internal TdsOperationStatus TryProcessReturnValue(int length,
return result;
}
}

byte len;
result = stateObj.TryReadByte(out len);
if (result != TdsOperationStatus.Done)
Expand Down
73 changes: 15 additions & 58 deletions ...Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.netfx.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,14 +453,22 @@ private void ReadSniError(TdsParserStateObject stateObj, uint error)
stateObj.SendAttention(mustTakeWriteLock: true);

PacketHandle syncReadPacket = default;
bool readFromNetwork = true;
RuntimeHelpers.PrepareConstrainedRegions();
bool shouldDecrement = false;
try
{
Interlocked.Increment(ref _readingCount);
shouldDecrement = true;

syncReadPacket = ReadSyncOverAsync(stateObj.GetTimeoutRemaining(), out error);
readFromNetwork = !PartialPacketContainsCompletePacket();
if (readFromNetwork)
{
syncReadPacket = ReadSyncOverAsync(stateObj.GetTimeoutRemaining(), out error);
}
else
{
error = TdsEnums.SNI_SUCCESS;
}

Interlocked.Decrement(ref _readingCount);
shouldDecrement = false;
Expand All @@ -473,7 +481,7 @@ private void ReadSniError(TdsParserStateObject stateObj, uint error)
}
else
{
Debug.Assert(!IsValidPacket(syncReadPacket), "unexpected syncReadPacket without corresponding SNIPacketRelease");
Debug.Assert(!readFromNetwork || !IsValidPacket(syncReadPacket), "unexpected syncReadPacket without corresponding SNIPacketRelease");
fail = true; // Subsequent read failed, time to give up.
}
}
Expand All @@ -484,7 +492,7 @@ private void ReadSniError(TdsParserStateObject stateObj, uint error)
Interlocked.Decrement(ref _readingCount);
}

if (!IsPacketEmpty(syncReadPacket))
if (readFromNetwork && !IsPacketEmpty(syncReadPacket))
{
// Be sure to release packet, otherwise it will be leaked by native.
ReleasePacket(syncReadPacket);
Expand Down Expand Up @@ -525,60 +533,9 @@ private void ReadSniError(TdsParserStateObject stateObj, uint error)
AssertValidState();
}

public void ProcessSniPacket(PacketHandle packet, uint error)
private uint GetSniPacket(PacketHandle packet, ref uint dataSize)
{
if (error != 0)
{
if ((_parser.State == TdsParserState.Closed) || (_parser.State == TdsParserState.Broken))
{
// Do nothing with callback if closed or broken and error not 0 - callback can occur
// after connection has been closed. PROBLEM IN NETLIB - DESIGN FLAW.
return;
}

AddError(_parser.ProcessSNIError(this));
AssertValidState();
}
else
{
uint dataSize = 0;

uint getDataError = SniNativeWrapper.SNIPacketGetData(packet, _inBuff, ref dataSize);

if (getDataError == TdsEnums.SNI_SUCCESS)
{
if (_inBuff.Length < dataSize)
{
Debug.Assert(true, "Unexpected dataSize on Read");
throw SQL.InvalidInternalPacketSize(StringsHelper.GetString(Strings.SqlMisc_InvalidArraySizeMessage));
}

_lastSuccessfulIOTimer._value = DateTime.UtcNow.Ticks;
_inBytesRead = (int)dataSize;
_inBytesUsed = 0;

if (_snapshot != null)
{
_snapshot.AppendPacketData(_inBuff, _inBytesRead);
if (_snapshotReplay)
{
_snapshot.MoveNext();
#if DEBUG
_snapshot.AssertCurrent();
#endif
}
}

SniReadStatisticsAndTracing();
SqlClientEventSource.Log.TryAdvancedTraceBinEvent("TdsParser.ReadNetworkPacketAsyncCallback | INFO | ADV | State Object Id {0}, Packet read. In Buffer: {1}, In Bytes Read: {2}", ObjectID, _inBuff, _inBytesRead);

AssertValidState();
}
else
{
throw SQL.ParsingError(ParsingErrorState.ProcessSniPacketFailed);
}
}
return SniNativeWrapper.SNIPacketGetData(packet, _inBuff, ref dataSize);
}

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