microsoft / microsoft/mssql-rs

mssql-odbc: fully close the effective_ard/effective_apd use-after-free race against concurrent SQLFreeHandle(SQL_HANDLE_DESC)

Open
#441 0 comments 0 reactions 0 assignees View on GitHub
mssql-odbc
Dominant language
Rust
Stars
53
Forks
14
Avg merge
1d 15h
Merged PRs (30d)
137

Description

## Context

Follow-up from [PR #436](https://github.com/microsoft/mssql-rs/pull/436) ([AB#47437](https://sqlclientdrivers.visualstudio.com/b95cf060-8083-439d-8ef1-405d5bf219d8/_workitems/edit/47437) — descriptor records as the binding/metadata source of truth), flagged by Copilot code review.

## Problem

Several entry points resolve a statement's *effective* ARD/APD (`StmtState::effective_ard`/`effective_apd`) — which can be an **explicit** descriptor reassociated via `SQLSetStmtAttrW(SQL_ATTR_APP_ROW_DESC/APP_PARAM_DESC, ...)` — as a raw pointer while holding the STMT lock, then drop that lock before dereferencing the pointer via `handle_from_raw::`. This is required by this crate's own locking-order rule (never hold a STMT lock while acquiring a DESC lock, to avoid an ABBA deadlock against `free_desc`'s DBC→STMT walk).

If a concurrent `SQLFreeHandle(SQL_HANDLE_DESC)` on that same explicit descriptor completes in the window between the pointer being resolved and it being dereferenced, the later dereference is a genuine use-after-free: `free_handle` calls `Box::from_raw` and drops the allocation, and `handle_from_raw` is an unchecked raw pointer cast with no liveness check of its own.

Affected call sites (all fixed with the mitigation below in #436):
- `bind_col.rs`: `bind_ard_column`, `unbind_ard_column`, `sql_free_stmt_unbind_safe`
- `bind_param.rs`: `bind_param_records`, `sql_free_stmt_reset_params_safe`
- `exec_common.rs`: `snapshot_bound_params`
- `fetch_scroll.rs`: the ARD bindings read in `fetch_scroll_safe`

## Current mitigation (in #436)

Each site now re-checks `crate::handles::live_type(ptr) == Some(HandleType::Desc)` immediately before dereferencing, converting the overwhelming majority of "freed out from under us" timings from undefined behavior into a clean `SQL_ERROR`. This **narrows but does not fully close** the race: there is still a check-then-use gap between the liveness check and the lock acquisition/dereference.

## Why a full fix wasn't attempted in #436

Fully closing this requires either:
- Holding a lock across the *entire* resolve-through-dereference sequence (e.g. the owning DBC's lock, which `free_desc` also acquires before clearing a statement's association) — a broader lock-scope change across every affected call site, with its own risk of new contention or deadlocks if any other path expects DESC operations to be independent of DBC-lock duration.
- Moving to shared ownership for descriptor associations (e.g. `Arc>` instead of a raw `*mut c_void` handle + registry-based liveness check) — a larger structural change to the whole handle system, not scoped to descriptors alone.

Neither is safe to attempt without live multi-threaded stress-testing, which wasn't available in the environment #436 was developed in.

## Suggested approach

Investigate holding the owning DBC's lock across the full resolve-then-use sequence at each affected call site (DBC→STMT→DESC, matching `free_desc`'s own ordering), as the reviewer originally suggested, and validate under a concurrent/stress test harness (multiple threads racing `SQLBindCol`/`SQLBindParameter`/`SQLFetchScroll` against `SQLFreeHandle(SQL_HANDLE_DESC)` on a shared explicit descriptor) before merging.

## References

- PR: https://github.com/microsoft/mssql-rs/pull/436
- Review thread: https://github.com/microsoft/mssql-rs/pull/436#discussion_r3899365000 (see also the mitigation commit for the exact call sites and regression tests)
- Related work item: [AB#47437](https://sqlclientdrivers.visualstudio.com/b95cf060-8083-439d-8ef1-405d5bf219d8/_workitems/edit/47437)

Contributor guide

Open the contributing guide

Research direction

Start by reviewing the effective_ard/effective_apd call sites in bind_col.rs, bind_param.rs, exec_common.rs, and fetch_scroll.rs, along with free_desc and the mitigation commit from PR #436. Run the existing regression tests referenced there, then validate changes with a concurrent stress harness racing descriptor use against SQLFreeHandle(SQL_HANDLE_DESC). Done means the resolve-through-dereference race is fully closed without introducing lock-order or contention problems.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
database
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.