Validate TDS parser prerequisites and invalidate malformed responses by paulmedynski · Pull Request #4941 · dotnet/SqlClient

paulmedynski · 2026-10-29T13:08:27Z

Description

Validate the metadata and authentication prerequisites needed to consume TDS tokens. Recognizing a token type alone does not establish that its required state exists. Reject the affected malformed responses through the existing corrupted-stream error path instead of dereferencing missing state or using unrelated result metadata.

Area	Change	Protocol basis / rationale	Behavior
ROW / NBCROW	Replace a Debug-only metadata assertion with runtime validation before processing, skipping, or returning a row.	MS-TDS ROW, section 2.2.7.20 requires preceding type metadata; NBCROW, section 2.2.7.15 derives the null bitmap from the column count.	Reject missing effective metadata without requiring fresh metadata before every row or disallowing valid reuse.
Parser ALTROW handling	Validate the alternate-metadata collection and matching identifier before skipping values.	MS-TDS ALTROW, section 2.2.7.2 requires a matching ALTMETADATA, section 2.2.7.1 identifier in the same result set.	Reject missing or unmatched alternate metadata while retaining valid alternate-result handling.
SSPI processing	Require an initialized provider before changing processing context, renting a buffer, or invoking the provider.	MS-TDS SSPI token, section 2.2.7.22 and SSPI Message, section 2.2.6.8 describe the authentication exchange; provider availability is the corresponding implementation prerequisite.	Reject unavailable authentication state rather than dereferencing a null provider.
Reader result transitions	Guard the early ALTROW branch in `TryHasMoreResults` and validate collection/identifier state in `TryNextResult`.	The same ALTROW matching requirement applies to reader consumption, not just parser-side skipping. The early guard precedes the first collection dereference.	`NextResult` / `NextResultAsync` reject missing or unmatched state instead of throwing an incidental null-reference error or retaining primary-result metadata.
Shared rejection and cleanup	Trace the failure without packet contents, mark the parser Broken, break the connection, and throw the existing corrupted-stream parsing error.	Driver error/ownership policy; MS-TDS does not prescribe a .NET exception type.	Close the public connection and doom the physical connection so it cannot return to healthy pool reuse.
Alternate metadata lookup	Remove the Debug-only failure for an unmatched identifier; keep the null lookup result and validate it at both consumers.	Callers now own controlled rejection consistently in Debug and Release.	Avoid Debug-only interruption before the common failure path.
Simulated-server regressions	Add 48 cases covering login and command failures, sync/async APIs, both pool implementations, consumed/unread rows, valid result controls, and identifiers split across packets. Capture operation and disposal failures separately.	Disposal must not replace the operation's exception and falsely satisfy an assertion. Assert the exact operation error, Closed state before cleanup, and successful disposal.	Verify rejection, pool recovery, packet continuation, and preservation of valid ordinary/null-compressed/alternate results.
Real-server compatibility tests	Add four sync/async and default/sequential-access cases for nullable rows, unread-row skipping, and subsequent command reuse.	Exercise normal result processing against SQL Server.	Runtime validation remains blocked by local server connectivity; see Testing.

Public API and compatibility

No public signatures, reference declarations, connection-string settings, or AppContext switches change.
Affected malformed responses during Open / OpenAsync, command execution, and reader transitions now produce the existing InvalidOperationException parsing error (CorruptedTdsStream, internal state 18) and invalidate the connection, rather than incidental null-reference failures or unrelated metadata fallback.
State 18 is an internal parsing diagnostic, not a SQL Server error number.
This is not a universal SqlException contract or blanket exception conversion. Applications catching only SqlException will not catch this parsing error.
Valid operations are intended to remain unchanged. Simulated-server controls cover ordinary/null-compressed rows, alternate results, row skipping, and packet continuation.
This is bounded prerequisite validation, not a full protocol state-machine rewrite or exhaustive validation of every ALTMETADATA ordering/uniqueness rule.
No dependencies or localized resources change. Protocol rationale and observable behavior are documented here; no public declaration updates are required.

Testing

Reject missing row metadata, unmatched alternate metadata, and unavailable SSPI providers through the existing corrupted-stream failure path. Guard reader transitions and add simulated-server and compatibility coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

🔵 Needs a closer look

Parser-driven connection teardown changes need compatibility validation because SQL Server tests could not connect and Windows/native-SNI and net462 runtime paths remain unverified.

No open issues 🎉

What changed in this PR

This PR hardens SqlClient’s TDS response handling by validating required metadata and authentication state before consuming tokens. Malformed responses use the existing corrupted-stream error path and invalidate the connection.

Changes:

Add prerequisite checks for ROW, NBCROW, ALTROW, and SSPI processing, including reader result transitions.
Centralize connection invalidation and let alternate-metadata consumers reject unmatched identifiers.
Add 48 simulated-server regressions and four real-server compatibility cases covering sync/async operations and valid result processing.

The PR reports passing simulated-server tests and cross-builds; real-server compatibility and additional platform runtime validation remain outstanding.

File	Description
`src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ParserPrerequisiteTests.cs`	Tests malformed responses, pool recovery, valid results, and packet continuation.
`src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectivityTests/ParserResultProcessingTest.cs`	Adds real-server coverage for nullable rows, result skipping, and connection reuse.
`src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs`	Validates alternate metadata during reader result transitions.
`src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Parser/Tokens/TdsAltMetadataCollection.cs`	Removes the Debug-only lookup failure so consumers handle missing metadata.
`src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Parser/TdsParser.cs`	Adds token prerequisite checks and shared corrupted-stream rejection.

_{Review effort: Balanced}

codecov · 2026-10-29T14:19:35Z

Codecov Report

❌ Patch coverage is 77.77778% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.23%. Comparing base (c76d081) to head (83b1bb5).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
...t/src/Microsoft/Data/SqlClient/Parser/TdsParser.cs	84.00%	4 Missing ⚠️
...ient/src/Microsoft/Data/SqlClient/SqlDataReader.cs	63.63%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4941      +/-   ##
==========================================
- Coverage   67.68%   66.23%   -1.46%     
==========================================
  Files         311      305       -6     
  Lines       45249    68310   +23061     
==========================================
+ Hits        30629    45248   +14619     
- Misses      14620    23062    +8442

Flag	Coverage Δ
CI-SqlClient	`?`
PR-SqlClient-Project	`66.23% <77.77%> (?)`

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

☔ View full report in Codecov by Harness.
📢 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.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

🔵 Needs a closer look

Core TDS rejection now tears down connections across login, command, and reader paths, requiring maintainer validation because CI evidence is absent and real-server, supported-runtime, and SNI compatibility remain unverified.

No open issues 🎉

_{Review effort: Balanced}

benrr101

I think the tests need a bit of work (which is why I submitted #4948) - but overall the change looks good. I'm glad we can just outright reject malformed packets (and hopefully no one demands we put it behind a context switch)

benrr101 · 2026-10-30T20:36:36Z

            }
        }

-        // @TODO: Debug fail usage here isn't ideal.


NB: wheeeee, someone is fixing my todo's 😜

benrr101 · 2026-10-30T20:44:19Z

+    [InlineData(true, true, true, false)]
+    [InlineData(true, true, false, true)]
+    [InlineData(true, true, true, true)]
+    public async Task AlternateRowWithoutMatchingMetadata_ThrowsParsingError(


I'd like to see this broken up a bit more. Let's have a separate test for using a reader vs not using a reader.

benrr101 · 2026-10-30T20:44:53Z

+    [InlineData(false, true)]
+    [InlineData(true, false)]
+    [InlineData(true, true)]
+    public async Task ValidMetadata_PreservesResultProcessing(bool useReader, bool useAsync)


Same request applies here. Reader tests should get their own test

benrr101 · 2026-10-30T20:45:35Z

+    [InlineData(false, true)]
+    [InlineData(true, false)]
+    [InlineData(true, true)]
+    public async Task AlternateRowIdAcrossPackets_PreservesValidation(bool knownId, bool useAsync)


This test is basically doing a positive and negative test in the same test. Please split these into separate tests.

benrr101 · 2026-10-30T20:47:45Z

Sorry @paulmedynski I meant that to be "Request Changes"

paulmedynski added this to the 8.0.0 milestone Oct 29, 2026

Copilot AI balanced review requested due to automatic review settings October 29, 2026 13:08

github-project-automation Bot added this to SqlClient Board Oct 29, 2026

github-project-automation Bot moved this to To triage in SqlClient Board Oct 29, 2026

Copilot started reviewing on behalf of paulmedynski October 29, 2026 13:08

View session

Copilot AI reviewed Oct 29, 2026

View reviewed changes

paulmedynski moved this from To triage to In progress in SqlClient Board Oct 29, 2026

paulmedynski added Code Health 💊 Issues/PRs that are targeted to source code quality improvements. Hotfix Candidate 🚑 Issues/PRs that are candidate for backporting to earlier supported versions. labels Oct 29, 2026

paulmedynski marked this pull request as ready for review October 29, 2026 14:19

paulmedynski requested a review from a team as a code owner October 29, 2026 14:19

paulmedynski moved this from In progress to In review in SqlClient Board Oct 29, 2026

github-actions Bot assigned benrr101 and cheenamalhotra Oct 29, 2026

Explain parser regression test scenarios and assertions

83b1bb5

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI balanced review requested due to automatic review settings October 29, 2026 18:13

Copilot started reviewing on behalf of paulmedynski October 29, 2026 18:14

View session

Copilot AI reviewed Oct 29, 2026

View reviewed changes

benrr101 reviewed Oct 30, 2026

View reviewed changes

Navigation Menu

Repository navigation

Validate TDS parser prerequisites and invalidate malformed responses - #4941

Validate TDS parser prerequisites and invalidate malformed responses#4941
paulmedynski wants to merge 2 commits into
mainfrom
dev/paul/icm-tds-login

paulmedynski commented Oct 29, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

codecov Bot commented Oct 29, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

benrr101 left a comment

Uh oh!

benrr101 Oct 30, 2026

Uh oh!

benrr101 Oct 30, 2026

Uh oh!

benrr101 Oct 30, 2026

Uh oh!

benrr101 Oct 30, 2026

Uh oh!

benrr101 commented Oct 30, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

paulmedynski commented Oct 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Public API and compatibility

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

benrr101 left a comment

Choose a reason for hiding this comment

Uh oh!

benrr101 Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

benrr101 Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

benrr101 Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

benrr101 Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

benrr101 commented Oct 30, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

paulmedynski commented Oct 29, 2026 •

edited

Loading

codecov Bot commented Oct 29, 2026 •

edited

Loading