mssql-tds: SQL_VARIANT narrows a u32 data length to u8, desynchronizing the token stream

[saurabh500]

Summary

read_sql_variant reads the variant frame length as a u32 off the wire and derives data_length from it, but six of the per-type dispatch arms narrow that value to u8 before handing it to a reader. When data_length > 255 the low byte survives and the high bytes are dropped, so the arm consumes far fewer bytes than the frame declared and the remainder is left on the stream, where it is parsed as the next column or token.

This is the same desync class PR #237 removes from read_decimal_data, sitting one frame above the code that PR touches.

Found while reviewing #237 — see this thread. Deliberately left out of that PR to keep it scoped.

Where

data_length is computed as a u32 with no upper bound:

mssql-rs/mssql-tds/src/datatypes/decoder.rs

Lines 508 to 522 in 5249a46

	let length = reader.read_uint32().await?;
	let variant_base_type = reader.read_byte().await?;
	let tds_type = TdsDataType::try_from(variant_base_type)?;
	let variant_prop_bytes = reader.read_byte().await?;
	let bytes_for_type_and_properties_byte = 2;
	// Use checked arithmetic to prevent integer underflow
	let data_length = length
	.checked_sub(bytes_for_type_and_properties_byte)
	.and_then(|v| v.checked_sub(variant_prop_bytes as u32))
	.ok_or_else(|| {
	crate::error::Error::ProtocolError(format!(
	"SQL_VARIANT data length calculation underflow: length={length}, prop_bytes={variant_prop_bytes}"
	))
	})?;

The dispatch arms then split into two groups. The two that widen are both range-checked; the six that narrow are not:

Prop bytes	Arm	Conversion	Bounded?
0	fixed-length types (L567)	as usize	n/a — reader uses a fixed width
0	Guid (L584)	as u8	❌
0	DateN (L585)	as u8	❌
1	TimeN (L606)	as u8	❌
1	DateTime2N (L610)	as u8	❌
1	DateTimeOffsetN (L614)	as u8	❌
2	BigVarBinary / BigBinary (L2116)	as usize	✅ MAX_ALLOC_SIZE
2	NumericN / DecimalN (L2129)	as u8	❌
7	string types (L2172)	as usize	✅ MAX_ALLOC_SIZE

The numeric arm and the binary arm are adjacent in the same match, one range-checks and the other truncates. That reads as an oversight rather than a distinction.

// decode_two_propbyte_variant
TdsDataType::BigVarBinary | TdsDataType::BigBinary => {
    let _max_length: u16 = reader.read_uint16().await?;
    if data_length as usize > MAX_ALLOC_SIZE {   // widened, checked
        return Err(...);
    }
    let mut buffer = vec![0u8; data_length as usize];
    reader.read_bytes(&mut buffer).await?;       // consumes every declared byte
    ColumnValues::Bytes(buffer)
}
TdsDataType::NumericN | TdsDataType::DecimalN => {
    let precision = reader.read_byte().await?;
    let scale = reader.read_byte().await?;
    let decimal_parts =
        GenericDecoder::read_decimal_data(reader, data_length as u8, precision, scale)  // narrowed
            .await?;

Impact

Server-controlled input. Every case below is a silent success — no error is raised, the read just resumes at the wrong offset:

Numeric, low byte non-zero. data_length = 65539 (0x10003) truncates to 3. read_decimal_data consumes 1 byte on main today; with #237 applied it consumes 3. Either way ~65,536 bytes are stranded and reinterpreted as the next field.

Numeric, low byte zero — value corruption too. data_length = 256 truncates to 0. read_decimal_data treats length 0 as NULL and returns immediately:

mssql-rs/mssql-tds/src/datatypes/decoder.rs

Lines 655 to 658 in 5249a46

	// If length is 0, then it is NULL.
	if length == 0 {
	return Ok(None);
	}

The column decodes as ColumnValues::Null having consumed zero bytes, and all 256 strand. A non-NULL value is reported as NULL and the stream desyncs.

Guid. read_guid rejects any length other than 16 — but data_length = 272 (0x110) truncates to exactly 16, passes the check, reads 16 bytes and strands 256. The validation is bypassed by the truncation that precedes it.

DateN. data_length = 256 → 0 → NULL, 256 bytes stranded. data_length = 259 → 3 → reads 3, strands 256.

TimeN. read_time matches 3 => 3 bytes, 4 => 4 bytes, _ => 5 bytes. data_length = 256 truncates to 0, falls through to the _ arm and reads 5 bytes for a value whose declared length was 256.

Reproduction sketch

No live server needed — mssql-mock-tds can emit the frame. Shape:

1. Row with two columns: a SQL_VARIANT holding a numeric, then any second column with a known value.
2. Hand-write the variant frame with length set so data_length is 65539, followed by 3 bytes of payload.
3. Decode the row.

Expected: a protocol error. Actual: column 1 decodes successfully and column 2 reads back garbage from inside column 1's payload.

Suggested fix

1. Bound the length once, before dispatch. All six arms want a u8; do the conversion where the invariant lives rather than at six call sites:

// in read_sql_variant, before the match on variant_prop_bytes
let narrow_len = || u8::try_from(data_length).map_err(|_| {
    crate::error::Error::ProtocolError(format!(
        "SQL_VARIANT data length {data_length} exceeds {} for base type {tds_type:?}",
        u8::MAX
    ))
});

Rejecting is correct here, not clamping — none of these types has a valid representation longer than 255 bytes, so an over-long declared length is malformed by definition.

2. Assert the stream position, not just the decoded value. Per David's note on the PR: a test that only checks the decoded value will pass while the stream is desynced. The assertion has to be that the next field reads back intact — the shape of decimal_partial_trailing_word_is_fully_consumed from #237.

Worth knowing before anyone reaches for a tidier assertion: TdsPacketReader exposes no position or consumed-bytes accessor (packet_reader.rs L46-L72), so an in-decoder assert consumed == data_length would mean adding a trait method. Following-column assertions in tests get the same coverage without that.

3. Cover each arm. Minimum: numeric with low byte 3, numeric with low byte 0 (the NULL case), Guid at 272, DateN at 256, TimeN at 256. Each asserting a following column reads back intact.

Notes

• Independent of Guard decimal magnitude reassembly against 128-bit shift overflow #237 — that PR fixes the limb count inside read_decimal_data, this is the length handed to it. Neither fix subsumes the other.
• Not a regression; this predates Guard decimal magnitude reassembly against 128-bit shift overflow #237 and odbc: mandatory source-type conversions for typed SQLGetData (P1a) [AB#47107] #217.