From e23d4cdccbf13ae4185e4d24c8abb94c8c3ce4cc Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Wed, 8 Apr 2026 13:54:16 +1000 Subject: [PATCH 1/3] fix null ref in SqlDataReader.GetChars MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../Microsoft/Data/SqlClient/SqlDataReader.cs | 2 +- .../SQL/DataReaderTest/DataReaderTest.cs | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs index 0809f322e2..d0bd8dc247 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs @@ -2137,7 +2137,7 @@ override public long GetChars(int i, long dataIndex, char[] buffer, int bufferIn // if bad buffer index, throw if ((bufferIndex < 0) || (buffer != null && bufferIndex >= buffer.Length)) { - throw ADP.InvalidDestinationBufferIndex(buffer.Length, bufferIndex, nameof(bufferIndex)); + throw ADP.InvalidDestinationBufferIndex(buffer?.Length ?? 0, bufferIndex, nameof(bufferIndex)); } // if there is not enough room in the buffer for data diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs index 5cf8b1e4b4..dbca743e61 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs @@ -741,6 +741,22 @@ DROP TABLE IF EXISTS [{tableName}] } } + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] + public static async Task GetCharsSequentialAccess_NullBufferNegativeBufferIndex_ThrowsArgumentOutOfRange() + { + using var connection = new SqlConnection(DataTestUtility.TCPConnectionString); + await connection.OpenAsync(); + + using var command = connection.CreateCommand(); + command.CommandText = "SELECT CONVERT(NVARCHAR(MAX), 'test')"; + + using var reader = await command.ExecuteReaderAsync(CommandBehavior.SequentialAccess); + Assert.True(await reader.ReadAsync()); + + var ex = Assert.Throws(() => reader.GetChars(0, 0, null, -1, 0)); + Assert.Equal("bufferIndex", ex.ParamName); + } + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] public static async Task CanGetCharsSequentially() { From e6b1b102ac119faa36afe4ad37c61ce691ef29fd Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Wed, 8 Apr 2026 13:56:54 +1000 Subject: [PATCH 2/3] . --- .../Microsoft/Data/SqlClient/SqlDataReader.cs | 12 +++---- .../SQL/DataReaderTest/DataReaderTest.cs | 32 +++++++++++++++++++ 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs index d0bd8dc247..cc8f1124ba 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs @@ -1889,13 +1889,13 @@ private TdsOperationStatus TryGetBytesInternal(int i, long dataIndex, byte[] buf } // if bad buffer index, throw - if (bufferIndex < 0 || bufferIndex >= buffer.Length) + if (bufferIndex < 0 || (buffer != null && bufferIndex >= buffer.Length)) { - throw ADP.InvalidDestinationBufferIndex(buffer.Length, bufferIndex, nameof(bufferIndex)); + throw ADP.InvalidDestinationBufferIndex(buffer?.Length ?? 0, bufferIndex, nameof(bufferIndex)); } // if there is not enough room in the buffer for data - if (cbytes + bufferIndex > buffer.Length) + if (buffer != null && cbytes + bufferIndex > buffer.Length) { throw ADP.InvalidBufferSizeOrIndex(cbytes, bufferIndex); } @@ -2246,13 +2246,13 @@ override public long GetChars(int i, long dataIndex, char[] buffer, int bufferIn } // if bad buffer index, throw - if (bufferIndex < 0 || bufferIndex >= buffer.Length) + if (bufferIndex < 0 || (buffer != null && bufferIndex >= buffer.Length)) { - throw ADP.InvalidDestinationBufferIndex(buffer.Length, bufferIndex, nameof(bufferIndex)); + throw ADP.InvalidDestinationBufferIndex(buffer?.Length ?? 0, bufferIndex, nameof(bufferIndex)); } // if there is not enough room in the buffer for data - if (cchars + bufferIndex > buffer.Length) + if (buffer != null && cchars + bufferIndex > buffer.Length) { throw ADP.InvalidBufferSizeOrIndex(cchars, bufferIndex); } diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs index dbca743e61..7074b3445f 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs @@ -757,6 +757,38 @@ public static async Task GetCharsSequentialAccess_NullBufferNegativeBufferIndex_ Assert.Equal("bufferIndex", ex.ParamName); } + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] + public static async Task GetChars_NullBufferNegativeBufferIndex_ThrowsArgumentOutOfRange() + { + using var connection = new SqlConnection(DataTestUtility.TCPConnectionString); + await connection.OpenAsync(); + + using var command = connection.CreateCommand(); + command.CommandText = "SELECT 'test'"; + + using var reader = await command.ExecuteReaderAsync(); + Assert.True(await reader.ReadAsync()); + + var ex = Assert.Throws(() => reader.GetChars(0, 0, null, -1, 4)); + Assert.Equal("bufferIndex", ex.ParamName); + } + + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] + public static async Task GetBytes_NullBufferNegativeBufferIndex_ThrowsArgumentOutOfRange() + { + using var connection = new SqlConnection(DataTestUtility.TCPConnectionString); + await connection.OpenAsync(); + + using var command = connection.CreateCommand(); + command.CommandText = "SELECT CONVERT(VARBINARY, 0x1234)"; + + using var reader = await command.ExecuteReaderAsync(); + Assert.True(await reader.ReadAsync()); + + var ex = Assert.Throws(() => reader.GetBytes(0, 0, null, -1, 4)); + Assert.Equal("bufferIndex", ex.ParamName); + } + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] public static async Task CanGetCharsSequentially() { From 37ffbf4e5faca39e9c1857720bbdbae496493e1e Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sun, 12 Apr 2026 10:40:36 +1000 Subject: [PATCH 3/3] . --- .../Microsoft/Data/SqlClient/SqlDataReader.cs | 12 +++---- .../SQL/DataReaderTest/DataReaderTest.cs | 32 ------------------- 2 files changed, 6 insertions(+), 38 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs index cc8f1124ba..d0bd8dc247 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs @@ -1889,13 +1889,13 @@ private TdsOperationStatus TryGetBytesInternal(int i, long dataIndex, byte[] buf } // if bad buffer index, throw - if (bufferIndex < 0 || (buffer != null && bufferIndex >= buffer.Length)) + if (bufferIndex < 0 || bufferIndex >= buffer.Length) { - throw ADP.InvalidDestinationBufferIndex(buffer?.Length ?? 0, bufferIndex, nameof(bufferIndex)); + throw ADP.InvalidDestinationBufferIndex(buffer.Length, bufferIndex, nameof(bufferIndex)); } // if there is not enough room in the buffer for data - if (buffer != null && cbytes + bufferIndex > buffer.Length) + if (cbytes + bufferIndex > buffer.Length) { throw ADP.InvalidBufferSizeOrIndex(cbytes, bufferIndex); } @@ -2246,13 +2246,13 @@ override public long GetChars(int i, long dataIndex, char[] buffer, int bufferIn } // if bad buffer index, throw - if (bufferIndex < 0 || (buffer != null && bufferIndex >= buffer.Length)) + if (bufferIndex < 0 || bufferIndex >= buffer.Length) { - throw ADP.InvalidDestinationBufferIndex(buffer?.Length ?? 0, bufferIndex, nameof(bufferIndex)); + throw ADP.InvalidDestinationBufferIndex(buffer.Length, bufferIndex, nameof(bufferIndex)); } // if there is not enough room in the buffer for data - if (buffer != null && cchars + bufferIndex > buffer.Length) + if (cchars + bufferIndex > buffer.Length) { throw ADP.InvalidBufferSizeOrIndex(cchars, bufferIndex); } diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs index 7074b3445f..dbca743e61 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs @@ -757,38 +757,6 @@ public static async Task GetCharsSequentialAccess_NullBufferNegativeBufferIndex_ Assert.Equal("bufferIndex", ex.ParamName); } - [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] - public static async Task GetChars_NullBufferNegativeBufferIndex_ThrowsArgumentOutOfRange() - { - using var connection = new SqlConnection(DataTestUtility.TCPConnectionString); - await connection.OpenAsync(); - - using var command = connection.CreateCommand(); - command.CommandText = "SELECT 'test'"; - - using var reader = await command.ExecuteReaderAsync(); - Assert.True(await reader.ReadAsync()); - - var ex = Assert.Throws(() => reader.GetChars(0, 0, null, -1, 4)); - Assert.Equal("bufferIndex", ex.ParamName); - } - - [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] - public static async Task GetBytes_NullBufferNegativeBufferIndex_ThrowsArgumentOutOfRange() - { - using var connection = new SqlConnection(DataTestUtility.TCPConnectionString); - await connection.OpenAsync(); - - using var command = connection.CreateCommand(); - command.CommandText = "SELECT CONVERT(VARBINARY, 0x1234)"; - - using var reader = await command.ExecuteReaderAsync(); - Assert.True(await reader.ReadAsync()); - - var ex = Assert.Throws(() => reader.GetBytes(0, 0, null, -1, 4)); - Assert.Equal("bufferIndex", ex.ParamName); - } - [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] public static async Task CanGetCharsSequentially() {