microsoft / microsoft/mssql-rs

SqlString::to_utf8_string panics on malformed UTF-8 from the server

Open Beginner friendly
#310 0 comments 0 reactions 0 assignees View on GitHub
bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.