Feature | Add scaffolding for SSRP response parsing#3741
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces test infrastructure and placeholder unit tests for SSRP (SQL Server Resolution Protocol) packet processing functionality. The changes lay the groundwork for testing both SQL Data Source responses and DAC (Dedicated Admin Connection) responses.
- Adds comprehensive test data generation utilities for creating and validating SSRP packet structures
- Implements utility classes for working with
ReadOnlySequence<byte>to parse binary network packets - Creates placeholder test methods marked with
ActiveIssuethat reference a planned implementation (issue #3700)
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| SsrpPacketTestData.cs | Provides test data generators for various valid and invalid SSRP packet scenarios, including SVR_RESP messages, DAC responses, and edge cases |
| SqlDataSourceResponseProcessorTest.cs | Placeholder unit tests for SQL Data Source response processing, covering empty buffers, invalid responses, and valid response data |
| DacResponseProcessorTest.cs | Placeholder unit tests for DAC response processing with similar coverage patterns |
| CodeAnalysis.cs | Adds NotNullWhenAttribute for better null-state analysis in conditional scenarios |
| ReadOnlySequenceUtilities.cs | Extension methods for reading bytes and little-endian ushort values from ReadOnlySequence<byte> |
| PacketBuffer.cs | Linked list node implementation for building ReadOnlySequence<byte> from multiple buffers |
| netfx/.../Microsoft.Data.SqlClient.csproj | Includes new common source files in .NET Framework project |
| netcore/.../Microsoft.Data.SqlClient.csproj | Includes new common source files in .NET Core project |
| public static bool ReadByte(this ref ReadOnlySequence<byte> sequence, ref ReadOnlySpan<byte> currSpan, ref long currPos, out byte value) | ||
| { | ||
| if (sequence.Length < sizeof(byte)) |
There was a problem hiding this comment.
The currPos parameter is incremented but its updated value may not reflect the actual state if the sequence is too short (line 23-27). Consider moving the increment after the length check succeeds, or document that callers should not rely on currPos when the method returns false.
There was a problem hiding this comment.
currPos is the current position in the ReadOnlySequence<byte>. If there's not enough space in the sequence, the method doesn't advance and it's thus appropriate to leave currPos untouched.
| public static bool ReadLittleEndian(this ref ReadOnlySequence<byte> sequence, ref ReadOnlySpan<byte> currSpan, ref long currPos, out ushort value) | ||
| { | ||
| if (sequence.Length < sizeof(ushort)) |
There was a problem hiding this comment.
Similar to the ReadByte method, the currPos parameter is incremented (line 69) before the actual read operation, which could leave it in an inconsistent state if the sequence length check fails. Consider moving the increment after successful validation or documenting this behavior.
There was a problem hiding this comment.
As per the ReadByte method - currPos is always in a consistent state. Once execution reaches line 69, there's guaranteed to be enough space in the ReadOnlySpan<byte> - the only question is whether we can directly read it from the current span, or need to reassemble it because byte 1 is in the current span and byte 2 is in the next span.
|
This pull request has been marked as stale due to inactivity for more than 30 days. If you would like to keep this pull request open, please provide an update or respond to any comments. Otherwise, it will be closed automatically in 7 days. |
github-actions
Bot
added
the
Stale
|
This PR is not stale. |
|
@edwardneal Can you rebase to main (no merge commit) to pickup the latest pipeline and CodeQL configs? That should eliminate the unnecessary CI checks and unblock the Code scanning. |
|
Since this is a new feature, we will need to prioritize our review of it against existing priorities, so this may not see much attention until early 2026. @edwardneal Do you think you could have all of the work for SSRP ready for the 7.0 GA in March? |
paulmedynski
added
Approval Needed
This consists of: * ReadOnlySequenceSegment derivative, * Utilities to read data from a ReadOnlySequence<byte> * Test cases
edwardneal
force-pushed
the
feat/ssrp/01-scaffolding
branch
from
3e5d56f to
c8f6688
Compare
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Thanks @paulmedynski. I tried rebasing, but that went badly wrong and the CodeQL error persisted. I think I've fixed it now, and the build looks like it's triggered properly. March is enough time to implement the feature, and I have a local branch with the SSRP response parsing logic and test cases. I definitely wouldn't prioritise SqlDataSourceEnumerator above the other changes in 7.0 though, and I think it's likely to be a little more sensitive to review (since the only way it can work is by parsing untrusted network broadcasts.) I'm happy enough for this series of PRs to lie dormant until post-GA. Implementation doesn't introduce a breaking change, it just means that clients can enumerate SQL Server instances on the local LAN rather than having GetDataSources throw an exception. |
|
This pull request has been marked as stale due to inactivity for more than 30 days. If you would like to keep this pull request open, please provide an update or respond to any comments. Otherwise, it will be closed automatically in 7 days. |
github-actions
Bot
added
the
Stale
|
This PR is not stale. |
paulmedynski
removed
the
Stale
|
This pull request has been marked as stale due to inactivity for more than 30 days. If you would like to keep this pull request open, please provide an update or respond to any comments. Otherwise, it will be closed automatically in 7 days. |
github-actions
Bot
added
the
Stale
|
This PR is not stale. |
github-actions
Bot
removed
the
Stale
|
@edwardneal - New conflicts here. I will work to get this approved, reviewed, and merged ASAP to avoid wasting your time with further conflicts. |
|
Thanks @paulmedynski, I've just merged. Most of the conflicts have come from PRs which predate the project merge. I'm not expecting many conflicts in this area of the codebase moving forwards, so we don't need to rush and displace any other priorities on that basis. |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
benrr101
left a comment
benrr101
left a comment
There was a problem hiding this comment.
I'm not entirely sure how this all comes together, buuuuuut I think it's fine. Requesting a handful of style changes for the tests, and a couple things to double check before approving.
github-project-automation
Bot
moved this from Backlog
to In progress
in SqlClient Board
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3741 +/- ##
==========================================
- Coverage 65.96% 64.38% -1.59%
==========================================
Files 275 273 -2
Lines 42993 65831 +22838
==========================================
+ Hits 28361 42383 +14022
- Misses 14632 23448 +8816
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
apoorvdeshmukh
added
Author attention needed
Lifted some constant string components to constants. Newline formatting. Where possible, used expression body.
|
Thanks @benrr101. There are a lot of formatting changes, but I think I've covered everything except for the names of the utility methods. The tests are empty because they're covering components which don't exist yet. As they're added, I'll populate them and set them to run in CI. They're in this PR so that we can confirm whether I need to add any other test cases to the set. I'm aware these components will eventually need to be able to parse real-world LAN traffic, so it needs to be tested against malicious packets. |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
| validTcpInfo, | ||
| omitVersion: true))), |
| CreateSVR_RESPMessage("tcp"), | ||
|
|
||
| // Port is non-numeric | ||
| CreateSVR_RESPMessage("tcp;one"), | ||
|
|
||
| // Port is > ushort.MaxValue | ||
| CreateSVR_RESPMessage("tcp;65536"), | ||
|
|
||
| // Port is < 0 | ||
| CreateSVR_RESPMessage("tcp;-1") | ||
| ]; | ||
|
|
||
| static ReadOnlySequence<byte> CreateSVR_RESPMessage(string tcpInfo) => |
There was a problem hiding this comment.
SCREAMING_SNAKE_CASE has now been removed.
Correct screaming snake case method. Correct ensure that test case provides a zero-length Version field (not an absent one.)
benrr101
left a comment
benrr101
left a comment
There was a problem hiding this comment.
Thanks for the changes! Looks good to me!
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Description
This is the first of four or five PRs which introduce an SSRP client to the managed SNI. This PR doesn't actually introduce any parsing code - it just adds test cases and lays some scaffolding/infrastructure down for the next one.
Key points of interest here:
ReadOnlySequence<byte>to store packet buffers, then read the SSRP responses from them. This introduces the use ofPacketBufferandReadOnlySequenceUtilities.ReadOnlySequenceUtilitiesis a workaround; netcore has aSequenceReader<T>type in-box. This isn't available to netfx though, and I didn't think it was worth maintaining two sets of fairly simple logic. We could alternatively bring theSequenceReader<T>andSequenceReaderExtensionssource code in-tree, as a few other libraries have done (examples 1 and 2.)Issues
Contributes to #3700.
Testing
There's nothing to test in this PR, so CI should validate it.