microsoft / microsoft/mssql-rs
SqlString::to_utf8_string panics on malformed UTF-8 from the server
- Dominant language
- Rust
- Stars
- 53
- Forks
- 14
- Avg merge
- 1d 15h
- Merged PRs (30d)
- 137
Description
### Describe the bug
`SqlString::to_utf8_string` panics on malformed UTF-8 instead of returning an error or replacing invalid sequences.
`mssql-tds/src/datatypes/sql_string.rs:60`:
```rust
EncodingType::Utf8 => String::from_utf8(self.bytes.clone()).unwrap(),
```
The bytes come straight off the wire, so this makes malformed server data a process-level panic in a library.
Three things make it worse than a normal `unwrap`:
1. **It is inconsistent with its own siblings.** The other two arms of the same `match` use `encoding_rs`, which is lossy and cannot panic — `EncodingType::Utf16` decodes via `UTF_16LE.decode(..)`, and `EncodingType::LcidBased` even has a `warn!` + Windows-1252 fallback for unsupported LCIDs. Only the UTF-8 arm aborts.
2. **It is reachable from `Display` and `Debug`** (`sql_string.rs:143` and `:153`). Panicking inside `Display` means logging or formatting a row can bring the process down, including on error paths that are trying to report an unrelated failure.
3. **It is on a broad public surface.** `to_utf8_string` is `pub` and has ~14 call sites across every binding — `mssql-js` (`binary_row_writer.rs`), `mssql-py-core` (`convert.rs`, `cursor.rs`), and `mssql-odbc` (`cell.rs`, `fetch_convert.rs`, `tds_value_serializer.rs`, `arrow_bulkcopy.rs`, `describe_parameter_encryption.rs`).
There is also a latent hint that this arm is not well understood, directly above the line:
```rust
// TODO: Investigation needed. When creating a Utf8 strings from the vector, the string is weirdly encoded.
// UTF16 decode works better.
```
### Steps to reproduce
The `EncodingType::Utf8` arm is selected for UTF-8 collations (`collation.utf8()`, `sql_string.rs:168`), i.e. SQL Server 2019+ collations such as `Latin1_General_100_CI_AS_SC_UTF8`.
Direct unit-level reproduction, no server required:
```rust
let s = SqlString::new(vec![0xE4, 0xBD], EncodingType::Utf8); // truncated 3-byte sequence
let _ = s.to_utf8_string(); // panics: called `Result::unwrap()` on an `Err` value: FromUtf8Error
```
End-to-end, any condition that delivers bytes that are not valid UTF-8 for such a column reaches the same line. That includes token-stream desynchronization, where subsequent bytes are interpreted as the wrong field — see #280, where `SQL_VARIANT` narrows a `u32` data length to `u8` and desynchronizes the stream. A desync turns a recoverable protocol bug into a panic.
### Expected behavior
Malformed data from the network should not panic a library. Either:
- decode lossily and consistently with the other arms — `String::from_utf8_lossy(&self.bytes).into_owned()`, substituting U+FFFD; or
- return `Result` / add a fallible variant and let callers decide.
Lossy decoding is the smaller change and matches what `EncodingType::Utf16` and `EncodingType::LcidBased` already do. It also removes the redundant `self.bytes.clone()` — `from_utf8_lossy` borrows and allocates once on the owned conversion, so the valid path costs no more than today.
Changing the return type to `Result` is the more correct option but is a breaking change across all three bindings, so it likely wants to be a separate decision.
### Actual behavior
```
thread '...' panicked at mssql-tds/src/datatypes/sql_string.rs:60:
called `Result::unwrap()` on an `Err` value: FromUtf8Error { .. }
```
For an async client this aborts the task, and when reached via `Display`/`Debug` it can fire from inside logging or error-formatting code.
### Version
`main` @ `7a9c0b93` (line numbers from `mssql-tds/src/datatypes/sql_string.rs` at that commit)
### Affected crate
mssql-tds
### Environment
Platform-independent — the panic is in pure decode logic and does not depend on OS, TLS, or auth configuration.
### Additional context
Found while reviewing the row-decode path for #247 / #253. It is independent of any performance work and exists on `main` today.
Suggested minimal fix:
```rust
EncodingType::Utf8 => String::from_utf8_lossy(&self.bytes).into_owned(),
```
Worth pairing with a test that asserts a truncated multi-byte sequence yields U+FFFD rather than panicking, since `sql_string.rs` currently has ~20 tests for this type and none of them cover invalid input.
If the stricter `Result`-returning shape is preferred instead, that should be tracked separately because it changes the public API of all three language bindings.
Contributor guide
Research direction
Start in mssql-tds/src/datatypes/sql_string.rs at the EncodingType::Utf8 arm of SqlString::to_utf8_string, then review the existing SqlString tests and its Display and Debug implementations. Run the unit tests with a truncated UTF-8 sequence and verify malformed bytes no longer panic and produce the expected replacement behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 82/100