microsoft / microsoft/mssql-rs

mssql-tds / mssql-odbc: infallible allocations behind the MAX_ALLOC_SIZE/MAX_PLP_SIZE caps abort the host process on allocation failure

Open
#484 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

`mssql-tds` validates wire-declared lengths against size caps (`MAX_ALLOC_SIZE` = 100 MB, `MAX_PLP_SIZE` = `i32::MAX`), but the allocations *behind* those caps use infallible `Vec` APIs. When the allocator refuses the request, Rust's OOM handler calls `abort()` — inside a driver that is `dlopen`ed/`LoadLibrary`d into an arbitrary host process.

This is the un-closed half of [AB#40371](https://sqlclientdrivers.visualstudio.com/mssql-rs/_workitems/edit/40371) ("Bug #20: Out-of-Memory Attack via Unbounded Memory Allocation", Closed, Critical). That item fixed the *unbounded* half by introducing the caps. The caps bound *how big* an allocation can be; they do not change *what happens when it fails*. A 2 GB allocation is no longer unbounded, but it still aborts if the allocator says no.

Surfaced while reviewing #444, which fixed one instance by hand (`DaeProgress::pending_bytes` in `SQLPutData`) with `try_reserve` + `HY001`. The remaining instances are pre-existing on `main` and are not in that PR's scope.

### Steps to reproduce

These are code-inspection findings; no live repro is attached. The most reachable path:

1. Connect to a server (or a peer that can inject TDS frames) and select a `varbinary(max)` / `nvarchar(max)` value in the hundreds of MB to ~2 GB range.
2. Run the client in a memory-constrained environment — a container with cgroup v2 `memory.max`, a 32-bit process, or simply a busy host.
3. `read_plp_bytes` allocates the whole value with infallible `Vec` APIs. When the allocation fails, the process aborts instead of returning a `TdsResult` error.

A hostile or malfunctioning server can drive this deliberately: `MAX_PLP_SIZE` permits a 2 GB declared length, and nothing requires the server to be honest about it.

### Expected behavior

An allocation the driver cannot satisfy surfaces as a diagnostic the caller can act on — `Error::ProtocolError` / an allocation error in `mssql-tds`, `HY001` in `mssql-odbc` — leaving the connection in a defined state. A driver loaded into someone else's process should never take that process down for a value it can simply refuse.

### Actual behavior

`abort()` via Rust's OOM handler. No diagnostic, no unwind, no chance for the host application to recover.

On Linux the failure mode is worse than on Windows, which is the opposite of what one might assume. Windows returns a failed commit, so `try_reserve` yields `Err` and a clean `HY001` is possible. Under default Linux overcommit (`vm.overcommit_memory=0`) the allocation *succeeds* and the OOM killer later picks a victim by `oom_score` — **which may not be our process**. Unbounded growth inside the driver can get an unrelated process on the box killed. This matters here because `mssql-odbc` ships a wider Linux matrix than Windows: three libc/OpenSSL tracks across x64 and arm64 (`mssql-odbc/tests/e2e/README.md:398-405`), reused on Debian bookworm, Ubuntu 22.04/24.04, Azure Linux 3, Alpine 3.18-3.21, and RHEL 8 / UBI 8.

### Version

commit d7ba300c (`origin/main`)

### Affected crate

Not applicable / Multiple

### Environment

- All supported platforms. Linux is the more severe case (see Actual behavior).
- Sites below are line-accurate against `main` @ d7ba300c; none of them are touched by #444.

### Additional context

#### Inventory, highest risk first

**1. PLP unknown-length accumulation — `mssql-tds/src/datatypes/decoder.rs:2040`**

```rust
plp_buffer.reserve(chunk_len); // infallible
```

Structurally the same bug #444 fixed: accumulate-across-chunks with no declared total. `MAX_PLP_SIZE` bounds the total at 2 GB, but reaching it through infallible `reserve` aborts. **Server-controlled**, so unlike the `SQLPutData` case the trigger does not require a cooperating application. I would rank this above the instance #444 fixed.

**2. PLP known-length — `mssql-tds/src/datatypes/decoder.rs:1984`**

```rust
let mut plp_buffer = vec![0u8; length]; // length only validated <= MAX_PLP_SIZE at :2100
```

A server declaring a 2 GB value gets a 2 GB infallible allocation. Secondary issue: this zero-fills all 2 GB immediately before `read_plp_chunks_into_slice` overwrites it — exactly the waste the sibling unknown-length path's comment at `:2035-2039` explains it avoids by using `spare_capacity_mut`. The two paths disagree with each other.

**3. `SQLGetData` payload — `mssql-odbc/src/api/get_data.rs:866`**

```rust
let mut payload = vec![0u8; max_read]; // derived from the application's BufferLength
```

Lower severity, since the application implicitly claims to own a buffer that size. Still aborts on a bogus `BufferLength` rather than returning `HY090`/`HY001`.

**4. `read_wchar_bytes` — `mssql-odbc/src/conversion/param_buffer.rs:261`**

```rust
let mut bytes = Vec::with_capacity(units * std::mem::size_of::());
```

The non-DAE analogue of the #444 bug: `SQLBindParameter` with a large `StrLen_or_IndPtr`. Single-shot and bounded by the application's claimed buffer, so mild. (`units * 2` cannot overflow, since `units = len_spec / 2`.)

**Not issues**, checked and cleared: `get_desc_rec.rs:206` is inside `#[cfg(test)]`; `auth/msqa.rs:597` takes a `u32` from the local auth library; the `String::with_capacity` calls in `api/catalog.rs`, `api/current_catalog.rs`, and `api/util.rs` are all bounded by input length; the `read_value_into!` fallback at `decoder.rs:120` is guarded by `MAX_ALLOC_SIZE` checks at every call site, so 100 MB infallible is low priority.

#### Existing in-tree precedent

`try_reserve` appears in exactly two places repo-wide: `mssql-odbc/src/api/fetch_scroll.rs:1391` (pre-existing) and the new one in #444. `fetch_scroll.rs` has the shape worth copying — a policy cap (`PLP_TYPED_MATERIALIZE_LIMIT`, `typed_plp_chunk_fits` at `:1265`) *plus* `try_reserve`, degrading to `RowIssue::Unsupported` rather than failing hard.

#### Proposed direction

Ordered by what actually closes the gap. Note that items 1 and 3 are the ones that work on Linux; `try_reserve` alone does not.

1. **Cap the value; treat `try_reserve` as the backstop, not the fix.** A policy limit is platform-independent and deterministic because you never approach the allocator's failure point at all. `PLP_TYPED_MATERIALIZE_LIMIT` is the in-tree model. This is the highest-value change.

2. **Prefer streaming with a bounded residual over materializing.** Carrying a partial code point across chunks (at most 3 bytes for UTF-8, one unit for a high surrogate) makes memory O(chunk) instead of O(value). The read path already does exactly this — `utf16le_chunk_to_utf8`'s `pending_byte` / `pending_high_surrogate` at `get_data.rs:1176`. Mirroring it on the write path is the real fix that #444 deliberately deferred.

3. **Budget cumulatively, not per call.** A per-call `try_reserve` does not stop 50 statements each buffering 100 MB. A connection- or environment-scoped `AtomicUsize` byte budget checked before reserving is deterministic on every platform, needs no nightly features, and is unit-testable without involving the OS.

4. **Watch the speculative 2x.** `Vec::try_reserve` deliberately over-allocates (amortized doubling), so growing to 2 GB peaks near 3 GB — 1 GB still live plus a 2 GB new allocation — plus a 1 GB memcpy. Where the total is known up front (PLP `Known(length)`, `SQL_LEN_DATA_AT_EXEC(n)`) reserve once, exactly. Where it is not (`SQL_DATA_AT_EXEC`) amortized growth is correct and `try_reserve_exact` per chunk would be quadratic, so #444's choice is right — but any cap should be chosen knowing peak is roughly 2x the cap.

5. **Reject on the declared total before buffering a byte.** `SQL_LEN_DATA_AT_EXEC(n)` supplies `n` at `SQLExecute` time; validating it against the cap there is the cheapest possible failure.

6. **Explicitly do not install a custom global allocator.** A `GlobalAlloc` wrapper with a byte ceiling would make `try_reserve` deterministic on Linux regardless of overcommit, and is exactly wrong for a `.so`/`.dll` loaded into someone else's process. Acceptable for `mssql-tds-cli` or fuzz targets only. Recording it here so nobody reaches for it. The scoped alternative (`allocator_api`, `Vec::new_in`) is still nightly-only.

7. **Document, do not depend on, deployment knobs.** `vm.overcommit_memory=2`, cgroup v2 `memory.max`, `RLIMIT_AS`. `RLIMIT_AS` is genuinely useful as a **test harness** for exercising the fallible paths deterministically on Linux, but a driver cannot set it for its host.

#### Related

- [AB#40371](https://sqlclientdrivers.visualstudio.com/mssql-rs/_workitems/edit/40371) (Closed, Critical) — established the caps; this issue is its un-closed half.
- [AB#45312](https://sqlclientdrivers.visualstudio.com/mssql-rs/_workitems/edit/45312) (New) — "Add an OOM-resilient fallback diagnostic path like msodbcsql's `ERR_STATUS_OOM`". The *reporting* counterpart: how to surface an OOM once detected, versus this issue's *detection* half. Natural sibling; worth linking both ways.
- [AB#40361](https://sqlclientdrivers.visualstudio.com/mssql-rs/_workitems/edit/40361) (Closed) — the `checked_add` capacity-overflow guards.
- #444 — fixed the `SQLPutData` / `pending_bytes` instance.
- #358 (Closed) — removed the PLP chunk-*count* cap, producing today's design where the total is bounded only by `MAX_PLP_SIZE`.
- #462 — enforce the no-panics rule via clippy. Adjacent but does not cover this: an abort from infallible allocation is invisible to `unwrap_used` / `expect_used` / `panic`.

Contributor guide

Open the contributing guide

Research direction

Start with the affected allocation sites in mssql-tds/src/datatypes/decoder.rs, then compare the fallible handling in mssql-odbc/src/api/fetch_scroll.rs and the fix from #444. Review the additional sites in api/get_data.rs and conversion/param_buffer.rs. Done means allocation failure is handled without aborting the host and returns the documented diagnostic while respecting an explicit policy.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust, sql
Domain
backend, databases, security
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.