microsoft / microsoft/mssql-rs
mssql-tds: SQL_VARIANT narrows a u32 data length to u8, desynchronizing the token stream
- Dominant language
- Rust
- Stars
- 53
- Forks
- 14
- Avg merge
- 1d 15h
- Merged PRs (30d)
- 137
Description
## 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](https://github.com/microsoft/mssql-rs/pull/237) — see [this thread](https://github.com/microsoft/mssql-rs/pull/237#issuecomment-5285315041). Deliberately left out of that PR to keep it scoped.
## Where
`data_length` is computed as a `u32` with no upper bound:
https://github.com/microsoft/mssql-rs/blob/5249a4650262669afcbfed7696a5136b0061eed2/mssql-tds/src/datatypes/decoder.rs#L508-L522
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](https://github.com/microsoft/mssql-rs/blob/5249a4650262669afcbfed7696a5136b0061eed2/mssql-tds/src/datatypes/decoder.rs#L567)) | `as usize` | n/a — reader uses a fixed width |
| 0 | `Guid` ([L584](https://github.com/microsoft/mssql-rs/blob/5249a4650262669afcbfed7696a5136b0061eed2/mssql-tds/src/datatypes/decoder.rs#L584)) | **`as u8`** | ❌ |
| 0 | `DateN` ([L585](https://github.com/microsoft/mssql-rs/blob/5249a4650262669afcbfed7696a5136b0061eed2/mssql-tds/src/datatypes/decoder.rs#L585)) | **`as u8`** | ❌ |
| 1 | `TimeN` ([L606](https://github.com/microsoft/mssql-rs/blob/5249a4650262669afcbfed7696a5136b0061eed2/mssql-tds/src/datatypes/decoder.rs#L606)) | **`as u8`** | ❌ |
| 1 | `DateTime2N` ([L610](https://github.com/microsoft/mssql-rs/blob/5249a4650262669afcbfed7696a5136b0061eed2/mssql-tds/src/datatypes/decoder.rs#L610)) | **`as u8`** | ❌ |
| 1 | `DateTimeOffsetN` ([L614](https://github.com/microsoft/mssql-rs/blob/5249a4650262669afcbfed7696a5136b0061eed2/mssql-tds/src/datatypes/decoder.rs#L614)) | **`as u8`** | ❌ |
| 2 | `BigVarBinary` / `BigBinary` ([L2116](https://github.com/microsoft/mssql-rs/blob/5249a4650262669afcbfed7696a5136b0061eed2/mssql-tds/src/datatypes/decoder.rs#L2116-L2122)) | `as usize` | ✅ `MAX_ALLOC_SIZE` |
| 2 | `NumericN` / `DecimalN` ([L2129](https://github.com/microsoft/mssql-rs/blob/5249a4650262669afcbfed7696a5136b0061eed2/mssql-tds/src/datatypes/decoder.rs#L2129)) | **`as u8`** | ❌ |
| 7 | string types ([L2172](https://github.com/microsoft/mssql-rs/blob/5249a4650262669afcbfed7696a5136b0061eed2/mssql-tds/src/datatypes/decoder.rs#L2172-L2178)) | `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.
```rust
// 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:
https://github.com/microsoft/mssql-rs/blob/5249a4650262669afcbfed7696a5136b0061eed2/mssql-tds/src/datatypes/decoder.rs#L655-L658
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:
```rust
// 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](https://github.com/microsoft/mssql-rs/blob/5249a4650262669afcbfed7696a5136b0061eed2/mssql-tds/src/io/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 #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 #237 and #217.
Contributor guide
Research direction
Start in mssql-tds/src/datatypes/decoder.rs at read_sql_variant and compare the narrowing dispatch arms with the checked binary arm. Review the decimal_partial_trailing_word_is_fully_consumed test pattern and use mssql-mock-tds for malformed frames. Done means oversized lengths are rejected and following-column assertions remain intact for the listed numeric, Guid, DateN, and TimeN cases.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust, sql
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100