microsoft / microsoft/mssql-rs
mssql-odbc: SQLSetDescRec/SQLSetDescFieldW can rewrite ARD/APD records mid-fetch with no STMT_STATE_FETCH_IN_PROGRESS guard
Nobody has claimed this yet.
- 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 code review.
## Problem
Now that the ARD/APD records *are* the binding storage ([AB#47437](https://sqlclientdrivers.visualstudio.com/b95cf060-8083-439d-8ef1-405d5bf219d8/_workitems/edit/47437)), `SQLSetDescRec` and `SQLSetDescFieldW` can rewrite `SQL_DESC_DATA_PTR` / `SQL_DESC_OCTET_LENGTH` / `SQL_DESC_CONCISE_TYPE` on a descriptor a fetch is actively using, with no `STMT_STATE_FETCH_IN_PROGRESS` guard. Every other entry point that touches the same records refuses in that window:
- `SQLBindCol` (`bind_col.rs:109`)
- `SQLFreeStmt(SQL_UNBIND)` (`bind_col.rs:315`)
- `SQLSetStmtAttr` (`set_stmt_attr.rs:164`)
`fetch_scroll_safe` snapshots `ColumnBinding::all_from_ard_state` before its fill loop, so the loop itself can't read a half-written record — but that snapshot is precisely why the guard exists on `SQLBindCol` in the first place: the point is that the application must not be able to hand the driver a *new* binding (and free the old buffer) while the fill loop is still writing through the snapshotted pointers. The descriptor-field API is now a second door into the same records, and before PR #436 it wasn't.
## Why this wasn't fixed directly in #436
A `DescHandle` has no back-pointer to the statement(s) it is associated with, so adding the guard to `SQLSetDescRec`/`SQLSetDescFieldW` needs a DBC → STMT walk, the same shape `free_desc` already uses to reset associations on free. Doing that walk for one door (the descriptor-field API) and not the others inconsistently, or rushing a partial version of it, risks being worse than the current, at-least-consistent gap. It needs the same careful DBC→STMT-ordered treatment as the rest of this crate's locking rules, not a quick patch alongside an already large PR.
## Suggested approach
Give `SQLSetDescRec`/`SQLSetDescFieldW` (`set_desc_rec.rs`, `set_desc_field.rs`) the same DBC → STMT walk `free_desc` uses (`.github/instructions/mssql-odbc.instructions.md`'s "Locking rules" section) to find every statement currently associated with the descriptor being modified (an explicit ARD/APD can be shared across statements via `SQL_ATTR_APP_ROW_DESC`/`SQL_ATTR_APP_PARAM_DESC` reassociation), and refuse with `ERR_FUNCTION_SEQUENCE` if any of them has `STMT_STATE_FETCH_IN_PROGRESS` set — matching `SQLBindCol`'s existing check.
## References
- PR: https://github.com/microsoft/mssql-rs/pull/436
- Review thread: https://github.com/microsoft/mssql-rs/pull/436#discussion_r3913342575
- Related work item: [AB#47437](https://sqlclientdrivers.visualstudio.com/b95cf060-8083-439d-8ef1-405d5bf219d8/_workitems/edit/47437)
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the locking rules in .github/instructions/mssql-odbc.instructions.md, then compare free_desc with SQLBindCol in bind_col.rs. Update set_desc_rec.rs and set_desc_field.rs to walk associated statements in DBC→STMT order and reject changes when any has STMT_STATE_FETCH_IN_PROGRESS, returning ERR_FUNCTION_SEQUENCE as SQLBindCol does.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 55/100