microsoft / microsoft/mssql-rs
mssql-odbc: fully close the effective_ard/effective_apd use-after-free race against concurrent SQLFreeHandle(SQL_HANDLE_DESC)
- 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
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