fix null ref in SqlDataReader#4159
Merged
paulmedynski merged 4 commits intodotnet:mainfrom Apr 17, 2026
Merged
Conversation
The bug: At SqlDataReader.cs:2140, when buffer is null and bufferIndex < 0, the condition (bufferIndex < 0) is true, so execution enters the block. But buffer.Length in the exception constructor throws NullReferenceException since buffer is null. This only affects the PLP + SequentialAccess path in GetChars. The other GetChars/GetBytes paths at lines 1760 and 1891 don't have the buffer != null && guard, so they'd NRE on the condition itself (different issue, but buffer is expected non-null there). The fix (SqlDataReader.cs:2140): Changed buffer.Length to buffer?.Length ?? 0 so it reports size 0 when buffer is null. The test (DataReaderTest.cs): Added GetCharsSequentialAccess_NullBufferNegativeBufferIndex_ThrowsArgumentOutOfRange which calls reader.GetChars(0, 0, null, -1, 0) with CommandBehavior.SequentialAccess on an NVARCHAR(MAX) column and asserts it throws ArgumentOutOfRangeException with paramName: "bufferIndex" — without the fix this throws NullReferenceException instead.
Contributor
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a NullReferenceException in SqlDataReader.GetChars when called in the PLP + CommandBehavior.SequentialAccess path with buffer == null and a negative bufferIndex, ensuring the intended ArgumentOutOfRangeException is thrown instead.
Changes:
- Update
SqlDataReader.GetChars(PLP + SequentialAccess) to avoid dereferencingbuffer.Lengthwhenbufferis null while constructing the argument exception. - Add a regression test covering
GetCharswith SequentialAccess + PLP,buffer == null, and negativebufferIndex. - Add additional tests for non-sequential
GetCharsandGetBytesnull-buffer + negativebufferIndexbehavior (currently inconsistent with implementation).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs |
Prevents NRE when building the invalid bufferIndex exception in the PLP + SequentialAccess GetChars path; also adds some redundant null-guards in exception-path validation. |
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs |
Adds a regression test for the sequential PLP scenario and two additional tests that currently don’t match product behavior when buffer == null. |
paulmedynski
added
the
Hotfix Candidate 🚑
paulmedynski
requested changes
Apr 10, 2026
Contributor
paulmedynski
left a comment
paulmedynski
left a comment
There was a problem hiding this comment.
Please address the Copilot comments.
github-project-automation
Bot
moved this from To triage
to In progress
in SqlClient Board
…DataReader.GetChars
paulmedynski
approved these changes
Apr 14, 2026
Contributor
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #4159 +/- ##
===========================================
- Coverage 74.27% 64.20% -10.07%
===========================================
Files 279 272 -7
Lines 42980 65763 +22783
===========================================
+ Hits 31922 42222 +10300
- Misses 11058 23541 +12483
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:
|
mdaigle
added
Hotfix 7.0.2
mdaigle
approved these changes
Apr 17, 2026
This was referenced Apr 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug: At SqlDataReader.cs:2140, when buffer is null and bufferIndex < 0, the condition (bufferIndex < 0) is true, so execution enters the block. But buffer.Length in the exception constructor throws NullReferenceException since buffer is null.
This only affects the PLP + SequentialAccess path in GetChars. The other GetChars/GetBytes paths at lines 1760 and 1891 don't have the buffer != null && guard, so they'd NRE on the condition itself (different issue, but buffer is expected non-null there).
The fix (SqlDataReader.cs:2140): Changed buffer.Length to buffer?.Length ?? 0 so it reports size 0 when buffer is null.
The test (DataReaderTest.cs): Added GetCharsSequentialAccess_NullBufferNegativeBufferIndex_ThrowsArgumentOutOfRange which calls reader.GetChars(0, 0, null, -1, 0) with CommandBehavior.SequentialAccess on an NVARCHAR(MAX) column and asserts it throws ArgumentOutOfRangeException with paramName: "bufferIndex" — without the fix this throws NullReferenceException instead.