Feature | Add scaffolding for SSRP response parsing by edwardneal · Pull Request #3741 · dotnet/SqlClient

edwardneal · 2025-11-01T21:02:16Z

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:

Around two thirds of the additions here are test cases. I've tried to cover as many of these as I can see based upon the MC-SQLR specification, with a particular focus on malformed data. These will end up parsing unsanitised network traffic, so I'm completely open to adding any others which would cover malicious traffic.
I'm planning to make fairly heavy use of ReadOnlySequence<byte> to store packet buffers, then read the SSRP responses from them. This introduces the use of PacketBuffer and ReadOnlySequenceUtilities.
The existence of ReadOnlySequenceUtilities is a workaround; netcore has a SequenceReader<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 the SequenceReader<T> and SequenceReaderExtensions source 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.

apoorvdeshmukh · 2025-11-04T06:59:46Z

/azp run

azure-pipelines · 2025-11-04T07:00:14Z

Azure Pipelines successfully started running 2 pipeline(s).

Copilot

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 ActiveIssue that 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

Copilot · 2025-11-04T07:04:27Z

+    public static bool ReadByte(this ref ReadOnlySequence<byte> sequence, ref ReadOnlySpan<byte> currSpan, ref long currPos, out byte value)
+    {
+        if (sequence.Length < sizeof(byte))


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.

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.

Copilot · 2025-11-04T07:04:27Z

+    public static bool ReadLittleEndian(this ref ReadOnlySequence<byte> sequence, ref ReadOnlySpan<byte> currSpan, ref long currPos, out ushort value)
+    {
+        if (sequence.Length < sizeof(ushort))


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.

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.

github-actions · 2025-12-05T02:33:41Z

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.

edwardneal · 2025-12-05T06:52:58Z

This PR is not stale.

paulmedynski · 2025-12-05T13:39:43Z

@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.

paulmedynski · 2025-12-05T13:41:37Z

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?

This consists of: * ReadOnlySequenceSegment derivative, * Utilities to read data from a ReadOnlySequence<byte> * Test cases

paulmedynski · 2025-12-05T19:52:59Z

/azp run

azure-pipelines · 2025-12-05T19:53:20Z

Azure Pipelines successfully started running 2 pipeline(s).

edwardneal · 2025-12-05T20:05:00Z

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.

github-actions · 2026-01-05T02:46:52Z

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.

edwardneal · 2026-01-05T11:52:48Z

This PR is not stale.

github-actions · 2026-02-05T02:57:33Z

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.

edwardneal · 2026-02-05T04:37:46Z

This PR is not stale.

apoorvdeshmukh · 2026-05-12T09:17:32Z

/azp run

azure-pipelines · 2026-05-12T09:18:02Z

Azure Pipelines successfully started running 2 pipeline(s).

benrr101

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.

codecov · 2026-05-12T20:48:19Z

Codecov Report

❌ Patch coverage is 0% with 54 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.38%. Comparing base (be95ca2) to head (9c48a65).
⚠️ Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
...Microsoft/Data/Common/ReadOnlySequenceUtilities.cs	0.00%	40 Missing ⚠️
...qlClient/src/Microsoft/Data/Common/PacketBuffer.cs	0.00%	12 Missing ⚠️
...lient/src/System/Diagnostics/CodeAnalysis.netfx.cs	0.00%	2 Missing ⚠️

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

Flag	Coverage Δ
CI-SqlClient	`?`
PR-SqlClient-Project	`64.38% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Lifted some constant string components to constants. Newline formatting. Where possible, used expression body.

edwardneal · 2026-05-14T21:28:36Z

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.

apoorvdeshmukh · 2026-05-19T12:07:43Z

/azp run

azure-pipelines · 2026-05-19T12:08:13Z

Azure Pipelines successfully started running 2 pipeline(s).

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Correct screaming snake case method. Correct ensure that test case provides a zero-length Version field (not an absent one.)

benrr101

Thanks for the changes! Looks good to me!

benrr101 · 2026-05-20T17:29:04Z

/azp run

azure-pipelines · 2026-05-20T17:29:33Z

Azure Pipelines successfully started running 2 pipeline(s).

benrr101 · 2026-06-03T17:43:27Z

Ok, @edwardneal , kicking the builds again to see get this through. @apoorvdeshmukh please review at your earliest.

edwardneal requested a review from a team as a code owner November 1, 2025 21:02

github-project-automation Bot added this to SqlClient Board Nov 1, 2025

github-project-automation Bot moved this to To triage in SqlClient Board Nov 1, 2025

apoorvdeshmukh requested a review from Copilot November 4, 2025 06:58

Copilot AI reviewed Nov 4, 2025

View reviewed changes

paulmedynski self-assigned this Nov 4, 2025

cheenamalhotra moved this from To triage to In review in SqlClient Board Nov 17, 2025

github-actions Bot added the Stale The Issue or PR has become stale and will be automatically closed shortly if no activity occurs. label Dec 5, 2025

paulmedynski moved this from In review to Backlog in SqlClient Board Dec 5, 2025

paulmedynski added Approval Needed Issues/PRs that require approval from the maintainers before changes will be accepted. and removed Stale The Issue or PR has become stale and will be automatically closed shortly if no activity occurs. labels Dec 5, 2025

Add scaffolding for SSRP response parsing

c8f6688

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 December 5, 2025 19:33

github-actions Bot added the Stale The Issue or PR has become stale and will be automatically closed shortly if no activity occurs. label Jan 5, 2026

paulmedynski removed the Stale The Issue or PR has become stale and will be automatically closed shortly if no activity occurs. label Jan 5, 2026

github-actions Bot added the Stale The Issue or PR has become stale and will be automatically closed shortly if no activity occurs. label Feb 5, 2026

github-actions Bot removed the Stale The Issue or PR has become stale and will be automatically closed shortly if no activity occurs. label Feb 6, 2026

cheenamalhotra unassigned paulmedynski May 8, 2026

benrr101 requested changes May 12, 2026

View reviewed changes

github-project-automation Bot moved this from Backlog to In progress in SqlClient Board May 12, 2026

apoorvdeshmukh added Author attention needed PRs that require author to respond or make updates to PR. and removed Approval Needed Issues/PRs that require approval from the maintainers before changes will be accepted. labels May 13, 2026

edwardneal added 5 commits May 14, 2026 20:02

Merge branch 'main' into feat/ssrp/01-scaffolding

e7f50ec

Replace ActiveIssue attributes with [Theory(Skip = "...")]

3d8176a

Formatting changes

ec2d00e

Lifted some constant string components to constants. Newline formatting. Where possible, used expression body.

Naming convention changes

e65e4f1

Parameter line chopping

85c49d1

apoorvdeshmukh requested review from benrr101 and Copilot May 19, 2026 12:09

Copilot started reviewing on behalf of apoorvdeshmukh May 19, 2026 12:10 View session

Copilot AI reviewed May 19, 2026

View reviewed changes

Comment thread src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/Sql/SsrpPacketTestData.cs Outdated

Comment thread src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/Sql/SsrpPacketTestData.cs Outdated

edwardneal added 2 commits May 19, 2026 16:43

Respond to code review

26b0d6f

Correct screaming snake case method. Correct ensure that test case provides a zero-length Version field (not an absent one.)

Add SSRP parsing test cases

95842c8

benrr101 approved these changes May 20, 2026

View reviewed changes

cheenamalhotra removed the Author attention needed PRs that require author to respond or make updates to PR. label May 28, 2026

cheenamalhotra moved this from In progress to In review in SqlClient Board May 28, 2026

paulmedynski modified the milestones: 7.1.0-preview2, 7.1.0-preview3 Jun 30, 2026

Uh oh!

Conversation

edwardneal commented Nov 1, 2025

Description

Issues

Testing

Uh oh!

apoorvdeshmukh commented Nov 4, 2025

Uh oh!

azure-pipelines Bot commented Nov 4, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

edwardneal Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

edwardneal Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Dec 5, 2025

Uh oh!

edwardneal commented Dec 5, 2025

Uh oh!

paulmedynski commented Dec 5, 2025

Uh oh!

paulmedynski commented Dec 5, 2025

Uh oh!

paulmedynski commented Dec 5, 2025

Uh oh!

azure-pipelines Bot commented Dec 5, 2025

Uh oh!

edwardneal commented Dec 5, 2025

Uh oh!

github-actions Bot commented Jan 5, 2026

Uh oh!

edwardneal commented Jan 5, 2026

Uh oh!

github-actions Bot commented Feb 5, 2026

Uh oh!

edwardneal commented Feb 5, 2026

Uh oh!

apoorvdeshmukh commented May 12, 2026

Uh oh!

azure-pipelines Bot commented May 12, 2026

Uh oh!

benrr101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

edwardneal commented May 14, 2026

Uh oh!

apoorvdeshmukh commented May 19, 2026

Uh oh!

azure-pipelines Bot commented May 19, 2026

Uh oh!

Copilot AI left a comment

edwardneal Nov 4, 2025 •

edited

Loading

codecov Bot commented May 12, 2026 •

edited

Loading