SqlDataRecord.SetBytes fails to set length, incorrectly preserving data from a previous call

[wjrogers]

Describe the bug

Suppose you are using IEnumerable<SqlDataRecord> as the value of a table-valued SqlParameter and re-using the same SqlDataRecord instance for each item. One of the fields is type VARBINARY(MAX), and you use the SetBytes method to copy data into the field. After the first call to SetBytes, a subsequent call that copies fewer bytes will write to the existing buffer without setting the field's length, causing the remaining stale buffered bytes from the first call to get copied to the server, too.

To reproduce

#!/usr/bin/env dotnet run
#:package Microsoft.Data.SqlClient@6.*

using Microsoft.Data.SqlClient.Server;
using System.Data;
using System.Data.SqlTypes;
using System.Text;

var columns = new SqlMetaData[]
{
    new("Data", SqlDbType.VarBinary, SqlMetaData.Max),
};

var rec = new SqlDataRecord(columns);
var buffer = new byte[4096];

// write first record value
SetBytes(rec, 0, buffer, "Two Words");
ReadBytesToConsole(rec, 0, buffer); // writes "Two Words"

// write second record value
SetBytes(rec, 0, buffer, "One");
ReadBytesToConsole(rec, 0, buffer); // writes "One Words"

// write third record value using SqlBytes, which avoids the bug
var sqlBytes = new SqlBytes(Encoding.UTF8.GetBytes("Three"));
rec.SetSqlBytes(0, sqlBytes);
ReadBytesToConsole(rec, 0, buffer); // writes "Three"

static void SetBytes(SqlDataRecord rec, int ordinal, byte[] buffer, string value)
{
    int length = Encoding.UTF8.GetBytes(value, buffer);
    rec.SetBytes(ordinal, 0, buffer, 0, length);
}

static void ReadBytesToConsole(SqlDataRecord rec, int ordinal, byte[] buffer)
{
    var length = rec.GetBytes(ordinal, 0, buffer, 0, buffer.Length);
    var s = Encoding.UTF8.GetString(buffer, 0, int.CreateChecked(length));
    Console.WriteLine(s);
}

Expected behavior

The program should output:

Two Words
One
Three

Further technical details

Microsoft.Data.SqlClient version: 6.1.4
.NET target: .NET 10.0
SQL Server version: N/A
Operating system: Windows 11 Pro 26200.7840

[mdaigle]

The internal byte array is only allocated on the first SetBytes call. Subsequent calls copy into that existing internal array. The internal array is only modified if it needs to be expanded to fit the new data.

SqlClient/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SqlRecordBuffer.cs

Lines 527 to 528 in 5d2d5f4

	_object = new byte[length];
	_type = StorageType.ByteArray;

Can you tell me more about the type of API you're looking for? It would be pretty expensive for us to implicitly allocate a new byte array on each SetBytes call and I wouldn't want to force that on others who rely on the existing behavior. What about something like a Clear() method?

(I also reopened #1805 as we need improved guidelines around SqlDataRecord reuse)

[edwardneal]

It's worth noting that the same problem likely occurs in SetChars.

One option which would have some symmetry with SqlDataReader.GetBytes might be to allow SqlDataRecord.SetBytes(ordinal, fieldOffset, buffer: null, bufferOffset: offset, length: length) to reallocate the underlying buffer.

[wjrogers]

Can you tell me more about the type of API you're looking for?

I don't want it to allocate a new byte array on every call; that would defeat the benefit of re-using the same instance. I think it should set length when you call SetBytes, the same as SetSqlBytes does here:

SqlClient/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/ValueUtilsSmi.cs

Lines 3265 to 3266 in d96eb70

	// Make sure to trim any left-over data
	setters.SetBytesLength(ordinal, currentOffset);

The current behavior produces corrupted data. It copies extra, stale bytes to the server because it doesn't remember the length of the buffered data unless it resizes its buffer.

[mdaigle]

The trouble with that approach is that there's a legitimate use case for calling SetBytes() multiple times. Because the existing API allows you to specify an offset within the destination buffer, others may be using that to iteratively populate the field. For backwards compatibility reasons, we won't be able to assume that the length should be reset.

[wjrogers]

The trouble with that approach is that there's a legitimate use case for calling SetBytes() multiple times

I don't think that's a problem. You can set BytesLength to fieldOffset + length. That's already how SqlRecordBuffer.SetBytes calculates the new length. It handles the non-zero fieldOffset case by checking the offset is not greater than BytesLength (no gap in copied data), then ensuring the buffer is big enough to fit the additional data.

SqlClient/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SqlRecordBuffer.cs

Lines 534 to 549 in 5332da3

	if (ndataIndex > BytesLength)
	{ // no gap is allowed
	throw ADP.ArgumentOutOfRange(nameof(fieldOffset));
	}
	if (ndataIndex + length > BytesLength)
	{ // beyond the current length
	int cbytes = ((byte[])_object).Length;
	if (ndataIndex + length > cbytes)
	{ // dynamic expansion
	byte[] data = new byte[Math.Max(ndataIndex + length, 2 * cbytes)];
	Buffer.BlockCopy((byte[])_object, 0, data, 0, (int)BytesLength);
	_object = data;
	}
	BytesLength = ndataIndex + length;
	}

I'd argue that this is where the bug is: it only sets BytesLength in the buffer-resize case. Either that last line should be moved down, outside the if statement, so it always sets BytesLength, or SqlRecordBuffer should never set BytesLength (deferring to ValueUtilsSmi, which calls SetBytesLength in many other cases).

[mdaigle]

I think your suggestion of always setting BytesLength makes sense, so let's call it the plan of record. Let me chat with my team and see if there are any other backwards compatibility concerns. This code is probably over a decade old, so we need to tread carefully with any changes that may impact the public surface.

[wjrogers]

For the record, using SqlBytes as a surrogate is an effective work-around! You can see the basic idea in my repro code, but it's worth mentioning you can re-use one SqlBytes instance, too, with e.g. a MemoryStream as the backing store.