[priyankatiwari08]

Description

TdsParser.ConsumePreLoginHandshake reads the option header (2-byte offset + 2-byte length) and the option data for VERSION, ENCRYPT, INSTANCE, MARS and FEDAUTHREQUIRED without checking that they fall inside the received PRELOGIN response. If a server returns a malformed PRELOGIN response, an IndexOutOfRangeException escapes from SqlConnection.Open/OpenAsync instead of a connection error.

This change:

• Adds ReadPreLoginOptionHeader(preLoginResponse, headerOffset, int? expectedBytes), which validates an option header and returns the option's data offset and length:

  • the 4-byte header lies within the response;
  • when expectedBytes is given, the declared length equals it (VERSION = 6 bytes for UL_VERSION + US_SUBBUILD; ENCRYPT, INSTANCE, MARS and FEDAUTHREQUIRED = 1 byte). This stops a short option from being read together with the bytes of the next option;
  • the declared data (dataOffset + dataLength) lies within the response.

  Failures throw the existing ParsingErrorState.CorruptedTdsStream parsing error (Internal connection fatal error. Error state: 18, Offset: N or ..., Length: N) and write a trace event. This matches how other PRELOGIN/TDS validation failures are reported, e.g. FedAuthRequiredPreLoginResponseInvalidValue.

• Reads and validates every option header once, before dispatching on the option: exact length for the options the client reads, and bounds-only (expectedBytes: null) for THREADID, TRACEID and unknown options, which share one branch that skips the option (the previous Debug.Fail for unknown options is removed). ENCRYPT under TDS 8 is validated with its 1-byte length before the value is ignored.

• Adds a total-size check, as ODBC/OLE DB do: each option contributes its token, 4-byte header and declared data length, the terminator contributes 1 byte, and the total must equal the response length. This rejects bytes that no option describes, data claimed by more than one option (when there is no compensating gap), and a response with no terminator.

• Test TDS server: TDSPreLoginToken.Deflate now writes the ENCRYPT and TRACEID data only when it writes the matching option header (it previously wrote an encryption byte even when the ENCRYPT option was omitted).

Compatibility: Responses whose option data falls outside the response, whose read options declare a non-standard length, or whose options do not describe exactly the bytes of the response, are now rejected with a parsing error. The required lengths are the ones defined by MS-TDS and already enforced by ODBC/OLE DB (JDBC enforces them for VERSION and ENCRYPT); SQL Server 2022 and 2025 responses were verified to connect (see Testing). No public API changes.

Comparison with other Microsoft SQL Server drivers

The checks added here match how the other Microsoft drivers already parse the PRELOGIN response:

Check	ODBC / OLE DB	JDBC	mssql-rs	SqlClient (this PR)
Option header fits in the response	Yes (ipInBuf + sizeof(PRELOGINOPTHDR) > cbPayload)	Yes (responseIndex + 4 >= responseLength)	N/A: reads sequentially through a checked reader that errors on short data	Yes (offset + 4 > payload.Length)
Option data fits in the response	Yes (iOffSet + 6 > cbPayload for VERSION, iOffSet >= cbPayload for the others)	Yes (optionOffset + optionLength > responseLength)	N/A: offsets are ignored and values are read in order	Yes (payloadOffset + payloadLength > payload.Length)
Declared option length matches the expected size	Yes (VERSION = 6, the others = 1)	Yes (VERSION = 6, ENCRYPT = 1)	No	Yes (VERSION = 6, ENCRYPT/INSTANCE/MARS/FEDAUTHREQUIRED = 1)
Options and terminator describe exactly the response length	Yes (cbOptDataTotal != cbPayload)	No	No	Yes

With this change SqlClient enforces the same declared-length and total-size rules as ODBC/OLE DB (a superset of the JDBC checks) and, like JDBC, also bounds-checks the headers and data of options it does not read.

Suggested release note (Fixed): Fixed SqlConnection.Open/OpenAsync throwing IndexOutOfRangeException instead of a connection error when the server's PRELOGIN response contains option offsets or lengths that are invalid or outside the response.

Issues

N/A

Testing

Added SimulatedServerTests/PreLoginOptionBoundsTests (85 cases). It uses a TdsServer subclass that replaces the PRELOGIN response with hand-built bytes. Cases run on both sync Open and async OpenAsync:

• Well-formed response with every option the client reads: the connection opens (control).
• Each of VERSION/ENCRYPT/INSTANCE/MARS/FEDAUTHREQUIRED with offset 0xFFFF: parsing error, state 18.
• Each read option with data truncated exactly at the end of the response (boundary): parsing error, state 18.
• Each option, including THREADID and TRACEID, with a truncated 4-byte header: parsing error, state 18.
• Each read option declaring a wrong length (0, 2, 4, 7 as applicable): parsing error, Length: N.
• VERSION declaring 2 bytes placed so a 4-byte read would overlap the next option's data: parsing error, Length: 2.
• THREADID/TRACEID/an unknown option (0x10) with offset or declared length beyond the response, or (unknown option) a truncated header: parsing error. With any in-bounds length (including 0) that keeps the response fully described: the option is skipped and the connection opens (control).
• Undescribed trailing bytes, two options sharing a data byte, and a missing terminator: parsing error, Length: N.

Results:

• dotnet test src\Microsoft.Data.SqlClient\tests\UnitTests\Microsoft.Data.SqlClient.UnitTests.csproj -f net10.0 --filter "FullyQualifiedName~PreLoginOptionBoundsTests": Passed (85/85).
• The original 66 cases against the parser from the first commit (offset checks only): 30 new cases Failed, as expected. Against the original parser they fail with IndexOutOfRangeException.
• dotnet test ... -f net10.0 --filter "FullyQualifiedName~SimulatedServerTests": Passed (576/576, after merging latest main).
• dotnet test src\Microsoft.Data.SqlClient\tests\UnitTests\Microsoft.Data.SqlClient.UnitTests.csproj -f net10.0 (full suite, after the total-size check): Passed (2062 passed, 9 skipped, 0 failed).
• dotnet test src\Microsoft.Data.SqlClient\tests\FunctionalTests\Microsoft.Data.SqlClient.FunctionalTests.csproj -f net10.0: 1364 passed, 5 failed; the 5 failures are environment-specific on the Windows dev box (Unix-only Always Encrypted provider tests, certificate store paths, AttachDbFileName on Linux) and unrelated to PRELOGIN.
• dotnet test ... -f net462 --filter "FullyQualifiedName~PreLoginOptionBoundsTests": Passed (85/85).
• Live check with a small console app on the patched driver including the total-size check (net10.0, integrated auth) against local SQL Server 2022 (16.0.1210; default instance over shared memory and TCP, and a named instance) and SQL Server 2025 (17.0.1145) named instance, Encrypt=Optional and Encrypt=Mandatory, MARS on and off, sync and async open: all 32 combinations connected. Encrypt=Strict could not be exercised locally (TCP is not enabled on these instances); CI covers it.
• Tests added or updated
• Public API changes documented (none)
• Ensure no breaking changes introduced

Guidelines

Please review the contribution guidelines before submitting a pull request:

• Contributing
• Code of Conduct
• Best Practices
• Coding Style
• Review Process

[Copilot]

🔵 Needs a closer look

Current-head CI remains unverified, and the reported full .NET Framework simulated-server run is incomplete, leaving validation for human confirmation.

No open issues 🎉

What changed in this PR

This PR hardens SqlClient’s PRELOGIN parsing so malformed option headers or data offsets produce an existing connection parsing error instead of IndexOutOfRangeException.

Changes:

• Adds shared bounds validation for VERSION, ENCRYPT, INSTANCE, MARS, and FEDAUTHREQUIRED.
• Preserves existing handling of declared option lengths, skipped options, and TDS 8 encryption.
• Adds 32 simulated-server cases covering valid responses and malformed headers or offsets through both Open and OpenAsync.

File	Description
src/​Microsoft.Data.SqlClient/​tests/​UnitTests/​SimulatedServerTests/​PreLoginOptionBoundsTests.cs	Adds sync/async PRELOGIN bounds regression tests and a valid-response control.
src/​Microsoft.Data.SqlClient/​src/​Microsoft/​Data/​SqlClient/​TdsParser.cs	Validates option headers and consumed data before reading them.

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Required trusted review policies and testing guides were unavailable, and no PR identifier was supplied to verify existing reviewer feedback or head-specific CI.

No open issues 🎉

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Trusted review policies and testing guidance were unavailable, and missing PR and commit identifiers prevented verification of existing review feedback and current-head CI.

No open issues 🎉

_{Review effort: Balanced}

[codecov]

Codecov Report

❌ Patch coverage is 97.18310% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.74%. Comparing base (c76d081) to head (666148a).
⚠️ Report is 19 commits behind head on main.

Files with missing lines	Patch %	Lines
...t/src/Microsoft/Data/SqlClient/Parser/TdsParser.cs	97.18%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4902      +/-   ##
==========================================
- Coverage   67.68%   66.74%   -0.95%     
==========================================
  Files         311      305       -6     
  Lines       45249    68458   +23209     
==========================================
+ Hits        30629    45695   +15066     
- Misses      14620    22763    +8143     

Flag	Coverage Δ
CI-SqlClient	?
PR-SqlClient-Project	66.74% <97.18%> (?)

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.

[mdaigle]

Some code organization nits.

[Copilot]

🔵 Needs a closer look

Human assessment is needed because option-data overlaps can pass the length-sum checks when offset by an unused byte, a case the new test does not cover.

No open issues 🎉

_{Review effort: Balanced}